Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Provide a way to ignore json_name in HTTP/JSON transcoding #6082

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ikhoon
Copy link
Contributor

@ikhoon ikhoon commented Jan 31, 2025

Motivation:

There is a bug in protoc that always adds json_name to the descriptor. protocolbuffers/protobuf#5587
This happens by default when compiling proto files in Bazel. To mitigate this bug, I propose to add HttpJsonTranscodingQueryParamMatchRule.JSON_NAME to decide whether to use or ignore json_name when matching query params.

Modifications:

  • Add HttpJsonTranscodingQueryParamMatchRule.JSON_NAME
    • If set, the json_name field option is used to match query parameters.
    • This option is enabled by default. Users can exclude the json name field option by specifying only ORIGINAL_FIELD or LOWER_CAMEL_CASE
    • If users want to give lower priority to the json name, set [ORIGINAL_FIELD, JSON_NAME]. the json_name is used only when there is no matching original field.
  • Breaking) The json_name field option will only be used when HttpJsonTranscodingQueryParamMatchRule.JSON_NAME is explicitly specified via HttpJsonTranscodingOptionsBuilder.queryParamMatchRules(). By default, JSON_NAME is set.

Result:

Motivation:

There is a bug in protoc that always adds json_name to the descriptor.
This happens by default when compiling proto files in Bazel.
To mitigate this bug, I propose to add `HttpJsonTranscodingQueryParamMatchRule.IGORE_JSON`
to forcibliy ignore json_name when matching query params or path
variables.

Modifications:

- Add `HttpJsonTranscodingQueryParamMatchRule.INGORE_JSON`
  - If enabled, the `json_name` field option is ignored.

Result:

- Fixes line#5890
- You can now ignore the `json_name` field option when trancoding
  HTTP/JSON to gRPC messages.
@ikhoon ikhoon added the defect label Jan 31, 2025
@ikhoon ikhoon added this to the 1.32.0 milestone Jan 31, 2025
@@ -172,16 +174,20 @@ static GrpcService of(GrpcService delegate, HttpJsonTranscodingOptions httpJsonT
continue;
}

// TODO(ikhoon): Extract the build-time code into a separate class such as
// HttpJsonTranscodingServiceBuilder or HttpJsonTranscodingSpecGenerator
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class gets bigger and bigger. Looks like it needs some refactoring.

@ikhoon
Copy link
Contributor Author

ikhoon commented Feb 5, 2025

@mscheong01 I want to inform you of this change as you added the LOWER_CAMEL_CASE option. If you used LOWER_CAMEL_CASE with the json_name option field to match query parameters, this PR could be a breaking change in runtime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant