Support mapping identity columns to enum properties #25536
Conversation
| if (converter != null) | ||
| { | ||
| var type = converter.ProviderClrType.UnwrapNullableType(); | ||
| if (!type.IsInteger() |
There was a problem hiding this comment.
This seems like a fairly provider-specific thing to embed in the general relational code (e.g. PG doesn't support decimal "identity" columns)...
I don't have a lot of context so I'm not sure what this is supposed to address... but are we OK with other value-generated properties with converters (e.g. GUID, or string or whatever)?
There was a problem hiding this comment.
Sounds like you have feedback on #25178
There was a problem hiding this comment.
Well, AFAIK I'm OK with value conversion for identity/sequence columns, and I guess I'm also OK with value conversion combined with client-side value generation (though maybe I'm missing issues). The check that #25178 removed was against all combinations of conversion and generation, where the check being added here seems to bring back restrictions which seem specific to the SQL Server concept of identity, no?
There was a problem hiding this comment.
Asking for @AndriySvyryd's opinion on this, since he shared concerns that we were being too lenient by removing the entire check.
There was a problem hiding this comment.
I don't know of any specific issues, I'm just concerned with the lack of testing for other types
There was a problem hiding this comment.
@AndriySvyryd Are you in favor of adding this check back in in, as in this PR?
Fixes #11970 Also, add back in the provider-agnostic check for converters with value generators, but make it less restrictive.
ajcvickers
force-pushed
the
ItsAnEnumJimButNotAsYouKnowIt0816
branch
from
803545c
to
30957d1
Compare
Fixes #11970
Also, add back in the provider-agnostic check for converters with value generators, but make it less restrictive.