fix: resolve paths where filter value contains a colon #1021
Conversation
Summary of ChangesHello @ZeRego, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in SCIM patch operations where paths containing filter values with colons were incorrectly parsed, leading to failed resolutions. The changes introduce a more robust path resolution mechanism that accurately distinguishes between URN prefixes and colons embedded within filter values, ensuring that patch operations function as expected in such scenarios. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a bug where colons in filter values were incorrectly interpreted as part of a URN schema. The fix, which involves using a regular expression to identify a URN prefix before searching for a colon, is a solid approach. My review includes a suggestion to simplify the code for better readability and a recommendation to expand test coverage to ensure the fix is effective for all relevant operations. Overall, this is a well-executed bug fix.
| it('REPLACE: should handle colon in quoted filter values', done => { | ||
| scimUser.roles = [ | ||
| { value: "3675b69e-8324-4110-bdca-059031aa8da3:admin", type: "admin" }, | ||
| { value: "other-role", type: "user" } | ||
| ]; | ||
| const expected = "updated"; | ||
| const patch: ScimPatchAddReplaceOperation = { | ||
| op: 'replace', | ||
| value: expected, | ||
| path: 'roles[value eq "3675b69e-8324-4110-bdca-059031aa8da3:admin"].value' | ||
| }; | ||
| const afterPatch = scimPatch(scimUser, [patch]); | ||
| expect(afterPatch.roles![0].value).to.be.eq(expected); | ||
| expect(afterPatch.roles![1].value).to.be.eq("other-role"); | ||
| return done(); | ||
| }); |
There was a problem hiding this comment.
This is a great test case for the replace operation. To ensure the fix is robust and covers all scenarios, I recommend adding similar tests for the add and remove operations, as they also rely on the resolvePaths function. This will help prevent future regressions.
Here are some examples:
For add:
it('ADD: should handle colon in quoted filter values', done => {
scimUser.roles = [
{ value: "3675b69e-8324-4110-bdca-059031aa8da3:admin", type: "admin" }
];
const patch: ScimPatchAddReplaceOperation = {
op: 'add',
value: 'new-type',
path: 'roles[value eq "3675b69e-8324-4110-bdca-059031aa8da3:admin"].type'
};
const afterPatch = scimPatch(scimUser, [patch]);
// 'add' on a single-valued attribute replaces it
expect(afterPatch.roles![0].type).to.be.eq('new-type');
return done();
});For remove:
it('REMOVE: should handle colon in quoted filter values', done => {
scimUser.roles = [
{ value: "3675b69e-8324-4110-bdca-059031aa8da3:admin", type: "admin" },
{ value: "other-role", type: "user" }
];
const patch: ScimPatchRemoveOperation = {
op: 'remove',
path: 'roles[value eq "3675b69e-8324-4110-bdca-059031aa8da3:admin"]'
};
const afterPatch = scimPatch(scimUser, [patch]);
expect(afterPatch.roles!.length).to.be.eq(1);
expect(afterPatch.roles![0].value).to.be.eq("other-role");
return done();
});There was a problem hiding this comment.
Seems unnecessary. imo, resolvePaths function could be exported so we could have unit test for it instead of integration tests across all operation methods.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
2 participants
Description
Correctly resolve paths where filter value contains
:Changes include
Closes issue(s)
Resolve: #1020
Checklist