Repository: amzn/ion-js
Pull request #679 (2021-02-10 01:33:42)

Add support for duplicate fieldnames in dom.Value#Struct
*Description of changes:*
This PR is to support duplicate fields in a Struct according to the [Ion Spec](https://amzn.github.io/ion-docs/docs/spec.html#struct).

*Tasks:*

1.  Added changes in the `dom.Value#Struct` for supporting duplicate fields. (All the `Struct` field will now be stored in `(fieldname, values)` -> `[string, Value[]]` form)
2.  `dom.Value#Struct` constructor would accept both `Iterable<string, Value>` as well as `Iterable<string, Value[]>` for creating `Struct`.
2.  Added `getAll` and `allFields` methods for getting all values of Struct fields.
3.  `toString` method returns with all the values of Struct fields.
2.  Changes in test suite for duplicate fields.
3.  add tests for duplicate fields. 


desaikd: *Completed Tasks:*

1. added changes for `getAll()` and `allFields()` methods.
2. changed `ionEquals()` method for checking unordered duplicate fields.
2. Tests suite changes for all the above tasks.

*Questions:*
Could you explain with an example on how the `fields` return value would look like? Also for the iterator?
e.g.
```
{foo: 1, baz: true, foo: 2} 
```
For above Struct value what should be the return value for `fields()` as well as iterator ? 
Currently it will return something like this:
```
[['foo',[1,2]] , ['baz' , true]]
```
desaikd: *Changes:*
```
{foo: 1, baz: true, foo: 2} 
```
`fields()` would return Struct fields as below with only the last value:
```
[['foo', 2] , ['baz' , true]]
```

- To get all the field values `allFields()` can be used.
- `toString` method returns with all the values of Struct fields.

Patch:
diff --git a/src/dom/Struct.ts b/src/dom/Struct.ts
index 7eecfe8..3250b72 100644
--- a/src/dom/Struct.ts
+++ b/src/dom/Struct.ts
@@ -41,10 +41,15 @@ export class Struct extends Value(
    * @param fields        An iterator of field name/value pairs to represent as a struct.
    * @param annotations   An optional array of strings to associate with this null value.
    */
-  constructor(fields: Iterable<[string, Value]>, annotations: string[] = []) {
+  constructor(
+    fields: Iterable<[string, Value]> | Iterable<[string, Value[]]>,
+    annotations: string[] = []
+  ) {
     super();
+    // Struct fields will always be stored as (fieldName, fieldValues) => (string, Value[])
     for (const [fieldName, fieldValue] of fields) {
-      this._fields[fieldName] = fieldValue;
+      this._fields[fieldName] =
+        fieldValue instanceof Value ? [fieldValue] : fieldValue;
     }
     this._setAnnotations(annotations);
 
@@ -53,7 +58,7 @@ export class Struct extends Value(
       // All values set by the user are stored in `this._fields` to avoid
       // potentially overwriting Struct methods.
       set: function (target, name, value): boolean {
-        target._fields[name] = value;
+        target._fields[name] = [value];
         return true; // Indicates that the assignment succeeded
       },
       get: function (target, name): any {
@@ -62,7 +67,12 @@ export class Struct extends Value(
         if (name in target) {
           return target[name];
         }
-        return target._fields[name];
+        let length =
+          target._fields[name] !== undefined ? target._fields[name].length : -1;
+        if (length === -1) {
+          return target._fields[name];
+        }
+        return target._fields[name][length - 1];
       },
       deleteProperty: function (target, name): boolean {
         // Property is deleted only if it's in _field Collection
@@ -83,24 +93,52 @@ export class Struct extends Value(
     if (typeof pathHead !== "string") {
       throw new Error(`Cannot index into a struct with a ${typeof pathHead}.`);
     }
-    const child: Value | undefined = this._fields[pathHead];
+    const child: Value[] | undefined = this._fields[pathHead];
+    if (child === undefined) {
+      return null;
+    }
+    if (pathTail.length === 0) {
+      return child[child.length - 1]!;
+    }
+    return child[child.length - 1]!.get(...pathTail);
+  }
+
+  getAll(...pathElements: PathElement[]): Value[] | null {
+    if (pathElements.length === 0) {
+      throw new Error("Value#get requires at least one parameter.");
+    }
+    const [pathHead, ...pathTail] = pathElements;
+    if (typeof pathHead !== "string") {
+      throw new Error(`Cannot index into a struct with a ${typeof pathHead}.`);
+    }
+    const child: Value[] | undefined = this._fields[pathHead];
     if (child === undefined) {
       return null;
     }
     if (pathTail.length === 0) {
-      return child;
+      return child!;
     }
-    return child.get(...pathTail);
+    let values: Value[] | undefined = [];
+    child.forEach((value) => values!.push(...value.getAll(...pathTail)!));
+    return values!;
   }
 
   fieldNames(): string[] {
     return Object.keys(this._fields);
   }
 
-  fields(): [string, Value][] {
+  allFields(): [string, Value[]][] {
     return Object.entries(this._fields);
   }
 
+  fields(): [string, Value][] {
+    let singleValueFields = Object.create(null);
+    for (const [fieldName, values] of this.allFields()) {
+      singleValueFields[fieldName] = values[values.length - 1];
+    }
+    return Object.entries(singleValueFields);
+  }
+
   elements(): Value[] {
     return Object.values(this._fields);
   }
@@ -112,7 +150,7 @@ export class Struct extends Value(
   toString(): string {
     return (
       "{" +
-      [...this.fields()]
+      [...this.allFields()]
         .map(([name, value]) => name + ": " + value)
         .join(", ") +
       "}"
@@ -122,9 +160,11 @@ export class Struct extends Value(
   writeTo(writer: Writer): void {
     writer.setAnnotations(this.getAnnotations());
     writer.stepIn(IonTypes.STRUCT);
-    for (const [fieldName, value] of this) {
-      writer.writeFieldName(fieldName);
-      value.writeTo(writer);
+    for (const [fieldName, values] of this.allFields()) {
+      for (let value of values) {
+        writer.writeFieldName(fieldName);
+        value.writeTo(writer);
+      }
     }
     writer.stepOut();
   }
@@ -138,16 +178,20 @@ export class Struct extends Value(
   }
 
   toJSON() {
-    return this._fields;
+    let normalizedFields = Object.create(null);
+    for (const [key, value] of this.fields()) {
+      normalizedFields[key] = value;
+    }
+    return normalizedFields;
   }
 
   static _fromJsValue(jsValue: any, annotations: string[]): Value {
     if (!(jsValue instanceof Object)) {
       throw new Error(`Cannot create a dom.Struct from: ${jsValue.toString()}`);
     }
-    const fields: [string, Value][] = Object.entries(
+    const fields: [string, Value[]][] = Object.entries(
       jsValue
-    ).map(([key, value]) => [key, Value.from(value)]);
+    ).map(([key, value]) => [key, [Value.from(value)]]);
     return new this(fields, annotations);
   }
 
@@ -171,13 +215,13 @@ export class Struct extends Value(
       // `compareOnlyIon` requires that the provided value be an ion.dom.Struct instance.
       if (other instanceof Struct) {
         isSupportedType = true;
-        valueToCompare = other.fields();
+        valueToCompare = other.allFields();
       }
     } else {
       // We will consider other Struct-ish types
       if (typeof other === "object" || other instanceof global.Object) {
         isSupportedType = true;
-        valueToCompare = Object.entries(other);
+        valueToCompare = Value.from(other).allFields();
       }
     }
 
@@ -185,20 +229,24 @@ export class Struct extends Value(
       return false;
     }
 
-    if (this.fields().length !== valueToCompare.length) {
+    if (this.allFields().length !== valueToCompare.length) {
       return false;
     }
     let matchFound: boolean = true;
     const paired: boolean[] = new Array<boolean>(valueToCompare.length);
-    for (let i: number = 0; matchFound && i < this.fields().length; i++) {
+    for (let i: number = 0; matchFound && i < this.allFields().length; i++) {
       matchFound = false;
       for (let j: number = 0; !matchFound && j < valueToCompare.length; j++) {
         if (!paired[j]) {
-          const child = this.fields()[i];
+          const child = this.allFields()[i];
           const expectedChild = valueToCompare[j];
           matchFound =
             child[0] === expectedChild[0] &&
-            child[1].equals(expectedChild[1], options);
+            this._ionValueEquals(
+              child[1].sort(),
+              expectedChild[1].sort(),
+              options
+            );
           if (matchFound) {
             paired[j] = true;
           }
@@ -214,4 +262,18 @@ export class Struct extends Value(
     }
     return matchFound;
   }
+
+  // helper function for comparison of all values of a field
+  _ionValueEquals(child: Value[], expectedChild: Value[], options): boolean {
+    if (child.length !== expectedChild.length) {
+      return false;
+    }
+
+    for (let i: number = 0; i < child.length; i++) {
+      if (!child[i].equals(expectedChild[i], options)) {
+        return false;
+      }
+    }
+    return true;
+  }
 }
diff --git a/src/dom/Value.ts b/src/dom/Value.ts
index 7443f8a..b876047 100644
--- a/src/dom/Value.ts
+++ b/src/dom/Value.ts
@@ -106,6 +106,26 @@ export interface Value {
    */
   get(...pathElements: PathElement[]): Value | null;
 
+  /**
+   * Like the `get(PathElement[])` method, but returns *all* of the values associated
+   * with a given field name instead of just the last one. For example, for the
+   * following struct:
+   *
+   *     {
+   *         foo: 1,
+   *         foo: null,
+   *         foo: true,
+   *     }
+   *
+   * a call to `get("foo")` would return a `dom.Boolean` representing `true`
+   * while a call to `getAll("foo")` would return an `Array` of
+   * `dom.Value`s: `[1, null, true]`.
+   *
+   * @param One or more values to be used to index into the Value.
+   * @returns null if no value is found at the specified path. Otherwise, returns the discovered Value.
+   */
+  getAll(...pathElements: PathElement[]): Value[] | null;
+
   /**
    * For the Struct type, returns an array containing the names of the fields in the Struct;
    * otherwise throws an Error.
@@ -118,6 +138,24 @@ export interface Value {
    */
   fields(): [string, Value][];
 
+  /**
+   * Like the `fields()` method, but returns an array containing the field name/value pairs with *all* of
+   * the associated values to a field name, instead of field name/value pairs with just the last associated value
+   * to a field name. For example, for the
+   * following struct:
+   *
+   *     {
+   *         foo: 1,
+   *         foo: null,
+   *         foo: true,
+   *     }
+   *
+   * a call to `fields()` would return a field name "foo" with a `dom.Boolean` representing `true`
+   * while a call to `allFields()` would return  a field name "foo" with an `Array` of
+   * `dom.Value`s: `[1, null, true]`.
+   */
+  allFields(): [string, Value[]][];
+
   /**
    * For the Struct, List, and SExpression types, returns an array containing the container's
    * nested values; otherwise throws an Error.
@@ -320,6 +358,10 @@ export function Value<Clazz extends Constructor>(
       this._unsupportedOperation("fields");
     }
 
+    allFields(): [string, Value[]][] {
+      this._unsupportedOperation("allFields");
+    }
+
     elements(): Value[] {
       this._unsupportedOperation("elements");
     }
@@ -328,6 +370,10 @@ export function Value<Clazz extends Constructor>(
       this._unsupportedOperation("get");
     }
 
+    getAll(...pathElements: PathElement[]): Value[] | null {
+      this._unsupportedOperation("getAll");
+    }
+
     as<T extends Value>(ionValueType: Constructor<T>): T {
       if (this instanceof ionValueType) {
         return (this as unknown) as T;
diff --git a/src/dom/index.ts b/src/dom/index.ts
index afb2ef9..6dd3ba3 100644
--- a/src/dom/index.ts
+++ b/src/dom/index.ts
@@ -126,11 +126,15 @@ function _loadValue(reader: Reader): Value {
 }
 
 function _loadStruct(reader: Reader): Struct {
-  const children: Map<string, Value> = new Map();
+  const children: Map<string, Value[]> = new Map();
   const annotations: string[] = reader.annotations();
   reader.stepIn();
   while (reader.next()) {
-    children.set(reader.fieldName()!, _loadValue(reader));
+    if (children.has(reader.fieldName()!)) {
+      children.get(reader.fieldName()!)!.push(_loadValue(reader));
+    } else {
+      children.set(reader.fieldName()!, [_loadValue(reader)]);
+    }
   }
   reader.stepOut();
   return new Struct(children.entries(), annotations);


Test patch:
diff --git a/test/dom/dom.ts b/test/dom/dom.ts
index 71d54e6..dbbdfd6 100644
--- a/test/dom/dom.ts
+++ b/test/dom/dom.ts
@@ -455,10 +455,10 @@ describe('DOM', () => {
       assert.equal("Jacob", s.get("name", "middle")!.stringValue());
 
       // Iteration
-      for (let [fieldName, value] of s.fields()) {
+      for (let [fieldName, values] of s.allFields()) {
          assert.isTrue(typeof fieldName === "string");
          assert.isTrue(fieldName.length > 0);
-         assert.isFalse(value.isNull());
+         values.forEach(value => {assert.isFalse(value.isNull())});
       }
 
       assert.equal(2, s.fields().length);
@@ -492,4 +492,53 @@ describe('DOM', () => {
 
       assert.equal(2, s.fields().length)
    });
+
+   it('load() Struct with duplicate fields as any', () => {
+      let s: any = load(
+          'foo::bar::{' +
+          'age: 55, ' +
+          'age: 41' +
+          '}'
+      )!;
+
+      let s1: Value = load(
+          'foo::bar::{' +
+          'name: {' +
+          'first: "John", ' +
+          'middle: "Jacob", ' +
+          'last: "Jingleheimer-Schmidt",' +
+          '},' +
+          'name: {' +
+          'first: "Jessie", ' +
+          'middle: "Bob", ' +
+          'last: "Pinkman",' +
+          '},' +
+          'age: 41' +
+          '}'
+      )!;
+      assert.equal(IonTypes.STRUCT, s.getType());
+      assert.deepEqual(['foo', 'bar'], s.getAnnotations());
+
+      // Field access return last value for a fieldname
+      assert.equal(41, s.age);
+
+      // get all values for given field name
+      assert.equal(55, s.getAll("age")[0]);
+      assert.equal(41, s.getAll("age")[1]);
+      assert.equal(2, s.getAll("age").length);
+      assert.isTrue(s1.getAll('name','middle')![0].equals("Jacob", {onlyCompareIon: false}));
+      assert.isTrue(s1.getAll('name','middle')![1].equals("Bob", {onlyCompareIon: false}));
+      assert.equal(2, s1.getAll('name','middle')!.length);
+
+
+      // Iteration
+      for (let [fieldName, value] of s) {
+         assert.isTrue(typeof fieldName === "string");
+         assert.isTrue(fieldName.length > 0);
+         assert.isFalse(value.isNull());
+      }
+
+      // length is 1 with two values: 55, 41
+      assert.equal(1, s.fields().length)
+   });
 });
\ No newline at end of file
diff --git a/test/dom/equivalence.ts b/test/dom/equivalence.ts
index a738593..1474262 100644
--- a/test/dom/equivalence.ts
+++ b/test/dom/equivalence.ts
@@ -347,4 +347,18 @@ describe("Equivalence", () => {
       )
     );
   });
+  it("equals() for Struct with duplicate fields", () => {
+    let value1: Value = load("{ foo: 'bar', foo: 'baz' }")!;
+    let value2: Value = load("{ foo: 'bar', foo: 'baz' }")!;
+    let value3: Value = load("{ foo: 'bar', foo: 'qux' }")!;
+    let value4: Value = load("{ foo: 1, baz: true, foo: 2 }")!;
+    let value5: Value = load("{ foo: 2, foo: 1, baz: true }")!;
+
+    assert.isTrue(value1.equals(value2));
+    assert.isTrue(value2.equals(value1));
+    assert.isFalse(value1.equals(value3));
+
+    // Equivalence for unordered fields
+    assert.isTrue(value4.equals(value5));
+  });
 });