Use table mappings for FromSql #32816
Conversation
The difference between the DbSet queries (work) and the FromSql queries (don't work) is that the DbSet queries use the table mappings, while the FromSql queries use the default mappings. The default mappings don't contain complex types. This may be an issue in of itself. Fixes #32699
| @@ -483,8 +483,8 @@ SELECT 1 | |||
| SELECT "OrderID", "ProductID", "UnitPrice", "Quantity", "Discount" | |||
| FROM "Order Details" | |||
| WHERE "OrderID" < 10300 | |||
| ) AS [m] | |||
There was a problem hiding this comment.
@roji This changed aliases in a few places. I didn't dig into why, since you are changing the alias generation code anyway, right?
There was a problem hiding this comment.
I'm guessing that having an actual relational Table from the table mappings above (as opposed to a TableBase from the default mappings) - with the actual table name - makes us use its name as the alias; that actually looks like an improvement to me (I've done some stuff in #32785 but didn't really look at FromSql specifically).
|
|
||
| [ConditionalTheory] | ||
| [MemberData(nameof(IsAsyncData))] | ||
| public virtual Task Filter_on_property_inside_nested_complex_type_with_FromSql(bool async) |
There was a problem hiding this comment.
Not necessarily a problem, but does the filtering inside the SQL add any actual coverage across all these variants? The thing that gets projected out of the SQL is the same, so in effect are these checking something beyond that the database performed the SQL WHERE correctly?
There was a problem hiding this comment.
No, I just added a few tests doing various things, since we didn't have any coverage of FromSql with complex types.
There was a problem hiding this comment.
Up to you, but I'd consider not adding tests if they don't add any coverage (maintenance, test running time...)
| async, | ||
| ss => ((DbSet<Customer>)ss.Set<Customer>()).FromSql( | ||
| $""" | ||
| SELECT DISTINCT [t].[Id], [t].[Name], [t].[BillingAddress_AddressLine1], [t].[BillingAddress_AddressLine2], [t].[BillingAddress_Tags], [t].[BillingAddress_ZipCode], [t].[BillingAddress_Country_Code], [t].[BillingAddress_Country_FullName], [t].[ShippingAddress_AddressLine1], [t].[ShippingAddress_AddressLine2], [t].[ShippingAddress_Tags], [t].[ShippingAddress_ZipCode], [t].[ShippingAddress_Country_Code], [t].[ShippingAddress_Country_FullName] |
There was a problem hiding this comment.
nit: align the SQL to the left like in the above tests
| @@ -483,8 +483,8 @@ SELECT 1 | |||
| SELECT "OrderID", "ProductID", "UnitPrice", "Quantity", "Discount" | |||
| FROM "Order Details" | |||
| WHERE "OrderID" < 10300 | |||
| ) AS [m] | |||
There was a problem hiding this comment.
I'm guessing that having an actual relational Table from the table mappings above (as opposed to a TableBase from the default mappings) - with the actual table name - makes us use its name as the alias; that actually looks like an improvement to me (I've done some stuff in #32785 but didn't really look at FromSql specifically).
| @@ -104,7 +104,8 @@ protected override Expression VisitExtension(Expression extensionExpression) | |||
| _sqlExpressionFactory.Select( | |||
| fromSqlQueryRootExpression.EntityType, | |||
| new FromSqlExpression( | |||
| fromSqlQueryRootExpression.EntityType.GetDefaultMappings().Single().Table, | |||
| fromSqlQueryRootExpression.EntityType.GetTableMappings().SingleOrDefault()?.Table | |||
There was a problem hiding this comment.
OK, I see...
First, yeah, I wonder whether the right fix isn't to have the table returned by GetDefaultMappings have mappings to the entity type's complex types; in other words, is this a metadata bug rather than a query bug (/cc @AndriySvyryd).
If not, then about this specific fix: this looks like it will throw for entity splitting (no longer a single table for the entity type); it also hard-codes a preference for querying the tables (over e.g. views) when using FromSql(), which I'm not sure is right...
There was a problem hiding this comment.
I'm not sure it's right either, but then it doesn't seem like using a DbSet with FromSql should be any different from using a DbSet without FromSql.
There was a problem hiding this comment.
Let's see what @AndriySvyryd has to say about all this... Don't we support defining both a view and a table mapping for an entity type, in which case querying its DbSet would query the view?
|
If there is a view defined, then again shouldn't a FromSql query do the same as a normal query? |
I think so, but doesn't this PR's change hard-code FromSql to prefer table bindings if they exist, over the default ones? I'm not great with metadata, but doesn't that mean we'd suddenly switch over to querying the table instead of the view when doing FromSql? |
Yes, because that's what we do for DbSets. At least, that's the difference I found when debugging this. I agree that I don't know if this is right or not. |
|
I'm confused :) Maybe let's discuss in triage/design... |
|
FromSql uses the default mappings to avoid depending on how tables are mapped and as a stepping stone towards #21627 |
|
@AndriySvyryd Shay and I were talking about this earlier, and I expect the default mappings are very under-tested when it comes to queries. We should do a review of all the places that query requests a mapping and check that it is requesting the right thing, and then check we have tests for all the cases where we select or fall back to the default mappings. |
|
Filed #32853 |
|
Should we use #32699 to track adding complex types to the default mappings? |
|
Replaced by #32867 |
…pings (#32901) * Add complex type mappings to the default relational mappings (#32867) * Add complex type mappings to the default relational mappings Replaces #32816 Fixes #32699 As discussed, leaving FromSql to use the default mappings, but add complex types to the default mappings. * Add note to SelectExpression * Fix merge and add quirks
The difference between the DbSet queries (work) and the FromSql queries (don't work) is that the DbSet queries use the table mappings, while the FromSql queries use the default mappings. The default mappings don't contain complex types. This may be an issue in of itself.
Fixes #32699