Objection.js: $relatedQuery shouldn't mutate loaded relations

[drew-r]

I find that $relatedQuery.findById and $relatedQuery.insert will mutate the relations loaded on a model instance.
I find that this isn't particularly useful & tends to create more problems than it avoids.

Consider e.g

• Loading a model with a filtered relation. Any insert or find operation on the same relation using $relatedQuery breaks the filter.
• A 'shortcut' relation that is defined only for syntactic sugar (e.g often this is the inverse of a more common access pattern - e.g find genre related to this artist, where you usually access genre->artist & don't really want genre nested under artist when serialized etc.) - querying with $relatedQuery you now have an unwanted piece of data non-obviously nested in your model

My proposal would be to disable this behaviour as I don't think a complete solution exists here.

[koskimas]

Why would we disable findById but not for example where('foo', 'bar')?

[drew-r]

Is there some mechanism at play that copies back results from $relatedQuery onto the relation on the model instance? If so I would propose to disable that entirely.
But yes it would follow that .where etc. would also not modify relations.

[koskimas]

Every time anything is fetched through a $relatedQuery the corresponding relation is assigned the result value. I agree that this should be disabled entirely, but I don't know how many people rely on this feature. It's one of those things that seem really scary to remove, but then again it may be that everyone has just learned to live with that and agree that it should be removed.

I really don't know what to do with this.

[jordansexton]

It is annoying when you want to serialize some model graph to return in JSON that if you do any related queries on a model beforehand, those will get included in the output all the way down. However, it probably will break existing applications, including mine in a few places. 😅

That said, it's not super hard to fix them with $loadRelated where needed. The current behavior is not surprising, but it is quite tedious to get around if you want to avoid it.

[drew-r]

Is there some way to reduce impact of such a change?
i.e leave default behaviour in place, implement strict $relatedQuery strategy enabled similar to the way defaultEagerAlgorithm etc are set...

[koskimas]

We could add a static config flag to Model that would be false by default. Then just mention something like this in the changelog If things blow up, set Model.theNewConfig = true 😄. I would really like to remove that stuff from the codebase too though.

[drew-r]

I was thinking to do the opposite by leaving it disabled by default for a couple versions with a 'deprecated in future' warning logged when the existing $relatedQuery algo is used.
The message would be something more like Get your code working with Model.theNewConfig = true soon because pretty soon it will be the default. :)

[koskimas]

The only problem with that is that the next version is 1.0 and we'd be stuck with that option for a long time.

[koskimas]

Actually we are stuck with the code and the option either way. The only way to get rid of this by 1.0 is to just remove it in 1.0 and force everyone to make the update. Not the most pleasant update.

[vladshcherbin]

@koskimas versions before 1.0 are expected to have breaking changes in api so it's possible to tag the next version as 0.10, introduce the change there with a warning and prepare users for changed behavior in 1.0.

[koskimas]

@VladShcherbin Yes I'm aware of that, but I'd really like to move to 1.0 as soon as possible.

[koskimas]

The problem with @drew-r's suggestion is that everyone who uses $relatedQuery would get their logs filled with deprecation warnings even if they never used the relation property.

Actually the breaking change from just removing the whole behaviour is not that bad. You just need to search for all your $relatedQuery queries and do this:

const children = await person.$relatedQuery('children');
person.children = children;

or

const insertedChild = await person.$relatedQuery('children').insert(child);
person.children.push(insertedChild);

if you really want the current behaviour. If $relatedQuery is used solely to fetch a relation, it can be replaced with eager or $loadRelated.

I think I'll just remove this feature in the next version without deprecating it first.

[jordansexton]

What if $relatedQuery(...).insert() assigned the property, but just await-ing to do a SELECT didn't? I think it's unintuitive and very duplicative to have to assign a prop/push an array when writing, but for reads, $relatedQuery is duplicating $loadRelated.

Also, what person.children has or hasn't been fetched? What if multiple children are inserted?

const insertedChild = await person.$relatedQuery('children').insert(child)
if (person.children) {
   person.children.push(insertedChild);
}
else {
   person.children = [insertedChild];
}


const insertedChildren = await person.$relatedQuery('children').insert(child1, child2, child3)
if (person.children) {
   person.children.push(...insertedChildren);
}
else {
   person.children = insertedChildren;
}

Aside from being pretty ugly, without checking for the children property's existence, the code implies that all of person's associated children are being set here. I understand the effect of the current behavior (create and push an array) is no different, but the code doesn't imply that it's the only associated record in the array. It gets worse when you are inserting multiple children at once (and then you have to know to push/concat multiple elements). I imagine my own code would get littered with constructs like the above.

I think the issue with $relatedQuery is that it's not intuitive for reads, given that eager and $loadRelated exist. However, removing its behavior for writes means an extremely common problem, knowledge of the relationship type, and checking for existing arrays, with different approaches to setting the related property (property assignment, array assignment, array append) has to be spread throughout userland code.

[jordansexton]

(Edited the comment above with a more complete example.)

[koskimas]

@drew-r @jordansexton We have two people on this thread and already two different opinions. I think everyone will have their own about this one.

I think it's even more inconsistent if $relatedQuery('foo').insert adds the result, but no other operation does. Now the reasoning is add the values whenever we can. I think the only consistent change is to remove the mutation altogether.

@jordansexton What if we add helpers like this:

const insertedChild = await person.$relatedQuery('children').insert(child)
person.$setRelated('children', insertedChild);

const insertedChildren = await person.$relatedQuery('children').insert([child1, child2, child3])
person.$appendRelated('children', insertedChild);

Those helpers would do the right thing based on relation type and whether the relations are empty or not.

[jordansexton]

I agree with you. What about an option (defaulting to falsy) to $relatedQuery that performs the current behavior (and could use $setRelated/$appendRelated under the hood)? This allows 1.0 to break compatibility, but also makes the upgrade quite easy/obvious.

[koskimas]

@jordansexton @drew-r What if I add two booleans:

Model.relatedFindQueryMutates = true;
Model.relatedInsertQueryMutates = true;

and leave them to true by default now?

[jordansexton]

Yeah, no opposition here. While I think an option arg/context prop to locally override when calling $relatedQuery might be useful, it also seems like this global option covers most cases easily.

[koskimas]

@jordansexton There is no mechanism for passing options to relatedQuery and I don't want to add a third argument for it. A separate options method would be no less code than using the new setRelated or appendRelated methods.

[jordansexton]

I figured as much, and that makes sense.

[drew-r]

@koskimas I like it. Thanks!!