-
Notifications
You must be signed in to change notification settings - Fork 38
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
Fix message not restored in delete operations #284
Fix message not restored in delete operations #284
Conversation
@EricWittmann this fixes the linked issue. Let me know if it's the proper way of solving it or if I need to do something different |
"summary": "Subscribe summary" | ||
"summary": "Subscribe summary", | ||
"message": { | ||
"name": "bullshit" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use less offensive test data? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, copied from the issue test data.
JsonCompat.setPropertyNull(json, Constants.PROP_TRAITS); | ||
if (aaiNode.message != null) { | ||
Object messageJson = writeMessageBase(aaiNode.message); | ||
JsonCompat.setProperty(json, Constants.PROP_MESSAGE, messageJson); | ||
} else { | ||
JsonCompat.setPropertyNull(json, Constants.PROP_MESSAGE); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think the way this should work is that the message
child of the operation should be processed in a separate visit method called visitMessage
. The reason we set nulls for the child property values at this level is to force the ordering of the properties in the resulting JSON.
So if there's a bug here, it's likely to be in the visitMessage
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will revisit this and let you know my findings.
@carlesarnal reminder that this is still pending. :) |
Yes, sorry, it's on my to-do list but other priorities arose. Will revisit this asap. |
No no - not a priority. Just making sure it was still on your backlog. :) |
No description provided.