micha91
commented
10 months ago 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 n attachments, we would have to make 2 requests if a single attachment changes instead of n+1.
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 👍
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.