Change deserialization and merge patch to bitwise #2909
Conversation
alzimmermsft
requested review from
weidongxu-microsoft,
haolingdong-msft,
XiaofeiCao and
srnagar
as code owners
There was a problem hiding this comment.
@haolingdong-msft could you take review on this, as it is about json-merge-patch?
| /** | ||
| * Set the result property: Operation result upon success. | ||
| * | ||
| * @param result the result value to set. |
There was a problem hiding this comment.
Is it just changing the order?
There was a problem hiding this comment.
It is just a change in order but it's a good time to have a discussion on how we want to order overridden methods. Before this change, the methods defined the type came first then overridden methods, but in serialization code the properties defined by super types came first. It'd be nice to have a standard system for both serialization and getters/setters, such as properties and setters/getters defined by super types always come first (or last).
There was a problem hiding this comment.
To me, as long as the two orders (methods order and properties processing order during serialziation) are consistent, and both override methods and new methods have their own section, that would be fine.
I also asked copilot. It says override methods from super class comes first. So I would vote for this - override methods come first. :)
| @@ -1339,7 +1346,7 @@ private void addJsonMergePatchRelatedPropertyAndAccessors(JavaClass classBlock, | |||
| classBlock.javadocComment(comment -> | |||
| comment.description("Stores updated model property, the value is property name, not serialized name")); | |||
| addGeneratedAnnotation(classBlock); | |||
| classBlock.privateFinalMemberVariable("Set<String> updatedProperties = new HashSet<>()"); | |||
| classBlock.privateMemberVariable("long updatedProperties = 0L"); | |||
There was a problem hiding this comment.
Once concern from me is would bitwise make it harder to debug?
e.g. we found the serialization content is wrong, and we want to see if the updated properties are expected.
There was a problem hiding this comment.
It would make debugging moderately more difficult; it'd be a change from checking updatedProperties.contains(string) to doing a bitwise operation that matches the logic used by the setter. In both cases you'll still need to look at the setter or serialization code to see what's going on.
There was a problem hiding this comment.
Emm.. what if the model has more than 64 properties (it is unlikely but not impossible)? java.util.BitSet?
microsoft-github-policy-service
bot
added
the
no-recent-activity
|
Hi @alzimmermsft. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
|
Hi @alzimmermsft. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing "/reopen" if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the "no-recent-activity" label; otherwise, this is likely to be closed again with the next cleanup pass. |
Updates JSON merge patch and
fromJsoncode to use bitwise tracking instead of aSet<String>and abooleanper tracking instance.