FromSqlRaw_queryable_simple_projection_composed references ignored properties

[lauxjpn]

The properties/columns UnitsOnOrder and ReorderLevel are referenced here inside the query:

https://github.com/aspnet/EntityFrameworkCore/blob/fb2a8ed9381941bfe41744a80e90f9cd0ea05c83/test/EFCore.Relational.Specification.Tests/Query/FromSqlQueryTestBase.cs#L682-L699

They are being explicitly ignored in the model though:

https://github.com/aspnet/EntityFrameworkCore/blob/fb2a8ed9381941bfe41744a80e90f9cd0ea05c83/test/EFCore.Specification.Tests/TestModels/Northwind/NorthwindContext.cs#L55

This does not make the SqlServer and Sqlite tests fail, because they bootstrap their tests with a SQL dump/db file for the Northwind database, which contains the ignored column.

In Pomelo.MySql, we don't load a dump for the tests (yet) and hit an exception, due to unknown columns in the query.

We use the following code in our NorthwindQueryMySqlFixture.OnModelCreating as a workaround for now:

    modelBuilder
        .Entity<Product>(e =>
        {
            e.Property(p => p.UnitsOnOrder);
            e.Property(p => p.ReorderLevel);
        });

There exists another issue regarding this test: #16079

[smitpatel]

EF model in test if subset of what northwind data is. Make sure you have full data of northwind rather than relying on test model.
So far we have

• Sql script for SqlServer
• In memory data for InMemory
• db file for SQLite
• Serialized json for Cosmos

Make sure to use correct way to load all the data. Partial data is test issue in MySQL.
In other way, this test verify that your raw sql can contain something which is not mapped and it still works.

[lauxjpn]

Make sure to use correct way to load all the data. Partial data is test issue in MySQL.

If that is the case, then there needs to be at least a test to ensure, that the full Northwind data set is being used for testing.
Because we are hitting this issues only in this one test I consider this to be unexpected.

FromSqlRaw_queryable_simple_projection_composed is not about using the column of an Ignored property in a raw sql query.
It's about composing a raw sql query with a projection, and for that, there is no need to make this test rely on a column that is not mapped in the model.

The only point in over 13,000 tests, where it is suddenly a problem not to have preloaded the entire Northwind data set, seems to be the FromSqlRaw_queryable_simple_projection_composed method.
Therefore, instead of creating a dependency on the preloaded data set due to a single test, the test should be changed.

In other way, this test verify that your raw sql can contain something which is not mapped and it still works.

If it should be tested that a raw sql query's filter can use a column that is ignored in the model, this should be made explicit and should be it's own separate test.

[smitpatel]

@lauxjpn - Northwind database is preloaded dataset and currently we only use subset of it. If you deviate from that thing, then yes some tests may fail. As a provider writer you have 2 choices, either correct the data in your northwind or just skip/override the test. Yes, we can change this one test but I would also insist on having a test which verify that the raw sql can be disconnected from mapped model & that new separate test would fail for you.

At the end of day, it is a test assembly we publish to help provider writers and not a source code. Not everything would make sense for all providers. So providers need to figure out if some tests they do not want run. I believe in the absence of proper data in Northwind, this is case for you.

[lauxjpn]

@smitpatel If you would consider a PR that ...

• Removes the dependency between FromSqlRaw_queryable_simple_projection_composed and the full Northwind data set
• Adds a compliance test to check the availability (of a couple of not mapped columns) of the full Northwind data set

... I can provide one.

This will make not a lot of difference for us anymore, but will be one less pitfall for upcoming provider developers.

[smitpatel]

@lauxjpn - We discussed this in team and we decided to keep existing behavior. As a provider if you are utilizing specification tests you will need full dataset for Northwind.

[lauxjpn]

Adding a compliance test to check the availability (of a couple of not mapped columns) of the full Northwind data set (using simple raw SQL) is probably to way to go here then.
It would document the dependency on the full data set and make it explicit.

[smitpatel]

@lauxjpn - That is the thing, we don't think value of adding a compliance test outweighs it's value. We will mention it in our docs for provider writers.