Skip to content
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

Merged
merged 2 commits into from
Dec 19, 2023
Merged

Feat serialize attachments #14

merged 2 commits into from
Dec 19, 2023

Conversation

huyenngn
Copy link
Member

@huyenngn huyenngn commented Oct 10, 2023

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.

@huyenngn huyenngn requested review from micha91 and ewuerger and removed request for micha91 October 10, 2023 08:25
@huyenngn huyenngn force-pushed the feat-serialize-attachments branch from af9eb40 to 44f14dc Compare October 10, 2023 15:35
@huyenngn huyenngn requested review from micha91 and removed request for ewuerger October 11, 2023 07:03
Base automatically changed from feat-add-attachments-workitem-relations to main October 19, 2023 14:52
@micha91
Copy link
Collaborator

micha91 commented Oct 20, 2023

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 👍

@huyenngn huyenngn force-pushed the feat-serialize-attachments branch from 44f14dc to 63a350e Compare November 16, 2023 09:01
Copy link
Collaborator

@micha91 micha91 left a 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? 🙂

polarion_rest_api_client/data_models.py Outdated Show resolved Hide resolved
polarion_rest_api_client/data_models.py Outdated Show resolved Hide resolved
@huyenngn huyenngn force-pushed the feat-serialize-attachments branch from 63a350e to 173eb79 Compare November 20, 2023 12:18
@huyenngn huyenngn force-pushed the feat-serialize-attachments branch from 173eb79 to 3feeedb Compare December 15, 2023 08:38
@micha91 micha91 merged commit 30a4b84 into main Dec 19, 2023
6 checks passed
@ewuerger ewuerger deleted the feat-serialize-attachments branch January 17, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants