Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

bug: URL Query parameter not bind to Optional proto message field #4660

Closed
afifurrohman-id opened this issue Aug 26, 2024 · 9 comments
Closed

Comments

@afifurrohman-id
Copy link

afifurrohman-id commented Aug 26, 2024

🐛 Bug Report

When using optional field as request message and will be expected to bind with HTTP url query parameter the optional field always nil

To Reproduce

  • buf.gen.yaml
version: v2
plugins:
  - local: protoc-gen-go
    out: .
    opt: paths=source_relative
  - local: protoc-gen-go-grpc
    out: .
    opt: paths=source_relative
  - local: protoc-gen-grpc-gateway
    out: .
    opt: paths=source_relative

inputs:
  - directory: .
  • buf.yaml
version: v2
name: example.com/foo
deps:
  - buf.build/googleapis/googleapis
  • svc.proto
syntax = "proto3";

import "google/api/annotations.proto";
import "google/protobuf/empty.proto";

message DeleteFooRequest {
    string foo_id = 1;
    optional bool soft_delete = 2;
}

service ProductService {

    rpc DeleteFoo(DeleteFooRequest) returns (google.protobuf.Empty) {
        option (google.api.http) = {
            delete: "/foo/{foo_id}"
        };
    }

}
  • Generate stub
buf generate
  • Create normal grpc gateway server
  • Test using curl with url?soft_delete=true or url?soft_delete=false

Expected behavior

it should either false or true

Actual Behavior

Soft delete it will always nil

Your Environment

Linux GRPC-GATEWAY 5.15.153.1-microsoft-standard-WSL2 #1 SMP Fri Mar 29 23:14:13 UTC 2024 x86_64 GNU/Linux

@johanbrandhorst
Copy link
Collaborator

Hi. Thanks for the example. Does it work if you don't use optional?

@afifurrohman-id
Copy link
Author

Hi. Thanks for the example. Does it work if you don't use optional?

Yes, it works as expected

@johanbrandhorst
Copy link
Collaborator

Ah, very cool! We haven't yet put time into supporting the optional keyword, so this sounds like a bug. The code you want to look at is in

func (*DefaultQueryParser) Parse(msg proto.Message, values url.Values, filter *utilities.DoubleArray) error {
for key, values := range values {
if match := valuesKeyRegexp.FindStringSubmatch(key); len(match) == 3 {
key = match[1]
values = append([]string{match[2]}, values...)
}
msgValue := msg.ProtoReflect()
fieldPath := normalizeFieldPath(msgValue, strings.Split(key, "."))
if filter.HasCommonPrefix(fieldPath) {
continue
}
if err := populateFieldValueFromPath(msgValue, fieldPath, values); err != nil {
return err
}
}
return nil
}
. I would start by adding a failing test case to https://github.com/grpc-ecosystem/grpc-gateway/blob/main/runtime/query_test.go. Contributions accepted!

@afifurrohman-id afifurrohman-id changed the title bug: URL Query parameter not bind to Optional proto message field bug: URL Query parameter not bind to Optional proto message field Aug 28, 2024
@lemonnn-8
Copy link

@johanbrandhorst if no one is working on this I can take this up :)

@johanbrandhorst
Copy link
Collaborator

Please go for it!

@lemonnn-8
Copy link

lemonnn-8 commented Oct 13, 2024

Hey @johanbrandhorst @afifurrohman-id

So I did some investigation and created a demo as per as bug reported. But it was working fine for me. I am attaching repo, so you guys can let me know if something is wrong from my side.

https://github.com/lemonnn-8/grpc-gateway-demo

looking forward to your reply :)

@afifurrohman-id
Copy link
Author

afifurrohman-id commented Oct 14, 2024

So I did some investigation and created a demo as per as bug reported. But it was working fine for me. I am attaching repo, so you guys can let me know if something is wrong from my side.

I can confirm now this working, i also re-generate the stubs output.
But, why it works, when grpc-gateway itself doesn't put a release and i didn't find update in googleapis that fixed specific http.proto ?

i can see that buf registry update the googleapis dep: https://buf.build/googleapis/googleapis/commits/commit/e7f8d366f5264595bcc4cd4139af9973 but is not related to http.proto

but i think googleapis http.proto is not the problem, the problem is the parser itself.

@lemonnn-8
Copy link

@johanbrandhorst I think we can close this issue then

@afifurrohman-id
Copy link
Author

Update, after long debugging i found where the problem comes.

The gateway and service is separate program, and i forgot to restart the gateway after re-generate the stubs that used by service and gateway.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

4 participants
@johanbrandhorst @lemonnn-8 @afifurrohman-id and others