Update ProtobufJsonPayloadConverter.java #552
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bugfix - use preconfigured printer rather than default JsonFormat.printer();
mschielmann
requested review from
Sushisource,
mfateev and
vitarb
as code owners
|
|
Spikhalskiy
suggested changes
Jul 2, 2021
There was a problem hiding this comment.
Thank you for your contribution!
While this change totally makes sense and looks compatible, it will change the way we serialize photo field names. We use JsonFormat.printer() now and we will use JsonFormat.printer().preservingProtoFieldNames() if this PR applied as it is. Could you please edit the constructor to preserve the current behavior?
Make JsonFormat.printer() a default printer to preserve the current behaviour while using the default constructor.
|
hi @Spikhalskiy thank you for your review - good catch, I've made a change you've requested (I hope that's what you've meant). |
Spikhalskiy
approved these changes
Jul 2, 2021
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Regarding #551 - bugfix - use preconfigured printer rather than default JsonFormat.printer();
What was changed
Bugfix - In JavaSDK, ProtobufJsonPayloadConverter allows you to configure both printer and parser. Yet, the preconfigured printer has not been used to convert Object to data. Default JsonFormat printer has been used instead.
Why?
Bugfix to allow configurable printer to be used (as designed, it can be passed as constructor param).
Checklist
Closes ProtobufJsonPayloadConverter not using custom printer #551
How was this tested:
Passed custom JsonFormat.Printer to constructor and see it being used.