✨ Use JSON Merge Patch when doing a partial update on records #352
Conversation
Viicos
force-pushed
the
feature/json-merge-patch
branch
from
2bcaa2b to
7481d91
Compare
src/objects/api/utils.py
Outdated
| if v is None: | ||
| if k in target: | ||
| # remove the key/value pair from target | ||
| del target[k] |
There was a problem hiding this comment.
By looking at the tests, I see that this is the desired behaviour, but I wonder about the case where we want to set a key to a null value (for example if the key is required but can take null values) 🤔
There was a problem hiding this comment.
This is following the RFC implementation, but I agree when writing this I thought about this use case. So we could maybe deviate from the spec, depending on which use case we cant to be able to achieve
There was a problem hiding this comment.
I have a general question:
The standard https://datatracker.ietf.org/doc/html/rfc7396 explicitly says that it introduces the new content type application/merge-patch+json. In Objects API only application/json content type is supported.
If Objects API starts supporting RFC 7396 behavior, I think it should support its content type.
@joeribekker what do you think?
src/objects/api/serializers.py
Outdated
| if "data" in validated_data: | ||
| # Apply JSON Merge Patch for record data | ||
| validated_data["data"] = merge_patch( |
There was a problem hiding this comment.
It should be
- -1 level of indentation
- only for PATCH
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #352 +/- ##
==========================================
+ Coverage 94.78% 94.81% +0.02%
==========================================
Files 131 133 +2
Lines 4543 4568 +25
==========================================
+ Hits 4306 4331 +25
Misses 237 237 ☔ View full report in Codecov by Sentry. |
Viicos
force-pushed
the
feature/json-merge-patch
branch
from
13527cd to
cdeb100
Compare
Fixes #351