Skip to content

feat: mergepatch to support partial update of nested objects #1891

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 21 commits into from
May 20, 2025
Merged

feat: mergepatch to support partial update of nested objects #1891

merged 21 commits into from
May 20, 2025

Conversation

sofyalaski
Copy link
Member

@sofyalaski sofyalaski commented May 9, 2025 •
edited
Loading

Description

As discussed in issue #954, this PR implements support for application/merge-patch+json type of content on requests to update endpoints. Support is added to the newer V4 endpoints for now.

Motivation

easier updates, especially of scientific metadata

Changes:

  • application/merge-patch+json content-type support
  • application/json content-type remains
  • in the controller function body it's checked what content-type user sends in the request, and if it's merge-patch+json, additional merge with the old document is applied
  • changes are applied only to the new V4 endpoints for now
  • for datasets, specific to ScientificMetadata and its interception, additional check is added that doesn't allow updating either value/unit without providing the second, when it was originally present in the dataset, to avoid user errors and keep functionality of the interceptor
  • eslint applied to jobs test files

Tests included

  • Included for each change/fix?
  • Passing?

Documentation

  • swagger documentation updated (required for API changes)
  • official documentation updated

official documentation info

@sofyalaski sofyalaski requested review from despadam and sbliven May 9, 2025 11:57
@sofyalaski sofyalaski marked this pull request as ready for review May 12, 2025 13:29
despadam
despadam reviewed May 13, 2025
View reviewed changes
Copy link
Member

@despadam despadam left a comment

Choose a reason for hiding this comment

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

I left some comments/questions. Once clarified I will review again.

The warnings about using any perhaps can be resolved if we use unknown instead.

@sofyalaski sofyalaski requested a review from despadam May 13, 2025 16:08
despadam
despadam requested changes May 14, 2025
View reviewed changes
@sofyalaski sofyalaski requested a review from Junjiequan May 15, 2025 09:02
Junjiequan
Junjiequan approved these changes May 19, 2025
View reviewed changes
Copy link
Member

@Junjiequan Junjiequan left a comment

Choose a reason for hiding this comment

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

I've left one comment, otherwise looks good to me.
I assigned @martin-trajanovski to review also, as he developed datasets v4 controller he knows it better than me.

@sofyalaski
Copy link
Member Author

Thank you Jay!

martin-trajanovski
martin-trajanovski reviewed May 19, 2025
View reviewed changes
@sofyalaski
Copy link
Member Author

sofyalaski commented May 19, 2025 •
edited
Loading

Thank you @martin (also for pointing out the utils function). The present isObject checks that the object does not contain the units, so it's the opposite of what I need. I put my function in the utils. Is it okay if the rest stay in the controller or should I move them too?

@martin-trajanovski
Copy link
Collaborator

Thank you @martin (also for pointing out the utils function). The present isObject checks that the object does not contain the units, so it's the opposite of what I need. I put my function in the utils. Is it okay if the rest stay in the controller or should I move them too?

I think you can move just those two that are more general ones like isRecord and isValueUnitObject. Other ones can stay as they are more specific to the dataset controller.

sbliven and others added 6 commits May 20, 2025 08:20
@sbliven @sofyalaski
feat: uniform handlebar variables for job actions (SciCatProject#1855)
b037a85 
* Provide a consistent context to all job actions

The change is most visible in handlebars templates. Before, `{{this}}`
would correspond to different objects depending on how the action is
called; either a DTO or a Job or even a Dataset. Now these are accessed
consistently through top-level fields; thus `{{id}}` becomes `{{job.id}}`,
`{{jobParams}}` becomes `{{request.jobParams}}`, etc.

This also adds access to environmental variables via `{{env}}`, making
it easier to include secrets actions without hard-coding them.

Tests and example files should be up-to-date.

* Add datasets to the context

Datasets get cached in the context, meaning that they only need to be
fetched once for all actions.
@sofyalaski
Revert "increase timeout"
6efa783 
This reverts commit feb0d6e.
@sofyalaski
Revert "add timeout to jest test"
4b391d5 
This reverts commit b10cb99.
@sofyalaski
Revert "feat: uniform handlebar variables for job actions (SciCatProj…
9d860d0 
…ect#1855)"

This reverts commit b037a85.
@sofyalaski
Delete test/config/.env.override
71d7047
@sofyalaski
Merge remote-tracking branch 'origin/master' into 954-add-mergepatch
961440d
@sofyalaski sofyalaski merged commit 3acdbd9 into SciCatProject:master May 20, 2025
13 checks passed
@sofyalaski sofyalaski mentioned this pull request May 21, 2025
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martin-trajanovski martin-trajanovski martin-trajanovski left review comments

@Junjiequan Junjiequan Junjiequan approved these changes

@despadam despadam despadam approved these changes

@sbliven sbliven Awaiting requested review from sbliven

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sofyalaski @martin-trajanovski @despadam @Junjiequan @sbliven