Skip to content

fixes customizer logic in the GsonGrpcJsonMarshallerBuilder#6146

Merged
trustin merged 10 commits intoline:mainfrom
AnyRoad:fix_gson_grpc_json_marshaller_builder
Mar 21, 2025
Merged

fixes customizer logic in the GsonGrpcJsonMarshallerBuilder#6146
trustin merged 10 commits intoline:mainfrom
AnyRoad:fix_gson_grpc_json_marshaller_builder

Conversation

@AnyRoad
Copy link
Contributor

@AnyRoad AnyRoad commented Mar 9, 2025

Motivation:

  • JsonFormat.Parser and JsonFormat.Printer classes are immutable and call of any method in customizer of the GsonGrpcJsonMarshallerBuilder creates new object.
  • Because of that result of the customer is ignored so we need a way to use new object created in the customizer.

Modifications:

  • Replaces Consumer which does not return anything with the Function<T, T> customizer which returns customized object.

Result:

  • We can actually customize JsonFormat.Parser and JsonFormat.Printer using the customizer object.

JsonFormat.Parser and JsonFormat.Printer classes are immutable and
calling any method in customizer creates new object.
Because of that result of the customer is ignored so we need a way
to use new object created in the customizer.
Replaces Consumer<T> with the Function<T, T> customizer which
returns customized object.
@CLAassistant
Copy link

CLAassistant commented Mar 9, 2025

CLA assistant check
All committers have signed the CLA.

@minwoox
Copy link
Contributor

minwoox commented Mar 10, 2025

Hi, @AnyRoad! Thanks for the PR!
Would you mind:

  • Signing CLA
  • Adding test cases that verities the differences?
  • Adding a new method instead of changing the signature of the existing method which breaks the compatibility?

@AnyRoad
Copy link
Contributor Author

AnyRoad commented Mar 13, 2025

Hi, @AnyRoad! Thanks for the PR! Would you mind:

* Signing CLA

* Adding test cases that verities the differences?

* Adding a new method instead of changing the signature of the existing method which breaks the compatibility?

Hi @minwoox !
I've signed the CLA, added unit tests and reverted back the original methods. Since they do not work properly I've marked them as deprecated.

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Thanks a lot, @AnyRoad! Left a couple minor comments about logging.

@AnyRoad AnyRoad requested a review from trustin March 15, 2025 11:55
Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Nice work! Thank you! 🙇

Copy link
Contributor

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Thanks! @AnyRoad 👍 👍 👍

@trustin trustin merged commit 5d711ee into line:main Mar 21, 2025
14 checks passed
@ikhoon ikhoon added defect and removed new feature labels Aug 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

Comments