-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Block collections of scalars with converters on Cosmos #34307
Conversation
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.
The change looks good, but see my comment about being unsure whether we're blocking too much (e.g. GUID arrays).. We may want to have a quick design chat here if you want.
|
||
static void ValidateType(ITypeBase typeBase, IDiagnosticsLogger<DbLoggerCategory.Model.Validation> logger) | ||
{ | ||
foreach (var property in typeBase.GetDeclaredProperties()) |
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.
Unrelated: could be good to have a utility extension for getting all declared properties recursively (maybe it already even exists - I feel like I've done this several times already, worth checking).
{ | ||
foreach (var property in typeBase.GetDeclaredProperties()) | ||
{ | ||
var typeMapping = property.GetElementType()?.GetTypeMapping(); |
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.
Not sure, but maybe it's also worth blocking if a vlue converter is defined on the collection property itself - not sure that works either...
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.
A value converter on the collection property causes the code to fall back to being a simple, opaque, value-converted property. It basically gets you back to the behavior (with custom converters) before we had built in configuration for primitive collections.
@@ -270,51 +272,30 @@ public override async Task Array_of_bool() | |||
|
|||
public override async Task Array_of_Guid() | |||
{ | |||
await base.Array_of_Guid(); | |||
Assert.Equal( |
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 think I wasn't aware we were handling e.g. GUID arrays via value converters... Do you think we're maybe blocking too much here? In other words, if it's only queries over such collections that don't work, maybe we shouldn't block the whole thing (at model validation) to allow at least serialization/deserialization? Does the update pipeline handle these correctly (i.e. #34153), and what actually worked in 8.0?
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.
Arrays of GUIDs were not supported in 8--throws in model validation--so I think we're okay to continue not supporting them in 9. That being said, if you think the query part for GUIDs converted to strings is something that works/can be made to work in 9, then we can specifically allow just that. Same for enums.
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.
Yeah, so as discussed offline, if this stuff wasn't supported before I think it's fine not to support it now either (until we fully support it). We could allow it in validation so that people can at least serialize/deserialize it, but I think it's fine to also not allow it.
Fixes #34218