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

[6.0] Mark internal the ValueConverter constructors that allow conversion of nulls #26232

Merged
merged 1 commit into from
Oct 9, 2021

Conversation

ajcvickers
Copy link
Member

@ajcvickers ajcvickers commented Oct 1, 2021

Resolves #26230.

Description

In 6.0 we implemented Allow HasConversion/ValueConverters to convert nulls. However, this has proved to be very problematic in practice with many pitfalls. For example:

These are not trivial issues to fix and for the query issues they are not easy to detect and will result in unexpected/bad data.

This makes the feature somewhat a pit-of-failure. However, the issue has 43 votes on GitHub, putting it in the top 25 and meaning that people are likely already using it. Therefore, we are proposing to mark these APIs as internal for 6.0. This still allows people to use them, but behind an analyzer warning and documentation. (This is similar to marking them as experimental--see EntityFrameworkExperimentalAttribute) We can then make them fully public in a future release after gathering more feedback and possibly making improvements.

Customer impact

Customers will get a warning when trying to use this feature. If they proceed (suppressing or ignoring the warning), it will still work as it does now, but they will hopefully be drawn to the caveats outlined above.

How found

Validating and documenting new features in EF Core 6.0.

Regression

No.

Testing

N/A

Risk

Low. The change marks the constructors that enable this as internal, but doesn't change them. We also make sure that none of our built-in converters use this feature.

…f nulls.

Resolves #26230.

Also revert built-in converters that were changed to allow this in 6.0 back to their 5.0 behavior.
@ajcvickers ajcvickers requested a review from a team October 1, 2021 21:30
@ajcvickers
Copy link
Member Author

@Pilchie For 6.0. However, I think it makes sense to send this one through Tactics.

@Pilchie
Copy link
Member

Pilchie commented Oct 1, 2021

Agreed - I think a Tactics discussion makes sense for this one.

@ajcvickers ajcvickers added this to the 6.0.0 milestone Oct 4, 2021
@ajcvickers ajcvickers removed this from the 6.0.0 milestone Oct 6, 2021
@ajcvickers ajcvickers merged commit 938a8b0 into release/6.0 Oct 9, 2021
@ajcvickers ajcvickers deleted the WannaTakeItInside1001 branch October 9, 2021 08:27
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.

Make value converting nulls internal for 6.0
4 participants