X Tutup
The Wayback Machine - https://web.archive.org/web/20220811221242/https://github.com/dotnet/efcore/pull/28663
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

Allow value converters to change nullability of columns #28663

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AndriySvyryd
Copy link
Member

@AndriySvyryd AndriySvyryd commented Aug 11, 2022

Fixes #24685

@AndriySvyryd AndriySvyryd requested a review from ajcvickers Aug 11, 2022
@ajcvickers
Copy link
Member

@ajcvickers ajcvickers commented Aug 11, 2022

@AndriySvyryd Despite what the issue title says, the only thing I was planning to do here was to allow a non-nullable CLR property to be configured as nullable in the database. Value converters that mess with nulls are problematic anyway, and just because a converter can convert nulls doesn't mean that it never results in a null. So the idea was that if you're sure (by whatever means necessary) that a nullable database column will not result in a null value in the CLR property, then we allow you to configure that. But we don't ever do that automatically.

That being said, if we want to do something more complex than that actually coupled to value converters, and we're confident it is correct, then I'm not necessarily against it.

@AndriySvyryd
Copy link
Member Author

@AndriySvyryd AndriySvyryd commented Aug 11, 2022

@ajcvickers Oh. That would be a part of #27970 then.

The approach in this PR works, but without #9329 there would be cases where setting a value converter wouldn't trigger conventions that depend on nullability. But the former solution somewhat suffers from this as well without #19806

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.

2 participants
X Tutup