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

Add endpoints for add/replace/delete metadata #46

Merged
merged 13 commits into from
Jul 7, 2022
Merged

Add endpoints for add/replace/delete metadata #46

merged 13 commits into from
Jul 7, 2022

Conversation

kamorel
Copy link
Contributor

@kamorel kamorel commented Jun 30, 2022

Description

Adds new endpoints to add, replace, or delete metadata from an object. Each call will create a copy (and thus new version) of the object with the changes applied.

Types of changes

New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING doc
  • I have checked that unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

@jujaga jujaga marked this pull request as draft June 30, 2022 18:25
Copy link
Member

@jujaga jujaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A ton of missing semicolon and Do not access Object.prototype method 'hasOwnProperty' from target object warnings. For the latter, you can always just attempt to access the property directly, and then evaluate the truthiness of the value to see if it exists or not; don't need to invoke the hasOwnProperty method at all.

Copy link
Contributor

@TimCsaky TimCsaky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good. Good that you did the controller and service as well for these!
i guess it would be better to add the tag related routes in another PR. And those controllers will work a bit differently.

not sure what we decided about the existing POST /object/:objId (to update an object) and whether you were able to modify metadata there as well. I think we discussed this: https://discord.com/channels/689896523848613952/689896524406194236/991494695609974855 probably could go in a separate PR. I think we should merge this soon.. unit tests could go in a separate PR.

@kamorel kamorel marked this pull request as ready for review July 6, 2022 17:02
@kamorel kamorel requested a review from jujaga July 6, 2022 17:02
Copy link
Member

@jujaga jujaga 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 may need to consider the deleteMetadata behavior some more. Current proposed implementation has it so that you must explicitly ask for each key to be deleted before it will be dropped. I see situations where it is possible the user would like to drop ALL metadata keys from an object but do not want to go back and explicitly figure out what was there. I think we should be able to support this use case - in which case, instead of returning a 422, its possible that by NOT specifying anything, we assume all metadata is to be dropped outside of our enforced values.

Let's chat a bit about some of the subtle edge cases offline but in general, it's looking pretty good otherwise. 👍

@kamorel kamorel requested a review from jujaga July 7, 2022 18:26
@jujaga jujaga merged commit c8cf21a into bcgov:master Jul 7, 2022
@kamorel kamorel deleted the feature/showcase-2639 branch September 1, 2022 22:51
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.

3 participants