kwhitley / treeize

Converts row data (in JSON/associative array format) to tree structure based on column naming conventions.
MIT License
189 stars 24 forks source link

Circular structure when there is no 'root' element #3

Closed AntJanus closed 9 years ago

AntJanus commented 10 years ago

Here's the test case. Let's say that I am querying for a single user but want all of the user information under the "user": {} node

Expectation:

[ {
  "user": {
     "name": "John",
     "id": 3,
   }
} ] 

Reality:

So I set up my aliases for using Knex and pulling in data like so:

['name as user:name', 'id as user:id']

console logged result:

[{
    "name": "John",
    "id": 3,
    "user": [Circular]
}]

Sending it over the wire via Node (ie. res.send()) causes the following error:

Possibly unhandled TypeError: Converting circular structure to JSON

Only way I've seen to resolve the issue is to have at least one root-level element.

kwhitley commented 10 years ago

Didnt see this earlier.... will look into it! Thanks for the examples :+1:

EvanK commented 10 years ago

I don't think it's that a root element must exist, so much as an element must exist at every level of a path. For example, this throws the same error because there's no element in the "bar" level of the path:

treeize.grow([
  {
    'root': true,
    'foo:bar:name': 'Bill_Brasky',
    'foo:bar:id': 1
  },
]);

Adding a falsey element at the "bar" level in fact ends up overwriting that element with the elements further down that path:

treeize.grow([
  {
    'root': true,
    'foo:bar:name': 'Bill_Brasky',
    'foo:bar:id': 1,
    'foo:bar': false,
  },
]);
/* gives us:
[
  {
    "root": true,
    "foo": {
      "bar": {
        "id": 1,
        "name": "Bill_Brasky"
      }
    }
  }
]
*/

Adding a truthy element, on the other hand, overwrites the elements further down the path for some reason:

treeize.grow([
  {
    'root': true,
    'foo:bar:name': 'Bill_Brasky',
    'foo:bar:id': 1,
    'foo:bar': true,
  },
]);
/* gives us:
[
  {
    "root": true,
    "foo": {
      "bar": true
    }
  }
]
*/
kwhitley commented 10 years ago

Makes sense @EvanK, as it would (if I recall) check for a node's existence before attempting attribute injection. You're running into the side effects of using a truthy definition (of which true and {} both share, as do false with undefined).

kwhitley commented 10 years ago

That does give me a good idea where to start though, thanks! Surprised I didn't address this in the original implementation. A deep branch (without leaves along the way) is not at all uncommon...

AntJanus commented 10 years ago

Interesting, so the temp workaround would be to create a node with the same name as the collection (or subnode) but mark it false?

Awesome.

Thanks @EvanK @kwhitley!

kwhitley commented 10 years ago

This is going to be challenging to address... currently the system works by creating a shallow "blueprint" of a new collection item... this would be a list of attributes with values, not attributes as objects (as shown in the first example here).

By adding even a single key:value attribute to each item in a collection (e.g. id = 1...x), you solve this issue and allow subsequent items to be stitched in to either the existing collection item or a new one as needed). Without it, you would have to do some form of deep object comparison each pass to determine which item to match to... which would also only work if each node was redundantly defined (which is required in the original treeize, but not the upcoming version that allows for non-uniformity between signatures).

@AntJanus @EvanK Thoughts?

kwhitley commented 9 years ago

Addressed