permitio / opal

Policy and data administration, distribution, and real-time updates on top of Policy Agents (OPA, Cedar, ...)
https://opal.ac
Apache License 2.0
4.48k stars 163 forks source link

add support for save_method PATCH #483

Closed thilak009 closed 1 year ago

thilak009 commented 1 year ago

Changes proposed

Add support for PATCH as a save_method, currently only PUT is supported as a save_method

Check List (Check all the applicable boxes)

Note to reviewers

Need thoughts/help on if there is a better way to do custom_encoder implementation, haven't used pydantic before

the reason i used it was because FastAPI does by default print response using JSON alias names but we are not returning response from FastAPI server but rather feeding the model directly to OPA in which case the field name would be used when doing a json.dumps instead of alias name

and also to remove default None value populated fields from the dumped JSON for any request that matches the JSONPatchAction type

One more thing is, OPA does not support move operation as a JSON patch operation, refer discussion and hence OPAL would also not support it what would be a good place in the docs to mentions this as a NOTE ?

netlify[bot] commented 1 year ago

Deploy Preview for opal-docs canceled.

Name Link
Latest commit f2899a176f2d51709041239799663d1e8706fde6
Latest deploy log https://app.netlify.com/sites/opal-docs/deploys/64ac4e7f065fff000844b2e3
orweis commented 1 year ago

One more thing is, OPA does not support move operation as a JSON patch operation, refer discussion and hence OPAL would also not support it what would be a good place in the docs to mentions this as a NOTE ?

I think this can go together with the full docs for this feature as a section in the data-updates tutorial and perhaps a quick reference in the data-sources article

thilak009 commented 1 year ago

I liked your solution to OpaClient's data cache. Unfortunately we don't have unit tests for it - have you tried (manually) testing the backup feature? (OFFLINE_MODE).

yep, i checked the updates in the JSON file manually when OFFLINE_MODE is enabled

thilak009 commented 1 year ago

also as Or Weis mentioned, adding this comment I am authorizing this contribution to open source