Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require value for @Parent when eager loaded #380

Merged
merged 3 commits into from
Aug 27, 2020
Merged

Conversation

tanner0101
Copy link
Member

@tanner0101 tanner0101 commented Aug 27, 2020

Queries that fail to eager-load a value for a @Parent relation will now throw an error (#380).

This change helps prevent unintended access of uninitialized @Parent properties in cases where no parent is found. For example, if the parent is deleted (including soft-deletion) or the reference was set to an invalid identifier.

@tanner0101 tanner0101 added bug Something isn't working semver-patch Internal changes only labels Aug 27, 2020
@tanner0101
Copy link
Member Author

tanner0101 commented Aug 27, 2020

One potential caveat with this change is that users could have been relying on this type of query not resulting in an error. It is possible to check if the parent was eager loaded using model.$parent.value instead of accessing directly. However, a state where the value in an @Parent field does not point to an existing model is technically invalid. Especially given this can cause uninitialized access assertions, it seems that Fluent should prevent this where possible. As a future direction, it may make sense to allow for eager loading to fail silently via a flag to recreate the old behavior. This would allow (careful) use of QueryBuilder even if the data is in an invalid state.

@tanner0101 tanner0101 merged commit ef2a9a7 into master Aug 27, 2020
@tanner0101 tanner0101 deleted the tn-require-parent branch August 27, 2020 20:00
@tanner0101
Copy link
Member Author

These changes are now available in 1.7.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working semver-patch Internal changes only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants