Skip to content

Revert pull-in of GearsOfWar related classes and remove constraint #145

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

Merged
merged 5 commits into from
Oct 12, 2023

Conversation

lauxjpn
Copy link
Member

@lauxjpn lauxjpn commented Oct 11, 2023

Since the reason for our custom copy of the original EF Core GearsOfWar related classes is a FK constraint that is handled differently by Jet in comparison to other database engines, the simplest way to fix the issue for the test classes is to just drop the constraint.

@lauxjpn lauxjpn added this to the 8.0.0-alpha.2 milestone Oct 11, 2023
@lauxjpn lauxjpn requested a review from ChrisJollyAU October 11, 2023 10:42
@lauxjpn lauxjpn self-assigned this Oct 11, 2023
@lauxjpn lauxjpn force-pushed the test/cleanup_gearsofwar branch from 7179a3a to 1306278 Compare October 11, 2023 14:53
@ChrisJollyAU
Copy link
Member

That might work for GearsOfWar but the TPC and TPH sets are completely broken.

The best case is if we can find a solution where both the model and the database are the same. In the above it is different.

I've been experimenting with the following the the OnModelCreating of the Fixture

modelBuilder.Entity<Officer>()
                .Property(o => o.LeaderSquadId)
                .HasConversion<int?>(
                    v => v == 0 ? null : v,
                    v => v ?? 0)
                .HasColumnType("integer NULL");

but it seems to be ignoring that custom type

@lauxjpn
Copy link
Member Author

lauxjpn commented Oct 11, 2023

I've been experimenting with the following the the OnModelCreating of the Fixture

modelBuilder.Entity<Officer>()
                .Property(o => o.LeaderSquadId)
                .HasConversion<int?>(
                    v => v == 0 ? null : v,
                    v => v ?? 0)
                .HasColumnType("integer NULL");

Unless this has recently been changed, null never reaches HasConversion(). It is handled by EF Core itself. (See Note in Value Conversions: Configuring a value converter.)


The best case is if we can find a solution where both the model and the database are the same. In the above it is different.

In case we don't find another way, we can always monkey patch it.

@lauxjpn lauxjpn force-pushed the test/cleanup_gearsofwar branch from 1306278 to 7d82b94 Compare October 11, 2023 22:41
@lauxjpn
Copy link
Member Author

lauxjpn commented Oct 11, 2023

That might work for GearsOfWar but the TPC and TPH sets are completely broken.

Alright, I added the necessary constraint removal for the TPC and TPT variants as well.
All 3 classes should work fine now.

Here are the results:

Tests (Before) Tests (After) Succeeded (Before) Succeeded (After) Failed (Before) Failed (After)
GearsOfWarQueryJetTest 1281 1281 1030 1024 251 257
TPCGearsOfWarQueryJetTest 1281 1281 1020 1018 261 263
TPTGearsOfWarQueryJetTest 1281 1281 1020 1012 261 269
Total 3843 3843 3070 3054 773 789

The slight difference in before/after results might be connected to the fixture cleanup I did (which is unrelated to the constraint issue and can be reverted where necessary).


@ChrisJollyAU Can I remove the Jet:MatchSimple annotation related code as part of this PR? It doesn't seem to do anything.

@ChrisJollyAU
Copy link
Member

I've been experimenting with the following the the OnModelCreating of the Fixture

modelBuilder.Entity<Officer>()
                .Property(o => o.LeaderSquadId)
                .HasConversion<int?>(
                    v => v == 0 ? null : v,
                    v => v ?? 0)
                .HasColumnType("integer NULL");

Unless this has recently been changed, null never reaches HasConversion(). It is handled by EF Core itself. (See Note in Value Conversions: Configuring a value converter.)

The conversion is one thing, but it is the HasColumnType that is not overriding the column type. Possibly need dotnet/efcore#24685 and dotnet/efcore#27970 for that to work

With regards to the test differences, it is nothing related to the fixture cleanup. Don't even have to look at the results, I've spent enough time playing in it to know the effects. It is just AssertSql failing on the different SQL generated by having LeaderSquadId go from int? to int

MatchSimple was still in progress. It added the annotation, but I hadn't found the right area in the processing/translating to pick it up and add the correct SQL to narrow the query to behave the same way as the foreign key should

@lauxjpn lauxjpn force-pushed the test/cleanup_gearsofwar branch from 7d82b94 to 6c78760 Compare October 12, 2023 10:26
@lauxjpn
Copy link
Member Author

lauxjpn commented Oct 12, 2023

With regards to the test differences, it is nothing related to the fixture cleanup. Don't even have to look at the results, I've spent enough time playing in it to know the effects. It is just AssertSql failing on the different SQL generated by having LeaderSquadId go from int? to int

Thanks, I fixed the SQL assertions. All tests that succeeded before also succeed now:

Tests (Before) Tests (After) Succeeded (Before) Succeeded (After) Failed (Before) Failed (After)
GearsOfWarQueryJetTest 1281 1281 1030 1034 251 247
TPCGearsOfWarQueryJetTest 1281 1281 1020 1026 261 255
TPTGearsOfWarQueryJetTest 1281 1281 1020 1026 261 255
Total 3843 3843 3070 3086 773 757

The following additional test were incidentally fixed as well:

  • GearsOfWarQueryJetTest.Project_collection_navigation_with_inheritance2
  • GearsOfWarQueryJetTest.Project_collection_navigation_with_inheritance3
  • TPCGearsOfWarQueryJetTest.Correlated_collections_left_join_with_self_reference
  • TPCGearsOfWarQueryJetTest.Project_collection_navigation_with_inheritance2
  • TPCGearsOfWarQueryJetTest.Project_collection_navigation_with_inheritance3
  • TPTGearsOfWarQueryJetTest.Correlated_collections_left_join_with_self_reference
  • TPTGearsOfWarQueryJetTest.Project_collection_navigation_with_inheritance2
  • TPTGearsOfWarQueryJetTest.Project_collection_navigation_with_inheritance3

MatchSimple was still in progress. It added the annotation, but I hadn't found the right area in the processing/translating to pick it up and add the correct SQL to narrow the query to behave the same way as the foreign key should

I reverted the related commit then for now. We want to keep the master branch as production ready as possible at any time (and thereby the daily builds). (Keep it in a local/fork branch until it gets useful.)

@ChrisJollyAU
Copy link
Member

Yeah I was aware of those tests. Having the LEaderSquadId as int? produced an extra row full of NULL's rather than no row. See further comments in readme https://github.com/bubibubi/EntityFrameworkCore.Jet/blob/master/test/EFCore.Jet.CustomBaseTests/Readme.md

@ChrisJollyAU
Copy link
Member

I think the only SQL not matching is just that related to DateTimeOffset which is fine at the moment

This will work for now but we should open an issue so that we can keep an eye on it and find a better solution that keeps the model and database in sync

Also this will probably need to be backported to the 7.0 branch as well. I had initially been working on it there and cherry-picked into master

Copy link
Member

@ChrisJollyAU ChrisJollyAU left a comment

Choose a reason for hiding this comment

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

Should be fine now

@lauxjpn lauxjpn merged commit 7bc7ee3 into CirrusRedOrg:master Oct 12, 2023
@lauxjpn lauxjpn deleted the test/cleanup_gearsofwar branch October 12, 2023 11:10
@lauxjpn
Copy link
Member Author

lauxjpn commented Oct 12, 2023

I think the only SQL not matching is just that related to DateTimeOffset which is fine at the moment

Yeah, I saw that. I'll open an issue to discuss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants