Skip to content

Remove lookup of JsonTypeMapping via JsonElement #32192

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

Closed
roji opened this issue Oct 30, 2023 · 3 comments · Fixed by #35788
Closed

Remove lookup of JsonTypeMapping via JsonElement #32192

roji opened this issue Oct 30, 2023 · 3 comments · Fixed by #35788

Comments

@roji
Copy link
Member

roji commented Oct 30, 2023

In order to find the provider's JsonTypeMapping, our code currently does a standard lookup with CLR type JsonElement (as a sort of special value). The problem is that some provider's actually do support JsonElement as a real property CLR type; in addition, if a user defines a JsonElement property on some entity type, this type mapping will be return, and things will likely fail in some unpredictable way.

We should find another way to look up the JsonTypeMapping, e.g. define some special CLR type instead of JsonElement that could be used, or introduce a new API or something.

/cc @maumar

@maumar
Copy link
Contributor

maumar commented Oct 31, 2023

dummy clr type sounds good to me

@roji
Copy link
Member Author

roji commented Feb 23, 2024

This is unfortunately more than a cleanup - the current hacky way of doing things in EFCore.PG unfortunately doesn't take care of literal generation, which is necessary e.g. when adding a column to an existing table (for the default value). There, CSharpHelper.UnknownLiteral is used, which just does a type mapping lookup based on the CLR type (JsonElement), retrieving the owned JSON type mapping rather than the non-owned one.

See npgsql/efcore.pg#3107 (comment) for more details.

@roji
Copy link
Member Author

roji commented Feb 28, 2025

This also blocks being able to use ToJson() with the PostgreSQL non-default jsonb type (I managed to hack around this for the default jsonb type); see npgsql/efcore.pg#3475 (comment).

@maumar do you think we can do this for 10?

@maumar maumar modified the milestones: Backlog, 10.0.0 Mar 12, 2025
@maumar maumar self-assigned this Mar 12, 2025
maumar added a commit that referenced this issue Mar 14, 2025
When adding support for JSON mapped types, we decided to use JsonElement as a CLR type marker for JsonTypeMapping. This decision was in hindsight incorrect, it makes it hard for providers that actually support weak-typed json (npgsql) and throws cryptic exception (No coercion operator is defined between types 'System.IO.MemoryStream' and 'System.Text.Json.JsonElement) for providers that don't support it, when customers try to do it regardless.
Fix is to create a dummy type and use that instead, so JsonElement is no longer recognized by base EFCore.

Fixes #32192
also fixes #34752
maumar added a commit that referenced this issue Mar 14, 2025
When adding support for JSON mapped types, we decided to use JsonElement as a CLR type marker for JsonTypeMapping. This decision was in hindsight incorrect, it makes it hard for providers that actually support weak-typed json (npgsql) and throws cryptic exception (No coercion operator is defined between types 'System.IO.MemoryStream' and 'System.Text.Json.JsonElement) for providers that don't support it, when customers try to do it regardless.
Fix is to create a dummy type and use that instead, so JsonElement is no longer recognized by base EFCore.

Fixes #32192
also fixes #34752
maumar added a commit that referenced this issue Mar 14, 2025
When adding support for JSON mapped types, we decided to use JsonElement as a CLR type marker for JsonTypeMapping. This decision was in hindsight incorrect, it makes it hard for providers that actually support weak-typed json (npgsql) and throws cryptic exception (No coercion operator is defined between types 'System.IO.MemoryStream' and 'System.Text.Json.JsonElement) for providers that don't support it, when customers try to do it regardless.
Fix is to create a dummy type and use that instead, so JsonElement is no longer recognized by base EFCore.

Fixes #32192
also fixes #34752
maumar added a commit that referenced this issue Mar 14, 2025
When adding support for JSON mapped types, we decided to use JsonElement as a CLR type marker for JsonTypeMapping. This decision was in hindsight incorrect, it makes it hard for providers that actually support weak-typed json (npgsql) and throws cryptic exception (No coercion operator is defined between types 'System.IO.MemoryStream' and 'System.Text.Json.JsonElement) for providers that don't support it, when customers try to do it regardless.
Fix is to create a dummy type and use that instead, so JsonElement is no longer recognized by base EFCore.

Fixes #32192
also fixes #34752
@maumar maumar closed this as completed in f94e421 Mar 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants