Auto-clean inferred 'id' field for when cleaning data; improve test coverage

src/Item.js

@@ -20,9 +20,9 @@
 * data.a = 2
 * console.log(`${data.a} - ${foo.a}`) // prints: '2 - 1'
 * ```
-* 
-* Attempts to set a value for an existing property will fail. Attempts to create 
-* 
+*
+* Attempts to set a value for an existing property will fail. Attempts to create
+*
 * ## Implementation notes
 *
 * - If we just wrap and return the 'this' in the Item constructor, it will not find any sub-class functions. It would

src/Resources.js

@@ -5,6 +5,8 @@ import { getSourceFile } from '@liquid-labs/federated-json'
 import { ListManager } from './ListManager'
 import { Item } from './Item'
 
+const passthruNormalizer = (id) => id
+
 /**
 * Common class for base resources support simple get and list functions.
 */
@@ -38,20 +40,40 @@ const Resources = class {
     if (readFromFile === true) {
       items = JSON.parse(fs.readFileSync(fileName))
     }
-    // add standard 'id' field if not present.
+
+    // set manuall set itemConfig
+    this.#itemConfigCache = itemConfig
+
     items = items || []
     // normalize and guarantee uniqueness of items (based on ID)
     const seen = {}
+    const hasExplicitId = this.#itemConfig.keyField === 'id'
     items.forEach((item) => {
+      // add standard 'id' field if not present.
+      if (hasExplicitId === true && !('id' in item)) {
+        throw new Error("Key field 'id' not found on at least one item.")
+      }
+      if (hasExplicitId === false && 'id' in item) {
+        throw new Error(`Inferred/reserved 'id' found on at least one item (key field is: ${this.#itemConfig.keyField}).`)
+      }
       item.id = item.id || this.idNormalizer(item[this.keyField])
       if (seen[item.id] === true) {
         throw new Error(`Found items with duplicate key field '${this.keyField}' values ('${item.id}') in the ${this.resourceName} list.`)
       }
       seen[item.id] = true
     })
 
-    // set manuall set itemConfig
-    this.#itemConfigCache = itemConfig
+    if (hasExplicitId === false) {
+      const origDataCleaner = this.#itemConfig.dataCleaner
+      const newCleaner = origDataCleaner === undefined
+        ? (data) => { delete data.id; return data }
+        : (data) => {
+          delete data.id
+          return origDataCleaner
+        }
+      this.#itemConfigCache = Object.assign({}, this.#itemConfig, { dataCleaner : newCleaner })
+      Object.freeze(this.#itemConfigCache)
+    }
 
     // setup ListManager
     this.listManager = new ListManager({
@@ -71,6 +93,7 @@ const Resources = class {
 
   // TODO: switch implementatiosn to set itemConfig directly, then we can do away with the 'Cache' convention and this constructor test.
   get #itemConfig() {
+    // return Object.assign({}, this.#itemConfigCache || this.constructor.itemConfig)
     return this.#itemConfigCache || this.constructor.itemConfig
   }
 
@@ -82,7 +105,7 @@ const Resources = class {
   /**
   * See [Item.idNormalizer](./Item.md#idnormalizer)
   */
-  get idNormalizer() { return this.#itemConfig.idNormalizer }
+  get idNormalizer() { return this.#itemConfig.idNormalizer || passthruNormalizer }
 
   get itemClass() { return this.#itemConfig.itemClass }
 

src/test/Item.test.js

@@ -18,15 +18,15 @@ const data = {
 
 const SetFoo = class extends Item {
   constructor(data) {
-    super(data, { allowSet : [ 'foo' ]})
+    super(data, { allowSet : ['foo'] })
   }
 }
 
 Item.bindCreationConfig({
-  itemClass: SetFoo,
-  itemName : 'foo',
-  keyField: 'id',
-  resourceName: 'foos'
+  itemClass    : SetFoo,
+  itemName     : 'foo',
+  keyField     : 'id',
+  resourceName : 'foos'
 })
 
 const SubItem = class extends Item {
@@ -100,17 +100,17 @@ describe('Item', () => {
 
     describe('set operations', () => {
       test('are not permitted on unknown properties', () => {
-        const subItem = new SubItem({ integer: 1, blah: 10 })
+        const subItem = new SubItem({ integer : 1, blah : 10 })
         expect(() => { subItem.foo = 12 }).toThrow()
       })
 
       test('by default are not permitted on known properties', () => {
-        const subItem = new SubItem({ integer: 1, blah: 10 })
+        const subItem = new SubItem({ integer : 1, blah : 10 })
         expect(() => { subItem.blah = 12 }).toThrow()
       })
 
       test('succeed when explicitly allowed', () => {
-        const foo = new SetFoo({ id: 1, foo: 12 })
+        const foo = new SetFoo({ id : 1, foo : 12 })
         expect(foo.foo).toBe(12)
         foo.foo = 10
         expect(foo.foo).toBe(10)

src/test/Resources.test.js

@@ -1,11 +1,13 @@
-/* global describe expect test */
+/* global beforeEach describe expect test */
+import fsPath from 'node:path'
 
 import { Item } from '../Item'
 import { Resources } from '../Resources'
 
 const Foo = class extends Item { }
 
 const fooConfig = {
+  dataCleaner  : (data) => { delete data.deleteMe; return data },
   itemClass    : Foo,
   itemName     : 'foo',
   keyField     : 'name',
@@ -15,10 +17,46 @@ const fooConfig = {
 Item.bindCreationConfig(fooConfig)
 
 describe('Resources', () => {
-  const foos = new Resources({ itemConfig : Foo.itemConfig })
+  describe('constructor', () => {
+    test("raises an exception when initialized with 'readFromFile' and 'items' are true/present", () =>
+      expect(() => new Resources({ fileName : 'foo.json', items : [{}], readFromFile : true })).toThrow())
+
+    test("raises an exception when initialized with 'readFromFile' and no 'fileName", () =>
+      expect(() => new Resources({ readFromFile : true })).toThrow())
+
+    test('will load items from a file', () => {
+      const dataPath = fsPath.join(__dirname, 'data', 'items.json')
+      const resources =
+        new Resources({ fileName : dataPath, readFromFile : true, itemConfig : fooConfig })
+      expect(resources.list({ rawData : true })).toHaveLength(1)
+    })
+
+    test('raises an exception when non-unique items are found', () =>
+      expect(() => new Resources({ itemConfig : { keyField : 'id' }, items : [{ id : 1 }, { id : 1 }] })).toThrow())
+
+    test("raises an exception when 'id' is present but not the key field", () =>
+      expect(() => new Resources({ itemConfig : Foo.itemConfig, items : [{ name : 'Bobby', id : 1 }] })).toThrow())
+
+    test("raises an exception whin 'id' is the key field, but not defined", () =>
+      expect(() => new Resources({ itemConfig : { keyField : 'id' }, items : [{ name : 'Bobby' }] })).toThrow())
+
+    test("raises an exception when non-'id' key field is not defined", () =>
+      expect(() => new Resources({ itemConfig : fooConfig, items : [{ id : 1 }] })).toThrow())
+  })
+
   describe('get(id, { required: true })', () => {
-    test('Raises an exception when no matching resourc found.', () => {
-      expect(() => foos.get('a foo', { required : true })).toThrow(/Did not find required foo 'a foo'./)
+    let foos
+    beforeEach(() => {
+      foos = new Resources({ itemConfig : Foo.itemConfig, items : [{ name : 'bar', deleteMe : true }] })
     })
+
+    test('get({clean: true, rawData: true}) returns cleaned data', () =>
+      expect('deleteMe' in foos.get('bar', { clean : true, rawData : true })).toBe(false))
+
+    test("get({clean: true, rawData: true}) cleans normalized 'id' field when not part of the data", () =>
+      expect('id' in foos.get('bar', { clean : true, rawData : true })).toBe(false))
+
+    test('raises an exception when no matching resource found.', () =>
+      expect(() => foos.get('a foo', { required : true })).toThrow(/Did not find required foo 'a foo'./))
   })
 })

src/test/data/items.json

@@ -0,0 +1,5 @@
+[
+  {
+    "name": "Bobby"
+  }
+]