-
Notifications
You must be signed in to change notification settings - Fork 165
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
Je/fix upgrade #3854
Je/fix upgrade #3854
Conversation
src/realm/table.cpp
Outdated
l.add(from_list.get_legacy(j)); | ||
auto val = from_list.get_legacy(j); | ||
if (val.is_null() && !nullable) { | ||
// Be forgiving about this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should we be forgiving about this? Isn't it better to just throw a better exception that describe what is wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error was also reported in Java here: realm/realm-java#6995 (comment)
It does feel slightly worrying if a list not allowed to contain Nulls do contain Nulls though? Any idea how this could happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to actually upgrade the file. If we just throw, there is nothing the user can do to fix the situation. I have not checked the core-5 code - yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I'm wondering how it is possible to come in this situation where the column is marked as not-null, but still contains null values? Doesn't that indicate another bug somewhere?
But agree that if there is nothing the user can do, then it is better to attempt to fix it instead of just leaving a broken Realm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked at it once more. Found another solution that actually returns the right value from list. Not sure if it is nicer.
fef45d0
to
cfba1d8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to add tests for this? Otherwise LGTM
Fixes #3836