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

Fix to #30244 - Json column fails to materialize only when default interceptor used #30403

Merged
merged 2 commits into from
Mar 15, 2023

Conversation

maumar
Copy link
Contributor

@maumar maumar commented Mar 5, 2023

Problem was that when we build materializer expression, ValueBufferTryReadValue call that extracts values of keys is typed with key's CLR type. This fails for json as we visit the materializer expression, because calls to ValueBufferTryReadValue are replaced with array accesses to object[] that we store key values in. We should be using ValueBufferTryReadValue when trying to get value for keys, like we do everywhere else, so that after the visitation expression types are correct.

Fixes #30244

maumar added 2 commits March 14, 2023 12:00
…terceptor used

Problem was that when we build materializer expression, ValueBufferTryReadValue call that extracts values of keys is typed with key's CLR type. This fails for json as we visit the materializer expression, because calls to ValueBufferTryReadValue are replaced with array accesses to object[] that we store key values in. We should be using ValueBufferTryReadValue<object> when trying to get value for keys, like we do everywhere else, so that after the visitation expression types are correct.

Fixes #30244
@ajcvickers
Copy link
Contributor

@maumar I moved the testing to the interceptor tests, which are setup to handle this.

@@ -25,5 +43,8 @@ protected override ITestStoreFactory TestStoreFactory
IServiceCollection serviceCollection,
IEnumerable<ISingletonInterceptor> injectedInterceptors)
=> base.InjectInterceptors(serviceCollection.AddEntityFrameworkSqlite(), injectedInterceptors);

public override DbContextOptionsBuilder AddOptions(DbContextOptionsBuilder builder)
=> base.AddOptions(builder.ConfigureWarnings(e => e.Ignore(SqliteEventId.CompositeKeyWithValueGeneration)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maumar Had to suppress his warning, which I don't think should apply when the owned type is mapped to JSON. Are we tracking this somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#26708, but maybe we need something json specific?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will people get this warning if they explicitly configure a composite key for a mapped JSON collection when that configuration is valid? /cc @AndriySvyryd

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will people get this warning if they explicitly configure a composite key for a mapped JSON collection when that configuration is valid?

Yes, unless they mark the non-fk property as never generated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maumar Given Andriy's response, I think we need a new issue for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ajcvickers ajcvickers marked this pull request as ready for review March 15, 2023 00:52
@maumar maumar merged commit 483bec6 into main Mar 15, 2023
@maumar maumar deleted the fix30244 branch March 15, 2023 02:33
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.

Json column fails to materialize only when default interceptor used
3 participants