Repository: scimmyjs/scimmy
Pull request #78 (2025-03-05 04:02:15)

Add predefined schema extension attribute accessors to `SCIMMY.Types.Schema` instances
Currently, the `SCIMMY.Types.Schema` constructor defines accessors for extension schemas, using the extension schema ID as the property name. The set accessor for these properties writes values directly to the underlying resource object, and then prevents extension of this object. Namespaced accessors are also defined for each attribute of the extension schema, which forward accessor operations to the extension schema property. When namespaced extension attribute accessors are used to add properties to the underlying resource object's extension schema value, they attempt to do so to a non-extensible target object. This raises an unintended extensibility exception, and effectively prevents modifying the extension schema value in any way.

This change updates the behaviour of the extension schema accessors, making the get accessor return a new interceding object with all extension attributes predefined as accessors in the same way direct schema attributes are predefined. The extension schema set accessor has also been updated to add or remove the extension schema ID from the `schemas` property, depending on whether the extension has any actual values. Test fixtures for individual `SCIMMY.Schemas.*` classes have been updated to verify extension schema attribute accessors behave as described. A new test fixture has been added to the `SCIMMY.Messages.PatchOp` class suite to verify it correctly handles namespaced attributes, and missing branch coverage has been added to the `SCIMMY.Types.Schema` class suite.


Patch:
diff --git a/src/lib/types/definition.js b/src/lib/types/definition.js
index fe53e4b..c61bd9f 100644
--- a/src/lib/types/definition.js
+++ b/src/lib/types/definition.js
@@ -96,9 +96,9 @@ export class SchemaDefinition {
             const extension = (this.attributes.find(a => a instanceof SchemaDefinition && name.toLowerCase().startsWith(a.id.toLowerCase()))
                 ?? (name.toLowerCase().startsWith(`${this.id.toLowerCase()}:`) || name.toLowerCase() === this.id.toLowerCase() ? this : false));
             // Get the actual attribute name minus extension ID
-            const attribute = (extension ? name.substring(extension.id.length+1) : "");
+            const attribute = (extension ? name.substring(extension.id.length + 1) : "");
             
-            // Bail out if no schema extension found with matching ID 
+            // Bail out if no schema extension found with matching ID
             if (!extension)
                 throw new TypeError(`Schema definition '${this.id}' does not declare schema extension for namespaced target '${name}'`);
             
@@ -245,7 +245,7 @@ export class SchemaDefinition {
         const schemas = [...new Set([
             this.id,
             ...(this.attributes.filter(a => a instanceof SchemaDefinition).map(s => s.id)
-                .filter(id => !!data[id] || Object.keys(data).some(d => d.startsWith(`${id}:`)))),
+                .filter(id => (data[id] !== undefined || Object.entries(data).some(([k, v]) => v !== undefined && k.startsWith(`${id}:`))))),
             ...(Array.isArray(data.schemas) ? data.schemas : [])
         ])];
         
@@ -324,7 +324,7 @@ export class SchemaDefinition {
                         // Start by only dealing with expressions that contain this extension...
                         .map((filter) => Object.entries(filter).filter(([k]) => k.startsWith(`${name}:`))).filter((filter) => filter.length)
                         // ...then remove the extension prefix
-                        .map((filter) => filter.reduce((res, [key, val]) => Object.assign(res, {[key.replace(`${name}:`, "")]: val}), {}))
+                        .map((filter) => filter.reduce((res, [key, val]) => Object.assign(res, {[key.replace(`${name}:`, "")]: val}), {}));
                     
                     try {
                         // Coerce the mixed value, using only namespaced attributes for this extension
diff --git a/src/lib/types/schema.js b/src/lib/types/schema.js
index 194889d..8465d70 100644
--- a/src/lib/types/schema.js
+++ b/src/lib/types/schema.js
@@ -24,6 +24,49 @@ const defineToJSONProperty = (target, definition, resource) => Object.defineProp
  */
 const hasActualValues = (target) => (Object.values(target).some((v) => typeof v === "object" ? hasActualValues(v) : v !== undefined));
 
+/**
+ * Create property descriptor definitions for an attribute
+ * @param {SCIMMY.Types.Attribute} attribute - the attribute to create the property descriptors for
+ * @param {Object} resource - the object storing underlying data
+ * @param {Object} receiver - the object where the accessors are being defined
+ * @param {String} direction - whether the resource is inbound from a request or outbound for a response
+ * @returns {Object} the property descriptors for the attribute, to be passed to Object.defineProperties
+ * @private
+ */
+const describeAttributeAccessors = (attribute, resource, receiver, direction) => ({
+    // Because why bother with case-sensitivity in a JSON-based standard?
+    // See: RFC7643§2.1 (https://datatracker.ietf.org/doc/html/rfc7643#section-2.1)
+    [attribute.name.toLowerCase()]: {
+        get: () => (receiver[attribute.name]),
+        set: (value) => (receiver[attribute.name] = value)
+    },
+    // Now set the handles for the actual name
+    // Overrides above if attribute.name is already all lower case
+    [attribute.name]: {
+        enumerable: true,
+        // Get and set the value from the internally scoped object
+        get: () => (resource[attribute.name]),
+        set: (value) => {
+            const {name, config: {mutable}} = attribute;
+            
+            // Check for mutability of attribute before setting the value
+            if (mutable !== true && receiver[name] !== undefined && receiver[name] !== value)
+                throw new SCIMError(400, "mutability", `Attribute '${name}' already defined and is not mutable`);
+            
+            try {
+                // Validate the supplied value through attribute coercion
+                resource[name] = attribute.coerce(value, direction);
+            } catch (ex) {
+                // Rethrow attribute coercion exceptions as SCIM errors
+                throw new SCIMError(400, "invalidValue", ex.message);
+            }
+            
+            // Indicate resource value has been set
+            return true;
+        }
+    }
+});
+
 /**
  * Automatically assigned attributes not required in schema extension values
  * @enum {"id"|"schemas"|"meta"} SCIMMY.Types.Schema~ShadowAttributes
@@ -107,7 +150,7 @@ export class Schema {
      * @property {Date} [meta.lastModified] - when this resource was last updated at the service provider
      * @property {String} meta.location - full, canonical URI of the resource being returned
      * @property {String} [meta.version] - version of the resource being returned, if any
-    */
+     */
     constructor(data = {}, direction) {
         const {schemas = []} = data;
         // Create internally scoped storage object
@@ -137,36 +180,7 @@ export class Schema {
         defineToJSONProperty(this, definition, resource);
         
         // Predefine getters and setters for all possible attributes
-        for (let attribute of attributes) Object.defineProperties(this, {
-            // Because why bother with case-sensitivity in a JSON-based standard?
-            // See: RFC7643§2.1 (https://datatracker.ietf.org/doc/html/rfc7643#section-2.1)
-            [attribute.name.toLowerCase()]: {
-                get: () => (this[attribute.name]),
-                set: (value) => (this[attribute.name] = value)
-            },
-            // Now set the handles for the actual name
-            // Overrides above if attribute.name is already all lower case
-            [attribute.name]: {
-                enumerable: true,
-                // Get and set the value from the internally scoped object
-                get: () => (resource[attribute.name]),
-                set: (value) => {
-                    const {name, config: {mutable}} = attribute;
-                    
-                    // Check for mutability of attribute before setting the value
-                    if (mutable !== true && this[name] !== undefined && this[name] !== value)
-                        throw new SCIMError(400, "mutability", `Attribute '${name}' already defined and is not mutable`);
-                    
-                    try {
-                        // Validate the supplied value through attribute coercion
-                        return (resource[name] = attribute.coerce(value, direction));
-                    } catch (ex) {
-                        // Rethrow attribute coercion exceptions as SCIM errors
-                        throw new SCIMError(400, "invalidValue", ex.message);
-                    }
-                }
-            }
-        });
+        for (let attribute of attributes) Object.defineProperties(this, describeAttributeAccessors(attribute, resource, this, direction));
         
         // Predefine getters and setters for all schema extensions
         for (let extension of extensions) Object.defineProperties(this, {
@@ -182,26 +196,52 @@ export class Schema {
                 get: () => {
                     // Go through and delete any undefined properties or complex attributes without actual values
                     for (let [key, value] of Object.entries(resource[extension.id] ?? {})) {
-                        if (value === undefined || (Object(value) === value && !hasActualValues(value))) {
+                        if (value === undefined || (Object(value) === value && !hasActualValues(value)))
                             delete resource[extension.id][key];
-                        }
                     }
                     
                     // If no attributes with values remaining, return undefined
-                    return !hasActualValues(resource[extension.id] ?? {}) ? undefined : resource[extension.id];
+                    if (!hasActualValues(resource[extension.id] ?? {})) return undefined;
+                    // Otherwise, return a pre-structured receiver for the extension
+                    else {
+                        // Set up the receiver with JSON stringifier attached
+                        const receiver = defineToJSONProperty({}, extension, resource[extension.id]);
+                        
+                        // Predefine getters and setters for all possible extension attributes
+                        for (let attribute of extension.attributes)
+                            Object.defineProperties(receiver, describeAttributeAccessors(attribute, resource[extension.id], receiver, direction));
+                        
+                        // Prevent additional attributes being added to the extension
+                        return Object.preventExtensions(receiver);
+                    }
                 },
                 set: (value) => {
                     try {
-                        // Validate the supplied value through schema extension coercion
-                        resource[extension.id] = extension.coerce(value, direction);
-                        
-                        // Return the value with JSON stringifier attached, marked as 
-                        defineToJSONProperty(resource[extension.id], extension, resource[extension.id]);
-                        return Object.assign(Object.preventExtensions(resource[extension.id]), value);
+                        // If no actual values, remove extension schema ID from schemas and unset value
+                        if (!hasActualValues(value)) {
+                            resource.schemas = resource.schemas.filter(id => id !== extension.id);
+                            delete resource[extension.id];
+                        }
+                        // Otherwise, add extension schema ID and set the value
+                        else {
+                            // Validate the supplied value through schema extension coercion
+                            resource[extension.id] = extension.coerce(value, direction);
+                            
+                            // If the extension now has a value when it didn't before, add schema ID to schemas
+                            if (!!resource[extension.id] && !resource.schemas.includes(extension.id))
+                                resource.schemas.push(extension.id);
+                            
+                            // Attach JSON stringifier and set the value back to the instance
+                            defineToJSONProperty(resource[extension.id], extension, resource[extension.id]);
+                            Object.assign(Object.preventExtensions(this[extension.id] ?? {}), value);
+                        }
                     } catch (ex) {
                         // Rethrow attribute coercion exceptions as SCIM errors
                         throw new SCIMError(400, "invalidValue", ex.message);
                     }
+                    
+                    // Indicate resource value has been set
+                    return true;
                 }
             },
             // Predefine namespaced getters and setters for schema extension attributes
@@ -238,11 +278,13 @@ export class Schema {
                                 target = target[path] = {...(target?.[path] ?? {})};
                             }
                             
-                            // Set the actual value
+                            // Set the actual value...
                             target[attribute.name] = value;
+                            // ...then assign it back to the extension for coercion
+                            this[extension.id] = Object.assign(this[extension.id] ?? {}, data);
                             
-                            // Then assign it back to the extension for coercion
-                            return (this[extension.id] = Object.assign(this[extension.id] ?? {}, data));
+                            // Indicate resource value has been set
+                            return true;
                         }
                     },
                     // Go through the process again for subAttributes


Test patch:
diff --git a/test/hooks/schemas.js b/test/hooks/schemas.js
index f407b3b..d8b811b 100644
--- a/test/hooks/schemas.js
+++ b/test/hooks/schemas.js
@@ -1,4 +1,4 @@
-import assert from "assert";
+import assert, {AssertionError} from "assert";
 import {Attribute} from "#@/lib/types/attribute.js";
 import {SchemaDefinition} from "#@/lib/types/definition.js";
 import {Schema} from "#@/lib/types/schema.js";
@@ -89,29 +89,49 @@ export default class ResourcesHooks {
             
             try {
                 instance[key.toLowerCase()] = value.toUpperCase();
-                assert.strictEqual(instance[key], value.toUpperCase(),
-                    "Schema instance did not include lower-case attribute aliases");
             } catch (ex) {
                 if (ex.scimType !== "mutability") throw ex;
             }
+            
+            assert.strictEqual(instance[key.toLowerCase()], instance[key],
+                "Schema instance did not include lower-case attribute aliases");
         });
         
         it("should include extension schema attribute property accessor aliases", async () => {
+            const attributes = [new Attribute("string", "testValue"), new Attribute("complex", "complexValue", {}, [new Attribute("string", "subValue")])];
+            const extension = new SchemaDefinition("Extension", "urn:ietf:params:scim:schemas:Extension", "", attributes);
+            const {constructor = {}} = await fixtures;
+            
             try {
-                // Add an extension with one attribute
-                TargetSchema.extend(new SchemaDefinition("Extension", "urn:ietf:params:scim:schemas:Extension", "", [new Attribute("string", "testValue")]));
+                // Add the extension, including complex attributes
+                TargetSchema.extend(extension);
                 
                 // Construct an instance to test against
-                const {constructor = {}} = await fixtures;
-                const target = "urn:ietf:params:scim:schemas:Extension:testValue";
                 const instance = new TargetSchema(constructor);
                 
-                instance[target] = "a string";
-                assert.strictEqual(instance[target], "a string",
+                // Make sure instance includes namespaced extension attribute accessors...
+                instance[`${extension.id}:testValue`] = "a string";
+                assert.strictEqual(instance[`${extension.id}:testValue`], "a string",
                     "Schema instance did not include schema extension attribute aliases");
-                instance[target.toLowerCase()] = "another string";
-                assert.strictEqual(instance[target], "another string",
+                // ...as well as lower-case aliases
+                instance[`${extension.id}:testValue`.toLowerCase()] = "another string";
+                assert.strictEqual(instance[`${extension.id}:testValue`.toLowerCase()], "another string",
                     "Schema instance did not include lower-case schema extension attribute aliases");
+                // Make sure instance includes accessors for nested complex attributes in the extension...
+                instance[`${extension.id}:complexValue.subValue`] = "a string";
+                assert.strictEqual(instance[`${extension.id}:complexValue.subValue`], "a string",
+                    "Schema instance did not include schema extension complex attribute aliases");
+                // ...as well as lower-case aliases for these too
+                instance[`${extension.id}:complexValue.subValue`.toLowerCase()] = "another string";
+                assert.strictEqual(instance[`${extension.id}:complexValue.subValue`.toLowerCase()], "another string",
+                    "Schema instance did not include lower-case schema extension complex attribute aliases");
+                // Make sure instance includes a lower-case alias for the extension ID accessors
+                instance[extension.id.toLowerCase()] = {testValue: "a string"};
+                assert.strictEqual(instance[extension.id.toLowerCase()].testValue, "a string",
+                    "Schema instance did not include schema extension alias");
+            } catch (ex) {
+                if (ex instanceof AssertionError) throw ex;
+                assert.fail(`Schema instance did not include extension schema attribute property accessor aliases\r\n[cause]: ${ex}`);
             } finally {
                 // Remove the extension so it doesn't interfere later
                 TargetSchema.truncate("urn:ietf:params:scim:schemas:Extension");
@@ -171,6 +191,7 @@ export default class ResourcesHooks {
         it("should clean up empty extension schema properties", async () => {
             // Get attributes for the extension ready
             const attributes = [
+                new Attribute("string", "deletedValue"),
                 new Attribute("complex", "testValue", {}, [
                     new Attribute("string", "stringValue"),
                     new Attribute("complex", "value", {}, [
@@ -184,6 +205,7 @@ export default class ResourcesHooks {
             const extension = new SchemaDefinition("Extension", "urn:ietf:params:scim:schemas:Extension", "", attributes);
             const source = {
                 ...constructor,
+                [`${extension.id}:deletedValue`]: "a string",
                 [`${extension.id}:testValue.stringValue`]: "a string",
                 [`${extension.id}:testValue.value.value`]: "a string"
             };
@@ -195,10 +217,14 @@ export default class ResourcesHooks {
                 // Construct an instance to test against
                 const instance = new TargetSchema(source);
                 
-                // Unset the extension value and check for cleanup
-                instance[`${extension.id}:testValue.value.value`] = undefined;
+                // Unset the extension values and check for cleanup
+                instance[`${extension.id}:deletedValue`] = undefined;
+                assert.strictEqual(instance[extension.id].deletedValue, undefined,
+                    "Schema instance did not clean up empty extension schema attribute properties");
                 instance[`${extension.id}:testValue.stringValue`] = undefined;
-                
+                assert.strictEqual(instance[extension.id].testValue.stringValue, undefined,
+                    "Schema instance did not clean up empty extension schema sub-attribute properties");
+                instance[`${extension.id}:testValue.value.value`] = undefined;
                 assert.strictEqual(instance[extension.id], undefined,
                     "Schema instance did not clean up empty extension schema properties");
             } finally {
diff --git a/test/lib/messages/patchop.js b/test/lib/messages/patchop.js
index f692f6a..2f14385 100644
--- a/test/lib/messages/patchop.js
+++ b/test/lib/messages/patchop.js
@@ -3,6 +3,7 @@ import path from "path";
 import url from "url";
 import assert from "assert";
 import {Attribute} from "#@/lib/types/attribute.js";
+import {SchemaDefinition} from "#@/lib/types/definition.js";
 import {PatchOp} from "#@/lib/messages/patchop.js";
 import {createSchemaClass} from "../../hooks/schemas.js";
 
@@ -236,6 +237,26 @@ describe("SCIMMY.Messages.PatchOp", () => {
                 }
             });
             
+            it(`should support namespaced attributes in '${op}' operations`, async () => {
+                const attributes = [new Attribute("string", "organization"), new Attribute("string", "test")];
+                const extension = new SchemaDefinition("Extension", TestSchema.definition.id.replace("Test", "Extension"), "", attributes);
+                const source = {id: "1234", userName: "asdf", [`${extension.id}:test`]: "value", ...(op === "add" ? {} : {[`${extension.id}:organization`]: "COMPANY1"})};
+                const target = {id: "1234", userName: "asdf", [extension.id]: {test: "value", ...(op === "remove" ? {} : {organization: "COMPANY2"})}};
+                const Operations = [{op, path: `${extension.id}:organization`, ...(op === "remove" ? {} : {value: "COMPANY2"})}];
+                
+                try {
+                    TestSchema.definition.extend(extension);
+                    
+                    const expected = new TestSchema(target, "out");
+                    const actual = await new PatchOp({...template, Operations}).apply(new TestSchema(source));
+                    
+                    assert.deepStrictEqual(actual, expected,
+                        `PatchOp 'apply' did not support namespaced attributes in '${op}' operations`);
+                } finally {
+                    TestSchema.definition.truncate(extension);
+                }
+            });
+            
             if (["add", "replace"].includes(op)) {
                 it(`should expect 'value' to be an object when 'path' is not specified in '${op}' operations`, async () => {
                     const Operations = [{op, value: false}];
diff --git a/test/lib/types/schema.js b/test/lib/types/schema.js
index 1562165..0620de2 100644
--- a/test/lib/types/schema.js
+++ b/test/lib/types/schema.js
@@ -42,6 +42,15 @@ describe("SCIMMY.Types.Schema", () => {
             assert.ok(typeof Schema.extend === "function",
                 "Static method 'extend' was not implemented");
         });
+        
+        it("should expect 'extension' argument to be a Schema class, SchemaDefinition instance, or collection of Attribute instances", () => {
+            assert.throws(() => Schema.extend({}), 
+                {name: "TypeError", message: "Expected 'extension' to be a Schema class, SchemaDefinition instance, or collection of Attribute instances"},
+                "Static method 'extend' did not throw with invalid object input");
+            assert.throws(() => Schema.extend([new Attribute("string", "test"), {}]),
+                {name: "TypeError", message: "Expected 'extension' to be a Schema class, SchemaDefinition instance, or collection of Attribute instances"},
+                "Static method 'extend' did not throw with invalid object input");
+        });
     });
     
     describe(".truncate()", () => {
@@ -49,6 +58,20 @@ describe("SCIMMY.Types.Schema", () => {
             assert.ok(typeof Schema.truncate === "function",
                 "Static method 'truncate' was not implemented");
         });
+        
+        it("should resolve schema definition instances from Schema classes", () => {
+            const attributes = [new Attribute("string", "aValue"), new Attribute("string", "aString", {returned: false})];
+            const Test = createSchemaClass({attributes});
+            const Extension = createSchemaClass({name: "Extension", id: Test.definition.id.replace("Test", "Extension")});
+            
+            Test.extend(Extension);
+            
+            try {
+                Test.truncate(Extension);
+            } catch (ex) {
+                assert.fail(`Static method 'truncate' did not resolve schema definition instance from Schema class\r\n[cause]: ${ex}`);
+            }
+        });
     });
     
     describe("@constructor", () => {
@@ -64,5 +87,20 @@ describe("SCIMMY.Types.Schema", () => {
             assert.deepStrictEqual(JSON.parse(JSON.stringify(actual)), expected,
                 "Schema instance did not include 'toJSON' method that strips attributes where returned is marked as 'never'");
         });
+        
+        it("should include 'toJSON' method on schema extension values", () => {
+            const Test = createSchemaClass();
+            const attributes = [new Attribute("string", "aValue"), new Attribute("string", "aString", {returned: false})];
+            const Extension = createSchemaClass({name: "Extension", id: Test.definition.id.replace("Test", "Extension"), attributes});
+            const source = {aValue: "a value"};
+            const expected = {schemas: [Test.definition.id, Extension.definition.id], meta: {resourceType: Test.definition.name}, [Extension.definition.id]: source};
+            
+            Test.extend(Extension);
+            
+            const actual = new Test({[Extension.definition.id]: {...source, aString: "a string"}});
+            
+            assert.deepStrictEqual(JSON.parse(JSON.stringify(actual)), expected,
+                "Schema instance did not include 'toJSON' method on schema extension values");
+        });
     });
 });
\ No newline at end of file