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

Various stuff from EFCore.PG JSON work #32255

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

roji
Copy link
Member

@roji roji commented Nov 8, 2023

Various mostly trivial changes that came out of the EFCore.PG JSON work (npgsql/efcore.pg#2922).

@roji roji requested a review from a team November 8, 2023 14:30
@@ -47,6 +47,59 @@ public virtual void Can_read_write_collection_of_ASCII_string_JSON_values(object
{ RelationalAnnotationNames.StoreType, storeType }, { CoreAnnotationNames.Unicode, false }
});

public override void Can_read_write_ulong_enum_JSON_values(EnumU64 value, string json)
Copy link
Member Author

Choose a reason for hiding this comment

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

This lifts the overrides for unsigned types to the relational level, since SQL databases don't have unsigned types.

Comment on lines +2534 to +2536
throw new InvalidOperationException(
RelationalStrings.JsonValueReadWriterMissingOnTypeMapping(
property.GetTypeMapping().GetType().Name, property.Name, property.DeclaringType.DisplayName()));
Copy link
Member

Choose a reason for hiding this comment

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

This would be better suited for model validation

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I agree. Unless you have some specific objections, I'll also leave this here just in case.

Copy link
Member

Choose a reason for hiding this comment

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

It would be duplicate code, we generally don't throw for things that were already validated to be true. You could leave it as an assert.

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.

None yet

2 participants