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.

Sign up for GitHub

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

Jump to bottom

Experimental support for the Azure SQL JSON type #34401

Merged
merged 7 commits into from
Aug 14, 2024
Merged

Experimental support for the Azure SQL JSON type #34401

merged 7 commits into from
Aug 14, 2024

Conversation

ajcvickers
Copy link
Contributor

Fixes #28452
Fixes #32150

Remaining work:

  • Test reverse engineering from an existing database
  • Output a warning when the native JSON type is used
  • Replace the ToJson overload with HasColumnType()
  • Move the type mapping visitation to another visitor

Known issues:

  • Various issues communicated with the SQL team--see TODO:SQLJSON
  • Testing is disabled until we have an appropriate server and driver to test against<!--

@ajcvickers ajcvickers force-pushed the JsonButNotTooMuch branch 5 times, most recently from b6250a2 to b35704a Compare August 12, 2024 17:32
ajcvickers
ajcvickers commented Aug 12, 2024
View reviewed changes
src/EFCore.SqlServer/Update/Internal/SqlServerModificationCommand.cs
{
Value = value,
TypeMapping = parameters.TypeMapping is SqlServerJsonElementTypeMapping
? SqlServerStringTypeMapping.UnicodeDefault
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't great, but I think can be cleaned up when we tackle the Element mapping work.

@ajcvickers ajcvickers marked this pull request as ready for review August 12, 2024 17:35
@ajcvickers ajcvickers requested a review from a team August 12, 2024 17:35
@ajcvickers
Copy link
Contributor Author

@roji This is ready for review from the EF side.

@roji roji mentioned this pull request Aug 13, 2024
Open
roji
roji reviewed Aug 13, 2024
View reviewed changes
Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

Here are the last comments - looks really good overall.

@AndriySvyryd may be interested in taking a look for the model building (and possibly update pipeline) aspects.

src/EFCore.SqlServer/Storage/Internal/SqlServerJsonElementTypeMapping.cs Outdated Show resolved Hide resolved
src/EFCore.SqlServer/Storage/Internal/SqlServerStringTypeMapping.cs Outdated Show resolved Hide resolved
src/EFCore.SqlServer/Storage/Internal/SqlServerStringTypeMapping.cs
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public static SqlServerStringTypeMapping JsonTypeDefault { get; } = new("json", sqlDbType: (SqlDbType)35);
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering whether it makes sense to have a separate SqlServerJsonTypeMapping that isn't SqlServerStringTypeMapping... It's a different database type with a different SqlDbType, SqlServerStringTypeMapping is already a bit complex with all the different facets, and a different type would allow for easier pattern matching e.g. in the query pipeline...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this at first, but the interactions with the element type mapping make it difficult. We need to remove (fix) the element type mapping and then see where this lands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@roji I was thinking about this again last night, while listening to Indian Rap Metal, and I think the question is do we want to allow a SqlServerStringTypeMapping with a store type of "json" to be treated as if it points to the native JSON type (even if a different type mapping exists), as if it's a string type with a custom database name--such as "text" for example, or be not allowed--such as "int"? I suspect that the answer is it should not be allowed, because if its a JSON type, we shouldn't treat it as a string. If you agree, I can try that now (that is, introduce a new type mapping and validate that a string type mapping is never used for the "json" type), and if it gets tricky due to the element mapping, then I'll add it in the issue for replacing the element mapping.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think that makes sense to me...

I may be missing something but... I'm not sure how important it is to guard against SqlServerStringTypeMapping with json (or int) - how would a user actually make that happen, assuming we never return it from SqlServerTypeMappingSource? In other words, if the type mapping source returns the specific SqlServerJsonTypeMapping whenever the column type is json, that seems sufficient...

But no objection to also checking/blocking or whatever... Anything here is fine I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@roji Agreed that it is somewhat unrealistic that an application would explicitly create a type mapping. Right now, we internally get type mappings not through the source. So it's probably more of an internal concern than an application facing concern. My main concern is that we don't sometimes use string type mappings, and sometimes not.

Copy link
Member

Choose a reason for hiding this comment

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

Understood - makes sense then as a sanity check!

src/EFCore.SqlServer/Storage/Internal/SqlServerStringTypeMapping.cs Show resolved Hide resolved
src/EFCore.SqlServer/Update/Internal/SqlServerModificationCommand.cs Outdated Show resolved Hide resolved
test/EFCore.SqlServer.FunctionalTests/Query/PrimitiveCollectionsQuerySqlServerJsonTypeTest.cs Outdated Show resolved Hide resolved
test/EFCore.SqlServer.FunctionalTests/Scaffolding/SqlServerDatabaseModelFactoryTest.cs Outdated Show resolved Hide resolved
test/EFCore.SqlServer.FunctionalTests/Update/JsonUpdateSqlServerTest.cs Show resolved Hide resolved
test/EFCore.SqlServer.FunctionalTests/Update/JsonUpdateSqlServerTest.cs Show resolved Hide resolved
test/EFCore.SqlServer.Tests/Infrastructure/SqlServerModelValidatorTest.cs Outdated Show resolved Hide resolved
@ajcvickers ajcvickers force-pushed the JsonButNotTooMuch branch 2 times, most recently from ee9d4b2 to ee8ffd5 Compare August 13, 2024 17:52
@ajcvickers ajcvickers requested a review from roji August 13, 2024 17:55
@AndriySvyryd
Copy link
Member

Add tests to CSharpMigrationsGeneratorTest and CompiledModelSqlServerTest

roji
roji approved these changes Aug 13, 2024
View reviewed changes
Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

OK to merge from my side - but see remaining minor nits and Andriy's comments.

src/EFCore.SqlServer/Query/Internal/SqlServerJsonPostprocessor.cs Outdated Show resolved Hide resolved
src/EFCore.SqlServer/Query/Internal/SqlServerJsonPostprocessor.cs Outdated Show resolved Hide resolved
src/EFCore.SqlServer/Query/Internal/SqlServerJsonPostprocessor.cs Outdated Show resolved Hide resolved
src/EFCore.SqlServer/Update/Internal/SqlServerModificationCommand.cs Show resolved Hide resolved
roji
roji reviewed Aug 14, 2024
View reviewed changes
test/EFCore.SqlServer.FunctionalTests/Query/PrimitiveCollectionsQuerySqlServerJsonTypeTest.cs

namespace Microsoft.EntityFrameworkCore.Query;

[SqlServerCondition(SqlServerCondition.SupportsFunctions2022 | SqlServerCondition.SupportsJsonType)]
Copy link
Member

Choose a reason for hiding this comment

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

Is the SupportsFunctions2022 still necessary now that we have SupportsJsonType?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Define "necessary". We still have tests that require this but don't require the JSON type. We could stop running those tests...

Copy link
Member

Choose a reason for hiding this comment

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

Well, the regular PrimitiveCollectionsQuerySqlServerTest (without the JSON type) doesn't have that condition, so why does the JSON version of that test suite need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 160 version of those tests does.

Here's where it is used:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically, I made the JSON type tests for primitive collections require SQL Server 160. But that doesn't mean that all the tests that need 160 (or these functions) require the JSON type.

So, we could collapse, but if we do it before the JSON type is supported anyway, then we stop running all those tests.

Copy link
Member

Choose a reason for hiding this comment

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

I see, OK then - makes sense.

@ajcvickers
Experimental support for the Azure SQL json type
1a0e1b8 
Fixes #28452
Fixes #32150

Remaining work:

- Test reverse engineering from an existing database
- Output a warning when the native JSON type is used
- Replace the ToJson overload with HasColumnType()
- Move the type mapping visitation to another visitor

Known issues:

- Various issues communicated with the SQL team--see TODO:SQLJSON
- Testing is disabled until we have an appropriate server and driver to test against
@ajcvickers
Revert changes needed for testing.
8b39f31
@ajcvickers
Fix SQLite tests
e1e0a95
@ajcvickers
EF code updates
d5f4b83
@ajcvickers
More review updates.
55cc30c
@ajcvickers ajcvickers mentioned this pull request Aug 14, 2024
Open
@ajcvickers ajcvickers mentioned this pull request Aug 14, 2024
Closed
@ajcvickers
Copy link
Contributor Author

Add tests to CSharpMigrationsGeneratorTest and CompiledModelSqlServerTest

Can you be more specific about what you want testing here?

@ajcvickers ajcvickers mentioned this pull request Aug 14, 2024
Open
@ajcvickers
Copy link
Contributor Author

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ajcvickers ajcvickers merged commit 37599d2 into main Aug 14, 2024
7 checks passed
@ajcvickers ajcvickers deleted the JsonButNotTooMuch branch August 14, 2024 14:38
@AndriySvyryd
Copy link
Member

AndriySvyryd commented Aug 14, 2024 •
edited
Loading

Can you be more specific about what you want testing here?

Tests that validate that the new json type mapping for owned types is preserved in the snapshot and in the compiled model.

@AndriySvyryd
Copy link
Member

Also add a test that validates that the appropriate migration operation is generated when the column type of an owned type mapped to json is changed

@AndriySvyryd AndriySvyryd mentioned this pull request Sep 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndriySvyryd AndriySvyryd AndriySvyryd left review comments

@roji roji roji approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@ajcvickers @AndriySvyryd @roji