Adding virtuals to Model creates circular reference errors when including them in toJSON

[Ventis]

Hi,

I updated my version of Keystone to 0.2.26 today and immediately started changing all my models from addPattern('standard meta') to {track: true}.
For all but one of my models this worked, even with existing data. One of them however is a bit of a special case. It has a virtual property that has to be returned when you get it through an API call.

I've had circular reference errors with this before so I have overwritten the toJSON method and it worked:

Waypoint.schema.set('toJSON', {
    virtuals: true,
    transform: function(doc, ret, options) {
        delete ret.list;
        delete ret.type;
        delete ret._;
        delete ret.typeOptionsMap;
        delete ret.typeOptions;
        delete ret.typeLabel;
        delete ret.typeData;
        delete ret.ugcTypeOptionsMap;
        delete ret.ugcTypeOptions;
        delete ret.ugcTypeLabel;
        delete ret.ugcTypeData;
        return ret;
    }
});

However with this new track option, it adds some new 'hidden' properties that create circular references, namely 'createdByRefList' and 'updatedByRefList'.

When I add those to the transform function it works fine again. My question (or suggestion) would be: is this the preferred way of getting virtual properties in a serialized Model instance? Or is there a better way to include them in the output of a query?

[creynders]

AFAICT this happens when your schema includes Relationship fields. This will explode keystone:

Foo.schema.set('toJSON', {
    virtuals: true
});

If the schema contains any Relationship fields.

[ignlg]

Because that function doesn't clean child objects. I'm using a custom transform too, but more complex to allow Relationships: https://gist.github.com/ignlg/519a4b6b9845d7442a48

[creynders]

@ignlg Cool! Can't it be included into keystone?

[adamscybot]

+1, ran into this today.

[ignlg]

@creynders Sorry for my late reply, I hadn't noticed your question. I'm not sure about the posible side effects in each use case. This code worked for us like a charm and it's, indeed, part of every model since then. But I don't even know where to add it to KeystoneJS safely.

[JedWatson]

This will explode keystone

💥

I was worried this would be a problem. Keystone relies on its virtuals quite a bit, I'll see if I can do something to kill the circular references. Maybe just exposing those properties as methods instead would help. Failing that, maybe adding in an option to default the toJSON function to exclude circular references (and other extraneous things) would be good.

@ignlg's function is cool but maybe it would be better to explicitly have a cleanup method on both the list and each field that would safely remove default virtuals / properties / add in properties to the default toJSON implementation... since each thing that's added is known at some point, we could be intelligent about it.

[JedWatson]

By "expose those properties as methods instead" I mean change from item.relationshiField.refList to item._.relationshipField.getRefList()

There must be a similar solution for createdByRefList and updatedByRefList but I haven't figured it out yet.

This would probably require a big version bump to push through, we could certainly queue it or maybe hide the new behaviour behind an option until 0.4 (which won't be nearly as long a wait as 0.3 was)

[creynders]

Closing due to inactivity, feel free to comment again if this issue persists for you!

[django-wong]

This issue is still happening in v4