Skip to content

Skip null entities when bag is populated #2181

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

Merged
merged 4 commits into from
Jul 16, 2019

Conversation

bahusoid
Copy link
Member

@bahusoid bahusoid commented Jun 17, 2019

That's the way it's done in hibernate so I see no good reasons why we do it differently. And also null elements break filtering for many-to-many collections.

@hazzik
Copy link
Member

hazzik commented Jun 17, 2019

We were discussing the behaviour of "not-found" somewhere, but I'm not sure where. Do you remember?

@hazzik
Copy link
Member

hazzik commented Jun 17, 2019

Found it: #1995 (comment)

@bahusoid
Copy link
Member Author

Yeah. The main concern about this change made Frederic about not-found="ignore" mapping:

I agree it does not look very useful to have those null elements in the collection. But we have to be careful about the flush behavior. Not having them could only mean they would be deleted from the database. We have to check how it behaves currently. If currently, these not-found elements are left untouched by flushes (unless explicitly removed from the collection by the application), changing this could be a serious breaking change for some applications.

But such elements can't be deleted anyway - NHibernate doesn't keep identifiers for them. So null or missing element won't make a difference for flush. I will add test to verify it.

@bahusoid
Copy link
Member Author

bahusoid commented Jun 17, 2019

I've added tests.. Turns out for many-to-many collections all broken records are purged anyway on flush if collection is changed. Because collection is fully removed via one shot delete command (delete from Collection where OwnerId = :id) and then recreated. So I've run those tests on master (with adjustments to null presence) and this branch and have verified that flushing behavior is not changed with this PR.

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

Successfully merging this pull request may close these issues.

2 participants