Merge pull request #1 from xanthous-tech/fix-circular

Fix Circular Issue

Author: osmanmesutozcan

.eslintrc.js

@@ -12,6 +12,7 @@ module.exports = {
   rules: {
     'no-inner-declarations': 'off',
     'prefer-spread': 'off',
+    '@typescript-eslint/camelcase': 'warn',
     '@typescript-eslint/no-namespace': 'off',
     '@typescript-eslint/no-use-before-define': 'off',
     '@typescript-eslint/no-non-null-assertion': 'off',

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@xanthous/dgraph-orm",
-  "version": "0.2.0-alpha",
+  "version": "0.2.0-alpha3",
   "description": "dgraph ORM in TypeScript",
   "main": "dist/index.js",
   "author": "Simon Liang <simon@x-tech.io>",
@@ -20,7 +20,8 @@
     "lint": "eslint ./src --ext .ts",
     "lint:fix": "eslint --fix",
     "format": "prettier --write",
-    "test": "jest"
+    "test": "jest",
+    "test:debug": "node --inspect-brk ./node_modules/.bin/jest"
   },
   "repository": {
     "type": "git",

src/transaction/transaction-builder.ts

@@ -25,49 +25,83 @@ export namespace TransactionBuilder {
 
   class TreeMapperBuilder<T = any> {
     private readonly _entryType: Constructor<T>;
-    private _jsonData: IObjectLiteral<any>[];
+    private _root: IObjectLiteral<any>[];
     private _resource = new Map<string, IObjectLiteral<any>>();
 
     constructor(type: Constructor<T>) {
       this._entryType = type;
     }
 
-    addJsonData(data: IObjectLiteral<any> | IObjectLiteral<any>[]): TreeMapperBuilder<T> {
-      this._jsonData = Array.isArray(data) ? data : [data];
+    setRoot(data: IObjectLiteral<any> | IObjectLiteral<any>[]): TreeMapperBuilder<T> {
+      if (!Array.isArray(data) && Private.isNode(data)) {
+        this._root = [data];
+      } else {
+        if (Private.isAllNodes(data as IObjectLiteral<any>[])) {
+          this._root = data as IObjectLiteral<any>[];
+        }
+      }
+
+      this.addJsonData(data);
       return this;
     }
 
     /**
      * Walk the resource graph and add all nodes into resource cache by its `uid`.
      */
-    addResourceData(data: IObjectLiteral<any> | IObjectLiteral<any>[]): TreeMapperBuilder<T> {
+    addJsonData(data: IObjectLiteral<any> | IObjectLiteral<any>[]): TreeMapperBuilder<T> {
       if (data && !(data instanceof Array) && data.uid) {
-        this._resource.set(data.uid, data);
+        const { obj, predicates } = Private.separatePredicates(data);
+
+        if (this._resource.has(obj.uid)) {
+          const existingObj = this._resource.get(obj.uid) as IObjectLiteral<any>;
+          // trying to not modify existing object ref
+          Object.assign(existingObj, obj);
+        } else {
+          this._resource.set(obj.uid, obj);
+        }
+
+        for (const key of predicates.keys()) {
+          const predicate = predicates.get(key) as IObjectLiteral<any>[];
+          // store nodes from the predicate
+          this.addJsonData(predicate);
+          const existingObj = this._resource.get(obj.uid) as IObjectLiteral<any>;
+          const pred = existingObj[key] as IObjectLiteral<any>[];
+
+          predicate.forEach(p => {
+            const pObj = this._resource.get(p.uid) as IObjectLiteral<any>;
+
+            if (pred.indexOf(pObj) > -1) {
+              return;
+            }
+
+            pred.push(pObj);
+          });
+        }
+
         return this;
       }
 
       data.forEach((d: any) => {
-        this.addResourceData(d);
+        this.addJsonData(d);
       });
 
       return this;
     }
 
     build(): ITransaction<T> {
-      // Do not traverse the json tree if there is no
-      // resource data.
-      if (this._resource.size > 0) {
-        const visited = new Set<string>();
-        Array.isArray(this._jsonData)
-          ? this._jsonData.map(jd => Private.expand(visited, this._resource, jd))
-          : Private.expand(visited, this._resource, this._jsonData);
-      }
-
-      if (!Array.isArray(this._jsonData)) {
-        this._jsonData = [this._jsonData];
-      }
-
-      return new Transaction(this._entryType, this._jsonData);
+      // eslint-disable-next-line @typescript-eslint/no-var-requires
+      // const util = require('util');
+      // console.log(
+      //   util.inspect(
+      //     this._root.map(o => this._resource.get(o.uid)),
+      //     { colors: true, depth: 20 }
+      //   )
+      // );
+
+      return new Transaction(
+        this._entryType,
+        this._root.map(o => this._resource.get(o.uid))
+      );
     }
   }
 }
@@ -76,35 +110,31 @@ export namespace TransactionBuilder {
  * Module private statics
  */
 namespace Private {
-  /**
-   * Visit all nodes in a tree recursively, matching node uid in the resource data and adding extra information.
-   *
-   * ### NOTE
-   * Expand will modify the data in-place.
-   */
-  export function expand(visited: Set<string>, resource: IObjectLiteral<any>, source: IObjectLiteral<any>): void {
-    if (resource.has(source.uid)) {
-      Object.assign(source, resource.get(source.uid));
-    }
+  export function separatePredicates(
+    obj: IObjectLiteral<any>
+  ): { obj: IObjectLiteral<any>; predicates: Map<string, IObjectLiteral<any>[]> } {
+    const predicates: Map<string, IObjectLiteral<any>[]> = new Map();
 
-    Object.keys(source).forEach(key => {
-      if (key === 'dgraph.type') {
-        return;
-      }
+    Object.keys(obj).forEach((key: string) => {
+      if (isPredicate(obj, key)) {
+        predicates.set(key, obj[key]);
 
-      if (!Array.isArray(source[key])) {
-        return;
+        obj[key] = [];
       }
+    });
 
-      source[key].forEach((node: any) => {
-        const visitingKey = `${source.uid}:${key}:${node.uid}`;
-        if (visited.has(visitingKey)) {
-          return;
-        }
+    return { obj, predicates };
+  }
 
-        visited.add(visitingKey);
-        return expand(visited, resource, node);
-      });
-    });
+  export function isPredicate(obj: IObjectLiteral<any>, key: string): boolean {
+    return isAllNodes(obj[key]);
+  }
+
+  export function isAllNodes(obj: IObjectLiteral<any>[]): boolean {
+    return Array.isArray(obj) && (obj as Array<any>).every(isNode);
+  }
+
+  export function isNode(obj: IObjectLiteral<any>): boolean {
+    return !!obj.uid;
   }
 }

src/transaction/transaction.ts

@@ -133,38 +133,36 @@ export class Transaction<T extends Object, V> implements ITransaction<T> {
   /**
    * Given a data class definition and plain object return an instance of the data class.
    */
-  private plainToClassExecutor<T extends Object, V>(
-    cls: Constructor<T>,
-    plain: V[],
-    storage: WeakMap<Object, T[]>
-  ): T[] {
-    // Bail early if already converted.
-    if (storage.has(plain)) {
-      return storage.get(plain)!;
-    }
+  private plainToClassExecutor<T extends Object, V>(cls: Constructor<T>, plain: V[], storage: WeakMap<Object, T>): T[] {
+    return plain.reduce((acc: any[], pln: V) => {
+      // Bail early if already converted.
+      if (storage.has(pln)) {
+        acc.push(storage.get(pln)!);
+        return acc;
+      }
 
-    // Build the entry class
-    const instances: T[] = plainToClass(cls, plain, {
-      enableCircularCheck: true,
-      strategy: 'exposeAll'
-    });
+      // Build the entry class
+      const ins: T = plainToClass(cls, pln, {
+        enableCircularCheck: true,
+        strategy: 'exposeAll'
+      });
 
-    // Keep reference to the instance so in case of circular we can simply get it from storage and complete the circle.
-    storage.set(plain, instances);
+      // Keep reference to the instance so in case of circular we can simply get it from storage and complete the circle.
+      storage.set(pln, ins);
+      acc.push(ins);
 
-    instances.forEach((ins, idx) => {
       this.trackProperties(ins);
 
       const predicates = MetadataStorage.Instance.predicates.get(ins.constructor.name);
       if (!predicates) {
-        return;
+        return acc;
       }
 
       // FIXME: If the same uid is referenced in multiple places in the data, currently we will have 2 different instances
       //   of the same object. We need to make sure we share the instance.
       predicates.forEach(pred => {
         this.trackPredicate(ins, pred);
-        const _preds = (plain[idx] as any)[pred.args.name];
+        const _preds = (pln as any)[pred.args.name];
 
         // If no data available assign a new data.
         if (!_preds) {
@@ -175,9 +173,9 @@ export class Transaction<T extends Object, V> implements ITransaction<T> {
           (ins as any)[pred.args.propertyName] = this.plainToClassExecutor(pred.args.type(), _preds, storage);
         }
       });
-    });
 
-    return instances;
+      return acc;
+    }, []) as T[];
   }
 
   /**

tests/delete.spec.ts

@@ -36,6 +36,7 @@ describe('Delete handling', function() {
 
     const transaction = TransactionBuilder.of(Person)
       .addJsonData(data)
+      .setRoot({ uid: '0x1' })
       .build();
 
     const jane = transaction.tree[0].friends.get()[0];

tests/mutation.spec.ts

@@ -39,7 +39,7 @@ describe('Mutation handling', () => {
     ];
 
     const transaction = TransactionBuilder.of(Person)
-      .addJsonData(data)
+      .setRoot(data)
       .build();
 
     transaction.tree[0].hobbies.get()[0].name = 'New Hobby Name';
@@ -137,7 +137,7 @@ describe('Mutation handling', () => {
     ];
 
     const transaction = TransactionBuilder.of(Person)
-      .addJsonData(data)
+      .setRoot(data)
       .build();
 
     transaction.tree[0].name = 'New John';

tests/serde.spec.ts

@@ -20,7 +20,7 @@ describe('Serialize deserialize', () => {
     ];
 
     const txn = TransactionBuilder.of(Work)
-      .addJsonData(data)
+      .setRoot(data)
       .build();
 
     const work = txn.tree[0];
@@ -79,7 +79,7 @@ describe('Serialize deserialize', () => {
     ];
 
     const txn = TransactionBuilder.of(Work)
-      .addJsonData(data)
+      .setRoot(data)
       .build();
 
     console.log(Utils.toObject(txn.tree[0]));
@@ -123,7 +123,7 @@ describe('Serialize deserialize', () => {
     ];
 
     const txn = TransactionBuilder.of(Person)
-      .addJsonData(data)
+      .setRoot(data)
       .build();
 
     expect(txn.tree[0].name).toEqual(data[0]['Person.name']);
@@ -170,7 +170,7 @@ describe('Serialize deserialize', () => {
     ];
 
     const txn = TransactionBuilder.of(Person)
-      .addJsonData(data)
+      .setRoot(data)
       .build();
 
     expect(txn.tree[0].name).toEqual(data[0]['Person.name']);
@@ -191,24 +191,20 @@ describe('Serialize deserialize', () => {
     const data = [
       {
         uid: '0x1',
-        'Parent.name': 'Parent',
         'Parent.has_child': [
           {
             uid: '0x2',
             'Parent.has_child': [
               {
                 uid: '0x3',
-                'Parent.has_child': null
+                'Parent.has_child': [{ uid: '0x1' }]
               }
             ]
           }
         ] as any[]
       }
     ];
 
-    // Circularly reference
-    data[0]['Parent.has_child'][0]['Parent.has_child'][0]['Parent.has_child'] = data as any;
-
     const resource = [
       {
         uid: '0x1',
@@ -225,27 +221,34 @@ describe('Serialize deserialize', () => {
     ];
 
     const txn = TransactionBuilder.of(Parent)
-      .addJsonData(data)
-      .addResourceData(resource)
+      .setRoot(data)
+      .addJsonData(resource)
       .build();
 
     Utils.printObject(txn.tree[0]);
+    Utils.printObject(
+      txn.tree[0]
+        .has_child.get()[0]
+        .has_child.get()[0]
+        .has_child.get()[0]
+    );
 
     expect(txn.tree[0].has_child.get()[0].name).toEqual('Node 0x2');
 
     expect(
-      txn.tree[0].has_child
-        .get()[0]
+      txn.tree[0]
+        .has_child.get()[0]
+        .has_child.get()[0]
         .has_child.get()[0]
-        .has_child.get()[0].name
+        .name
     ).toEqual('Node 0x1');
 
     expect(
-      txn.tree[0] ===
-        txn.tree[0].has_child
-          .get()[0]
+      txn.tree[0].name ===
+        txn.tree[0]
           .has_child.get()[0]
           .has_child.get()[0]
+          .has_child.get()[0].name
     ).toBeTruthy();
   });
 });