KEMBAR78
Support JSON columns in compiled models by ajcvickers · Pull Request #30254 · dotnet/efcore · GitHub
Skip to content

Conversation

@ajcvickers
Copy link
Contributor

Fixes #29602

The main change here is to store the JSON type mapping in the relational model, and obsolete the previous storage in the underlying EF model.

@ajcvickers ajcvickers requested a review from a team February 11, 2023 00:56
var jsonColumnName = entityType.GetContainerColumnName()!;
var jsonColumnTypeMapping = (entityType.GetViewOrTableMappings().SingleOrDefault()?.Table
?? entityType.GetDefaultMappings().Single().Table)
.FindColumn(jsonColumnName)!.StoreTypeMapping;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AndriySvyryd I'm not sure if this is right. Should it just look on the DefaultMappings? Does the column need to be added to both the table mappings and the default mappings? If so, what's the appropriate way to do this? Should it be the same column instance?

Copy link
Member

Choose a reason for hiding this comment

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

Default mapping is currently used for FromSql, so yes, the column should be added to the default mapping also

Copy link
Member

Choose a reason for hiding this comment

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

Here you should decide whether to use Table, View or default mapping based on what mapping is being used for the container entity type

var jsonColumnTypeMapping = targetEntityType.GetContainerColumnTypeMapping()!;
var jsonColumnTypeMapping = (entityType.GetViewOrTableMappings().SingleOrDefault()?.Table
?? entityType.GetDefaultMappings().Single().Table)
.FindColumn(jsonColumnName)!.StoreTypeMapping;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same question here.

@ajcvickers
Copy link
Contributor Author

@maumar @AndriySvyryd As I understand it, query won't use the default mapping unless there is no table or view mapping. Do we have existing query tests for the default mapping? (Presumably, FromSql tests where the entity type is explicitly not mapped to a table or view?) If so, where?

@ajcvickers ajcvickers force-pushed the JsonAndTheCompilonauts029 branch from 9166ddc to 4b56e9b Compare February 18, 2023 11:02
@ajcvickers
Copy link
Contributor Author

@AndriySvyryd @maumar Updated to store the container column in the default mapping.

@AndriySvyryd
Copy link
Member

As I understand it, query won't use the default mapping unless there is no table or view mapping

FromSql should always use it, though there might be bugs where a different mapping is used when translating properties than the one used for the entity type.

/// </summary>
public virtual RelationalTypeMapping StoreTypeMapping
=> PropertyMappings.First().TypeMapping;
=> _storeTypeMapping ?? PropertyMappings.First().TypeMapping;
Copy link
Member

Choose a reason for hiding this comment

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

Store the result in _storeTypeMapping

Fixes #29602

The main change here is to store the JSON type mapping in the relational model, and obsolete the previous storage in the underlying EF model.
@ajcvickers ajcvickers force-pushed the JsonAndTheCompilonauts029 branch from 4b56e9b to f73a22e Compare March 1, 2023 11:56
@ajcvickers
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ajcvickers ajcvickers merged commit f0315cf into main Mar 1, 2023
@ajcvickers ajcvickers deleted the JsonAndTheCompilonauts029 branch March 1, 2023 19:03
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.

Compiled models with JSON columns

2 participants