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

[BUGFIX beta] Inverse null relationships should throw if model doesn't exist 2 #4675

Closed
wants to merge 1 commit into from

Conversation

canufeel
Copy link

this is continuation of #4558 which is closed in favour of this one.

2.6.0 Introduced a bug such that for { inverse: null } relationships the relationship target is not checked at model creation time. However without { inverse: null } we would get an error when trying to create a model with relationship to another model that doesn't exist. This PR tries to bring in consistent behaviour for such cases - perform a check for model type during model creation and throw an error if the model is not present in the store.

… at least in debug to avoid introducing potential bugs and display appropriate error messages. for performance those checks in prod should be disabled
@bmac
Copy link
Member

bmac commented Nov 21, 2016

This looks good to me. @runspired I would appreciate your eyes on this pr.

@runspired
Copy link
Contributor

@bmac will review this evening 👍

@runspired
Copy link
Contributor

I forgot to come back and say that I reviewed and it looked good. @bmac

@runspired
Copy link
Contributor

Specifically, it does not affect or hinder any efforts at laziness, and is properly scoped to debug code paths. Any additional work to ensure laziness of relationship setup need not hold this up.

@bmac
Copy link
Member

bmac commented Dec 5, 2016

@canufeel do you have time to rebase this pr?

@canufeel
Copy link
Author

canufeel commented Dec 6, 2016

@bmac sure! Would spend some time on it in the next couple days.

@canufeel
Copy link
Author

@bmac did a rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants