-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feat serialize attachments #14
Conversation
af9eb40
to
44f14dc
Compare
First of all: Good catch - seems like there is obviously something missing in the unit tests, otherwise we would have known that the checksum calculation can't work with the current approach. @huyenngn could you add a unit test for this? Regarding the checksum field of the work item: I like the idea of knowing whether the work item itself changed or just the attachments. With the current approach we always have to update everything. But the solution in this PR does not really solve the problem for changes of the attachment content, right? We now know whether the attachments or the work item changed, but as the checksum field is part of the work item, we would still have to patch the work item if one of the attachments changes. And as we don't know, which attachment changed, we would still have to update all attachments whenever one changed. I think it would be beneficial to split the checksums, if we had one checksum per attachment. This way, if we have I would propose to reduce this PR to the bug fix of non json serializable bytes and implement the split of the checksum separately. This way we can get rid of a bug and merge this PR soon 👍 |
44f14dc
to
63a350e
Compare
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.
I think we should include the attachments in the checksum calculation. In another PR we could try to implement separate checksums per attachment. In addition, I would prefer to not expect that all content-bytes can be utf-8 decoded. We could use base64 instead, but maybe you have another good idea? 🙂
63a350e
to
173eb79
Compare
173eb79
to
3feeedb
Compare
Work item attachments take content as bytes.
When calculating the workitem checksum a JSON dump is performed but bytes aren't JSON serialisable.
Regarding checksum, when checksum of attachments differ doesn't mean the whole work item needs to be patched and if work item checksum differs doesn't necessarily mean attachments need patching either.