Use PRAGMA defer_foreign_keys instead of PRAGMA foreign_keys when migrating

[ferferga]

This Pull Request changes the use of PRAGMA foreign_keys in SQLite migrations to PRAGMA defer_foreign_keys, which are allowed inside transactionss without any issue.

It's one of the possible fixes for #35871, and I think the best one, since we will use the exact pragma that SQLite wants to be used for transactions, instead of working around it.

I did my best attempt to update the tests as well, but not sure this is how I should do it (since I didn't see any mention to BEGIN TRANSACTION inside the generated SQL in tests.

• I've read the guidelines for contributing and seen the walkthrough
• I've posted a comment on an issue with a detailed description of how I am planning to contribute and got approval from a member of the team
• The code builds and tests pass locally (also verified by our automated build checks)
• Commit messages follow this format:

        Summary of the changes
        - Detail 1
        - Detail 2

        Fixes #bugnumber

• Tests for the changes have been added (for bug fixes / features)
• Code follows the same patterns and style as existing code in this repo

[ferferga]

@dotnet-policy-service agree

[cincuranet]

This looks good. Thanks.

[ferferga]

Removing the explicit SupressTransaction seems to have tripped the tests for some reason 😅

[cincuranet]

Yeah. It made tests very angry. :)

[cincuranet]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[cincuranet]

Tests Alter_column_remove_autoincrement and Replace_string_primary_key_with_autoincrement_identity don't have updated SQL and still use PRAGMA foreign_keys = 0;.

[cincuranet]

    Microsoft.EntityFrameworkCore.Migrations.MigrationsSqliteTest.Replace_string_primary_key_with_autoincrement_identity [FAIL]
      Assert.Equal() Failure: Strings differ
                 ↓ (pos 0)
      Expected: "PRAGMA defer_foreign_keys = OFF;"
      Actual:   "CREATE UNIQUE INDEX "IX_Person_Ssn" ON "P"···
                 ↑ (pos 0)
      Stack Trace:
           at Microsoft.EntityFrameworkCore.TestUtilities.TestSqlLoggerFactory.AssertBaseline(String[] expected, Boolean assertOrder, Boolean forUpdate)
        D:\a\_work\1\s\test\EFCore.Relational.Specification.Tests\Migrations\MigrationsTestBase.cs(3723,0): at Microsoft.EntityFrameworkCore.Migrations.MigrationsTestBase`1.AssertSql(String[] expected)
        D:\a\_work\1\s\test\EFCore.Sqlite.FunctionalTests\Migrations\MigrationsSqliteTest.cs(2132,0): at Microsoft.EntityFrameworkCore.Migrations.MigrationsSqliteTest.Replace_string_primary_key_with_autoincrement_identity()
        --- End of stack trace from previous location ---

 Microsoft.EntityFrameworkCore.Migrations.MigrationsSqliteTest.Alter_column_remove_autoincrement [FAIL]
      System.ArgumentOutOfRangeException : Index was out of range. Must be non-negative and less than the size of the collection. (Parameter 'index')
      Stack Trace:
           at Microsoft.EntityFrameworkCore.TestUtilities.TestSqlLoggerFactory.AssertBaseline(String[] expected, Boolean assertOrder, Boolean forUpdate)
        D:\a\_work\1\s\test\EFCore.Relational.Specification.Tests\Migrations\MigrationsTestBase.cs(3723,0): at Microsoft.EntityFrameworkCore.Migrations.MigrationsTestBase`1.AssertSql(String[] expected)
        D:\a\_work\1\s\test\EFCore.Sqlite.FunctionalTests\Migrations\MigrationsSqliteTest.cs(2306,0): at Microsoft.EntityFrameworkCore.Migrations.MigrationsSqliteTest.Alter_column_remove_autoincrement()
        --- End of stack trace from previous location ---

[cincuranet]

@ferferga I took over and fixed the two tests. When (if) the CI is green, I'll merge today. Thanks for your contribution!

[ferferga]

@cincuranet Sorry, I didn't see those modifications were in main and I had to update my branch. When I first fixed the tests it was really late here and did it quick (so I didn't bother running the checks locally or waiting for CI 😅). Today I was finally available again and just right now I was spinning up a Codespace to fix them, but thank you very much for being faster than me! 😊

One question I have now: I noticed that, instead of using 1 or 0, I used ON or OFF respectively, and I'm not sure now if I did it for any special reason. What do you think about using 1 and 0 with defer_foreign_keys as well? Thinking it now, it seems better to me: It's more consistent with foreign_keys's EFCore usage (which is still used when establishing a connection to the database) and, although negligible and probably a useless microoptimization, the generated SQL is shorter and hence it might be processed faster.

Let me know what do you think, luckily it's just a find&replace thing.

[cincuranet]

I personally like ON/OFF better. But for the consistency sake, let's keep it 1/0.