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?
Updates JSON merge patch and
fromJsoncode to use bitwise tracking instead of aSet<String>and abooleanper tracking instance.