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

Path with a complex filter match only the first occurrence #560

Merged
merged 7 commits into from
Nov 30, 2023
Merged

Path with a complex filter match only the first occurrence #560

merged 7 commits into from
Nov 30, 2023

Conversation

leonardo-speranzon
Copy link
Contributor

@leonardo-speranzon leonardo-speranzon commented Nov 27, 2023

Description

To solve this bug i changed slightly the logic of navigate, applyRemoveOperation() and applyRemoveOperation() to work with multiple paths (arrays of paths) instead of a single one.
I've added some test for checking the cases discussed in the issue.

In the new code there is FIXME describing a possible unwanted error, but it should be only caused by some schemas out of the SCIM standard (with depth greater than 1). If it is ok i can remove the comment otherwise i can try to fix it

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking changes (change that is not backward-comptible and/or changes current functionality)

Closes issue(s)

Resolve #503

Checklist

  • I have tested this code
  • I have added unit test to cover this code
  • I have updated the Readme
  • I have followed the contributing guide

- navigate return a list of sub-schemas
- `applyRemoveOperation` and `applyAddOrReplaceOperation` adapted to the new signature (and logic)
This reverts commit e8354a6.
"REPLACE: replace on multiValued objects without complete path"
thomaspoignant
thomaspoignant previously approved these changes Nov 30, 2023
Copy link
Owner

@thomaspoignant thomaspoignant left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this contribution this is awesome and it fixes an important loop hole.

I've just put some NIT comments about some commented logs if you can have a look.
As soon as you've fixed it I will create a new version.

src/scimPatch.ts Outdated Show resolved Hide resolved
src/scimPatch.ts Outdated Show resolved Hide resolved
@leonardo-speranzon
Copy link
Contributor Author

Oh i hadn't noticed those comments, i will remove them as soon as i can.

Copy link

sonarcloud bot commented Nov 30, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@thomaspoignant thomaspoignant merged commit 94d330b into thomaspoignant:master Nov 30, 2023
5 checks passed
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.

Path with a complex filter match only the first occurrence
2 participants