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 JsonElement has not implemented code literal generation #3111

Conversation

luca-esse
Copy link

@luca-esse luca-esse commented Feb 22, 2024

Fixes #3107

Implementing the GenerateCodeLiteral on the "NpgsqlOwnedJsonTypeMapping seems to fix the System.NotSupportedException: The type mapping for 'JsonElement' has not implemented code literal generation." error in the migrations, as explained in my issue #3107.

I've done manual tests to ensure that migrations for the new JSON Owned types are still working with this fix and there doesn't seem to be any issue.

Let me know if you see any issue with this.

Thank you

@roji
Copy link
Member

roji commented Feb 22, 2024

@luca-esse I haven't investigated #3107 or looked at this in any depth, but as the name suggests, NpgsqlOwnedJsonTypeMapping is used for owned entity mapping (the general EF support), and not when trying to map JsonElement properties (which are the Npgsql-specific, weakly-typed "DOM" way to map); see the docs for more info on these mapping methods.

@roji roji closed this Feb 22, 2024
@luca-esse
Copy link
Author

luca-esse commented Feb 23, 2024

@roji well, if you look at my issue, you will see that this is not the case unfortunately, the NpgsqlOwnedJsonTypeMapping is also used for the JsonElement properties, since you have commented out the original clrMapper for the JsonElement and put this one instead.

here is a blame to the commented out line

and this is the current mapping:
{ typeof(JsonElement), _jsonbOwned },

As you can see, now the jsonElement is mapped to the NpgsqlOwnedJsonTypeMapping and this breaks the migrations if this method is not implemented, as explained in my issue

This is not the case for example with the JsonDocument, which is still mapped to NpgsqlJsonTypeMapping and in fact it’s working fine

@Laurianti
Copy link

This is a significant regression that seriously affects the ability to use JsonElement.

It warrants a more thorough investigation to understand the full impact and find a viable solution.

dotnet/efcore#32192

@roji
Copy link
Member

roji commented Feb 23, 2024

@luca-esse @Laurianti I spent some time to investigate this, and posted my findings as #3107 (comment) - please take a look. @luca-esse you're right that adding literal generation to NpgsqlOwnedJsonTypeMapping (as this PR does) would work around the specific bug, but that goes even further in the (current) hacky direction, whereas we really need to clean this up properly instead. The workaround I posted (specifying the default SQL explicitly) seems reasonable until that happens.

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.

Migrations blocked by: The type mapping for 'JsonElement' has not implemented code literal generation
3 participants