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

* Build done

* Changed navigate() signature:
- navigate return a list of sub-schemas
- `applyRemoveOperation` and `applyAddOrReplaceOperation` adapted to the new signature (and logic)

* Add tests for cases where valuePath match multiple elements

* Revert "Build done"

This reverts commit e8354a6.

* Fixed case special case of replace:
"REPLACE: replace on multiValued objects without complete path"

* Removed commented console logs

---------

Co-authored-by: Thomas Poignant <[email protected]>

src/scimPatch.ts

@@ -168,14 +168,14 @@ function resolvePaths(path: string): string[] {
 
 function applyRemoveOperation<T extends ScimResource>(scimResource: T, patch: ScimPatchRemoveOperation): T {
     // We manipulate the object directly without knowing his property, that's why we use any.
-    let resource: Record<string, any> = scimResource;
+    let resources_scoped: Record<string, any>[];
     validatePatchOperation(patch);
 
     // Path is supposed to be set, there are a validation in the validateOperation function.
     const paths = resolvePaths(patch.path);
 
     try {
-        resource = navigate(resource, paths, {isRemoveOp: true});
+        resources_scoped = navigate(scimResource, paths, {isRemoveOp: true});
     } catch (error) {
         if (error instanceof InvalidRemoveOpPath) {
             return scimResource;
@@ -188,34 +188,38 @@ function applyRemoveOperation<T extends ScimResource>(scimResource: T, patch: Sc
 
     if (!IS_ARRAY_SEARCH.test(lastSubPath)) {
         // This is a mono valued property
-        if (!patch.value) {
-            // No value in the remove operation, we delete it.
-            delete resource[lastSubPath];
-            return scimResource;
+        for (const resource of resources_scoped) {
+            if (!patch.value) {
+                // No value in the remove operation, we delete it.
+                delete resource[lastSubPath];
+            } else {
+                // Value in the remove operation, we remove the children by value.
+                resource[lastSubPath] = removeWithPatchValue(resource[lastSubPath], patch.value);
+            }
         }
-
-        // Value in the remove operation, we remove the children by value.
-        resource[lastSubPath] = removeWithPatchValue(resource[lastSubPath], patch.value);
         return scimResource;
     }
+    for (const resource of resources_scoped) {
 
-    // The last element is an Array request.
-    const {attrName, valuePath, array} = extractArray(lastSubPath, resource);
+        // The last element is an Array request.
+        const {attrName, valuePath, array} = extractArray(lastSubPath, resource);
 
-    // We keep only items who don't match the query if supplied.
-    resource[attrName] = filterWithQuery<any>(array, valuePath, {excludeIfMatchFilter: true});
+        // We keep only items who don't match the query if supplied.
+        resource[attrName] = filterWithQuery<any>(array, valuePath, {excludeIfMatchFilter: true});
 
-    // If the complex multi-valued attribute has no remaining records, the attribute SHALL be considered unassigned.
-    if (resource[attrName].length === 0)
-        delete resource[attrName];
+        // If the complex multi-valued attribute has no remaining records, the attribute SHALL be considered unassigned.
+        if (resource[attrName].length === 0)
+            delete resource[attrName];
+    }
 
     return scimResource;
 }
 
 
 function applyAddOrReplaceOperation<T extends ScimResource>(scimResource: T, patch: ScimPatchAddReplaceOperation, treatMissingAsAdd: boolean): T {
     // We manipulate the object directly without knowing his property, that's why we use any.
-    let resource: Record<string, any> = scimResource;
+    // let resource: Record<string, any> = scimResource;
+    let resources_scoped: Record<string, any>[];
     validatePatchOperation(patch);
 
     if (!patch.path)
@@ -226,10 +230,10 @@ function applyAddOrReplaceOperation<T extends ScimResource>(scimResource: T, pat
     const lastSubPath = paths[paths.length - 1];
 
     try {
-        resource = navigate(resource, paths);
+        resources_scoped = navigate(scimResource, paths);
     } catch(e) {
         if (e instanceof FilterOnEmptyArray || e instanceof FilterArrayTargetNotFound) {
-            resource = e.schema;
+            const resource: Record<string, any> = e.schema;
             // check issue https://github.com/thomaspoignant/scim-patch/issues/42 to see why we should add this
             const parsedPath = parse(e.valuePath);
             if (isAddOperation(patch.op) &&
@@ -261,29 +265,37 @@ function applyAddOrReplaceOperation<T extends ScimResource>(scimResource: T, pat
         }
         throw e;
     }
-
     if (!IS_ARRAY_SEARCH.test(lastSubPath)) {
-        resource[lastSubPath] = addOrReplaceAttribute(resource[lastSubPath], patch);
-        return scimResource;
+        for (const resource of resources_scoped) {
+            resource[lastSubPath] = addOrReplaceAttribute(resource[lastSubPath], patch);
+        }
+        return scimResource;   
     }
 
+
+
     // The last element is an Array request.
-    const {valuePath, array} = extractArray(lastSubPath, resource);
+    for (const resource of resources_scoped) {
+
+        const {valuePath, array} = extractArray(lastSubPath, resource);
 
-    // Get the list of items who are successful for the search query.
-    const matchFilter = filterWithQuery<any>(array, valuePath);
+        // Get the list of items who are successful for the search query.
+        const matchFilter = filterWithQuery<any>(array, valuePath);
 
-    // If the target location is a multi-valued attribute for which a value selection filter ("valuePath") has been
-    // supplied and no record match was made, the service provider SHALL indicate failure by returning HTTP status
-    // code 400 and a "scimType" error code of "noTarget".
-    if (isReplaceOperation(patch.op) && matchFilter.length === 0) {
-        throw new NoTarget(patch.path);
+        // If the target location is a multi-valued attribute for which a value selection filter ("valuePath") has been
+        // supplied and no record match was made, the service provider SHALL indicate failure by returning HTTP status
+        // code 400 and a "scimType" error code of "noTarget".
+        if (isReplaceOperation(patch.op) && matchFilter.length === 0) {
+            throw new NoTarget(patch.path);
+        }
+        for (let i = 0; i < array.length; i++) {
+            // We are sure to find at least one index because matchFilter comes from array.
+            if(matchFilter.includes(array[i])){
+                array[i] = addOrReplaceAttribute(array[i], patch);
+            }
+        }
     }
 
-    // We are sure to find an index because matchFilter comes from array.
-    const index = array.findIndex(item => matchFilter.includes(item));
-    array[index] = addOrReplaceAttribute(array[index], patch);
-
     return scimResource;
 }
 
@@ -315,38 +327,45 @@ function extractArray(subPath: string, schema: any): ScimSearchQuery {
  * @param options options used while calling navigate
  * @return the parent object of the element we want to edit
  */
-function navigate(inputSchema: any, paths: string[], options: NavigateOptions = {}): Record<string, unknown> {
-    let schema = inputSchema;
+function navigate(inputSchema: any, paths: string[], options: NavigateOptions = {}): Record<string, unknown>[] {
+    let schemas: any[] = [inputSchema];
     for (let i = 0; i < paths.length - 1; i++) {
         const subPath = paths[i];
 
         // We check if the element is an array with query (ex: emails[primary eq true).
-        if (IS_ARRAY_SEARCH.test(subPath)) {
-            try {
-                const {attrName, valuePath, array} = extractArray(subPath, schema);
-                // Get the item who is successful for the search query.
-                const matchFilter = filterWithQuery<any>(array, valuePath);
-                // We are sure to find an index because matchFilter comes from array.
-                const index = array.findIndex(item => matchFilter.includes(item));
-                if (index < 0) {
-                    throw new FilterArrayTargetNotFound('A matching array entry was not found using the supplied filter.', attrName, valuePath, schema);
-                }
-                schema = array[index];
-            } catch (error) {
-                if(error instanceof FilterOnEmptyArray){
-                    error.schema = schema;
+        if (IS_ARRAY_SEARCH.test(subPath)) {           
+            schemas = schemas.flatMap((schema)=>{ 
+                try {
+                    const {attrName, valuePath, array} = extractArray(subPath, schema);
+                    // Get the item who is successful for the search query.
+                    const matchFilter = filterWithQuery<any>(array, valuePath);
+                    if (matchFilter.length === 0) {
+                        throw new FilterArrayTargetNotFound('A matching array entry was not found using the supplied filter.', attrName, valuePath, schema);
+                    }
+                    return matchFilter;
+                } catch (error) {
+                    /*
+                        FIXME In some edge cases, not fully compliant with SCIM, this can prematurely throw en error
+                        For example if we have a structure with multiValued complex
+                        attribute inside of a multiValued complex attribute, in
+                        this case this throw even if only a "branch" of the search fail.
+                    */
+                    if(error instanceof FilterOnEmptyArray){
+                        error.schema = schema;
+                    }
+                    throw error;
                 }
-                throw error;
-            }
+            });
         } else {
-            // The element is not an array.
-            if (!schema[subPath] && options.isRemoveOp)
-                throw new InvalidRemoveOpPath();
+            schemas = schemas.flatMap((schema)=>{
+                if (!schema[subPath] && options.isRemoveOp)
+                    throw new InvalidRemoveOpPath();
 
-            schema = schema[subPath] || (schema[subPath] = {});
+                return schema[subPath] || (schema[subPath] = {});
+            });
         }
     }
-    return schema;
+    return schemas;
 }
 
 /**
@@ -368,6 +387,11 @@ function addOrReplaceAttribute(property: any, patch: ScimPatchAddReplaceOperatio
             return patch.value;
         }
 
+        if(isReplaceOperation(patch.op)){
+            return property.map( 
+                (it) => addOrReplaceAttribute(it,patch,multiValuedPathFilter)
+            );
+        }
         const a = property;
         if (!deepIncludes(a, patch.value))
             a.push(patch.value);

test/scimPatch.test.ts

@@ -347,6 +347,74 @@ describe('SCIM PATCH', () => {
             return done();
         });
 
+        it('REPLACE: replace on multiValued objects', done => {
+            scimUser.emails = [
+                { value: "[email protected]", primary: true, newProperty: "pre"},
+                { value: "[email protected]", primary: false, newProperty: "pre" },
+                { value: "[email protected]", primary: false, newProperty: "pre" },
+                { value: "[email protected]", primary: false, newProperty: "pre" },
+            ];
+            const expected = "post";
+            const patch: ScimPatchAddReplaceOperation = {op: 'replace', value: expected, path: 'emails.newProperty'};
+            const afterPatch = scimPatch(scimUser, [patch]);
+            expect(afterPatch.emails[0].newProperty).to.be.eq(expected);
+            expect(afterPatch.emails[1].newProperty).to.be.eq(expected);       
+            expect(afterPatch.emails[2].newProperty).to.be.eq(expected);       
+            expect(afterPatch.emails[3].newProperty).to.be.eq(expected);         
+            return done();
+        });
+        it('REPLACE: replace on multiValued objects without complete path', done => {
+            scimUser.emails = [
+                { value: "[email protected]", primary: true, newProperty: "pre"},
+                { value: "[email protected]", primary: false, newProperty: "pre" },
+                { value: "[email protected]", primary: false, newProperty: "pre" },
+                { value: "[email protected]", primary: false, newProperty: "pre" },
+            ];
+            const expected = "post";
+            const patch: ScimPatchAddReplaceOperation = {op: 'replace', value: {newProperty: expected}, path: 'emails'};
+            const afterPatch = scimPatch(scimUser, [patch]);
+            expect(afterPatch.emails[0].newProperty).to.be.eq(expected);
+            expect(afterPatch.emails[1].newProperty).to.be.eq(expected);       
+            expect(afterPatch.emails[2].newProperty).to.be.eq(expected);       
+            expect(afterPatch.emails[3].newProperty).to.be.eq(expected);      
+            expect(afterPatch.emails.length).to.be.eq(4);
+            return done();
+        });
+
+        it('REPLACE: filter that match multiple elements of complex multiValued attribute', done => {
+            scimUser.emails = [
+                { value: "[email protected]", primary: true, newProperty: "pre"},
+                { value: "[email protected]", primary: false, newProperty: "pre" },
+                { value: "[email protected]", primary: false, newProperty: "pre" },
+                { value: "[email protected]", primary: false, newProperty: "pre" },
+            ];
+            const expected = "post";
+            const patch: ScimPatchAddReplaceOperation = {op: 'replace', value: expected, path: 'emails[primary eq false].newProperty'};
+            const afterPatch = scimPatch(scimUser, [patch]);
+            expect(afterPatch.emails[0].newProperty).to.be.eq("pre");
+            expect(afterPatch.emails[1].newProperty).to.be.eq(expected);       
+            expect(afterPatch.emails[2].newProperty).to.be.eq(expected);       
+            expect(afterPatch.emails[3].newProperty).to.be.eq(expected);         
+            return done();
+        });
+
+        it('REPLACE: filter that match multiple elements of complex multiValued attribute without complete path', done => {
+            scimUser.emails = [
+                { value: "[email protected]", primary: true, newProperty: "pre"},
+                { value: "[email protected]", primary: false, newProperty: "pre" },
+                { value: "[email protected]", primary: false, newProperty: "pre" },
+                { value: "[email protected]", primary: false, newProperty: "pre" },
+            ];
+            const expected = "post";
+            const patch: ScimPatchAddReplaceOperation = {op: 'replace', value: {newProperty: expected}, path: 'emails[primary eq false]'};
+            const afterPatch = scimPatch(scimUser, [patch]);
+            expect(afterPatch.emails[0].newProperty).to.be.eq("pre");
+            expect(afterPatch.emails[1].newProperty).to.be.eq(expected);       
+            expect(afterPatch.emails[2].newProperty).to.be.eq(expected);       
+            expect(afterPatch.emails[3].newProperty).to.be.eq(expected);  
+            return done();
+        });
+
         // see https://github.com/thomaspoignant/scim-patch/issues/489
         it('REPLACE: Replace op with value of empty object results in circular reference', done => {
             const expected = [
@@ -832,6 +900,41 @@ describe('SCIM PATCH', () => {
             expect(afterPatch.emails.length).to.be.eq(2);
             return done();
         });
+
+
+        it('ADD: filter that match multiple elements of complex multiValued attribute', done => {
+            scimUser.emails = [
+                { value: "[email protected]", primary: true },
+                { value: "[email protected]", primary: false },
+                { value: "[email protected]", primary: false },
+                { value: "[email protected]", primary: false },
+            ];
+            const expected = "post";
+            const patch: ScimPatchAddReplaceOperation = {op: 'add', value: expected, path: 'emails[primary eq false].newProperty'};
+            const afterPatch = scimPatch(scimUser, [patch]);
+            expect(afterPatch.emails[0].newProperty).to.be.an('undefined');
+            expect(afterPatch.emails[1].newProperty).to.be.eq(expected);       
+            expect(afterPatch.emails[2].newProperty).to.be.eq(expected);       
+            expect(afterPatch.emails[3].newProperty).to.be.eq(expected);         
+            return done();
+        });
+
+        it('ADD: filter that match multiple elements of complex multiValued attribute without complete path', done => {
+            scimUser.emails = [
+                { value: "[email protected]", primary: true },
+                { value: "[email protected]", primary: false },
+                { value: "[email protected]", primary: false },
+                { value: "[email protected]", primary: false },
+            ];
+            const expected = "post";
+            const patch: ScimPatchAddReplaceOperation = {op: 'add', value: {newProperty: expected}, path: 'emails[primary eq false]'};
+            const afterPatch = scimPatch(scimUser, [patch]);
+            expect(afterPatch.emails[0].newProperty).to.be.an('undefined');
+            expect(afterPatch.emails[1].newProperty).to.be.eq(expected);       
+            expect(afterPatch.emails[2].newProperty).to.be.eq(expected);       
+            expect(afterPatch.emails[3].newProperty).to.be.eq(expected);  
+            return done();
+        });
     });
     describe('remove', () => {
         it('REMOVE: with no path', done => {
@@ -1041,6 +1144,40 @@ describe('SCIM PATCH', () => {
             expect(scimUser).not.to.eq(afterPatch);
             return done();
         });
+
+
+
+        it('REMOVE: remove all sub-attributes in a complex multi-valued attribute', done => {
+            scimUser.emails = [
+                { value: "[email protected]", primary: true, newProperty: "pre" },
+                { value: "[email protected]", primary: false, newProperty: "pre" },
+                { value: "[email protected]", primary: false, newProperty: "pre" },
+                { value: "[email protected]", primary: false, newProperty: "pre" },
+            ];
+            const patch: ScimPatchRemoveOperation = {op: 'remove', path: 'emails.newProperty'};
+            const afterPatch = scimPatch(scimUser, [patch]);
+            expect(afterPatch.emails[0].newProperty).to.be.an('undefined');  
+            expect(afterPatch.emails[1].newProperty).to.be.an('undefined');       
+            expect(afterPatch.emails[2].newProperty).to.be.an('undefined');       
+            expect(afterPatch.emails[3].newProperty).to.be.an('undefined');         
+            return done();
+        });
+
+        it('REMOVE: filter that match multiple elements of complex multiValued attribute', done => {
+            scimUser.emails = [
+                { value: "[email protected]", primary: true, newProperty: "pre" },
+                { value: "[email protected]", primary: false, newProperty: "pre" },
+                { value: "[email protected]", primary: false, newProperty: "pre" },
+                { value: "[email protected]", primary: false, newProperty: "pre" },
+            ];
+            const patch: ScimPatchRemoveOperation = {op: 'remove', path: 'emails[primary eq false].newProperty'};
+            const afterPatch = scimPatch(scimUser, [patch]);
+            expect(afterPatch.emails[0].newProperty).to.be.eq("pre");
+            expect(afterPatch.emails[1].newProperty).to.be.an('undefined');       
+            expect(afterPatch.emails[2].newProperty).to.be.an('undefined');       
+            expect(afterPatch.emails[3].newProperty).to.be.an('undefined');         
+            return done();
+        });
     });
 
     describe('invalid requests', () => {