Don't do inverse work if inverse is explicitly turned off #4225
Conversation
| var inverse = record.type.inverseFor(relationshipMeta.key, store); | ||
| let inverseKey; | ||
| let inverse = null; | ||
| if (!relationshipMeta.options || |
There was a problem hiding this comment.
I think this would change the default behavior as it is now, right? If you don't provide an options hash we try to find the inverse.
There was a problem hiding this comment.
Right, which is why if !relationshipMeta.options, we take the original path of finding the inverse
There was a problem hiding this comment.
it is if there is no options it does try
There was a problem hiding this comment.
Gotcha. Is false also a documented option for inverse? I haven't seen it before.
There was a problem hiding this comment.
@asakusuma I think if (options && (options.inverse === false || options.inverse === null)) { return; } maybe more clear
There was a problem hiding this comment.
or moving it into a function since it will be inlined anyway
There was a problem hiding this comment.
@fivetanley good point, I'll remove false. Personally I think false is a better API, but I want to limit the scope of this PR to just the performance improvement.
@krisselden I've updated the PR to use a function as you suggested
Don't do inverse work if inverse is explicitly turned off
|
Can we regression test this please |
|
@stefanpenner what exactly do you mean here by regression test? |
|
@asakusuma add a test that verifies it doesn't do the work. |
|
On that same note we should add a test that the autodiscovery work is avoided when explicitly defined as well |
| let inverseKey; | ||
| let inverse = null; | ||
| if (shouldFindInverse(relationshipMeta)) { | ||
| inverse = record.type.inverseFor(relationshipMeta.key, store); |
There was a problem hiding this comment.
test this code path is take when it should be taken, and not if it should not.
|
@krisselden I was under the impression that regression testing is testing that you didn't break previous stuff. Regardless, I can add both of those tests. |
|
It is that, it is also used to describe testing a bug fix to ensure the fix isn't regressed. |
|
@asakusuma I've opened #4236 so we don't lose track adding tests for this. Let me know if you have questions regarding how to add the tests. I'm happy to help. Don't feel obliged, let me know if you don't have time for this. Thanks! |
|
@pangratz thanks for opening the ticket, I plan on getting to this tomorrow or Friday. |
If the model explicitly turns off inverse, no work should be done to resolve this inverse.