[GH-4243] more sqlite schema manager fixes #4256

beberlei · 2020-09-06T07:38:31Z

Q	A
Type	bug
BC Break	no
Fixed issues	#4243

Summary

The previous fix #4246 did not fully fix the bug, because it missed handling of constraint_name == NULL in a downstream function. This lead to foreign keys without name grouped together.

Patch includes one regression example copied from Taylor in the original Laravel issue, but could use another one from @greg0ire example with the documents table, pending the original SQL to create it from Laravel blueprint.

greg0ire · 2020-09-06T10:03:14Z

The blueprint code for the documents table is here: https://github.com/uuf6429/doc-sqlite-poc/blob/b9b5bd688342ca7d0b75addf8d54579e50a8c4d7/database/migrations/2030_09_02_3_create_documents_table.php

That repo also comes with an SQLite database versioned in it. Using sqliteman to describe the documents table, I obtain the following piece of SQL:

CREATE TABLE "documents" (
    "hash" varchar not null,
    "id" integer not null primary key autoincrement,
    "created_at" datetime null,
    "updated_at" datetime null,
    "title" varchar not null,
    "ek" varchar not null,
    "iv" varchar not null,
    "deleted_at" datetime null,
    "type_id" integer not null,
    "group_id" integer null,
    foreign key("type_id") references "types"("id") on delete cascade,
    foreign key("group_id") references "groups"("id") on delete cascade
)

cc @uuf6429

tests/Doctrine/Tests/DBAL/Functional/Schema/SqliteSchemaManagerTest.php

beberlei · 2020-09-06T14:00:54Z

lib/Doctrine/DBAL/Schema/Comparator.php

-                        $changes++;
-                        unset($fromFkeys[$key1], $toFkeys[$key2]);
-                    }
+                    continue 2;


The continue 2; are new, but i believe they were missing in the previous code. If the "fromKey" from the outer loop was matched to a "toKey", then we can skip to the next "fromKey", and don't need to match the current fromKey to another "toKey".

This change in the code that is used by all platforms is only covered by an SQLite test. Would it be possible to rework the new test to use the DBAL APIs and run across platforms?

Wouldn't applying https://github.com/doctrine/dbal/pull/4256/files#r484110298 be enough to do this? Or does "use the DBAL APIs" mean something else?

@greg0ire, yes, it's exactly the same. I just reiterated because the previous comment wasn't taken into account.

I tried it in #4262 , and it works only for SQLite although I used a high level API.

It is fine if it only works with SQLite right now, we can mark the test incomplete for other platforms. But this way, we at least identified the inconsistency in the API (which is a bug) which will be documented in the form of an incomplete test.

beberlei · 2020-09-06T14:11:49Z

AppVeyor fail is unrelated

greg0ire · 2020-09-06T14:53:04Z

Fixed issues | #4243

Not entirely, but it's definitely a step forward 👍

After that we still have to figure out the issue of implicit indexes, that fails even with this patch:

git clone git@github.com:uuf6429/doc-sqlite-poc.git
cd doc-sqlite-poc
composer config repositories.beberlei vcs https://github.com/beberlei/dbal
composer require doctrine/dbal "dev-GH-4243-MoreSqliteSchemaManagerFixes as 2.10.3"
vendor/bin/phpunit tests/DatabaseTest.php
PHPUnit 8.5.8 by Sebastian Bergmann and contributors.

E                                                                   1 / 1 (100%)

Time: 189 ms, Memory: 10.00 MB

There was 1 error:

1) Tests\DatabaseTest::testThatTableExists
Illuminate\Database\QueryException: SQLSTATE[HY000]: General error: 1 no such index: IDX_A2B07288FE54D947 (SQL: DROP INDEX IDX_A2B07288FE54D947)

With this PR + greg0ire@68fe000 , that test passes, but that's probably not the best fix we can come up with 😅 .

lib/Doctrine/DBAL/Schema/Comparator.php

lib/Doctrine/DBAL/Schema/SqliteSchemaManager.php

tests/Doctrine/Tests/DBAL/Functional/Schema/SqliteSchemaManagerTest.php

morozov · 2020-09-06T15:49:45Z

The change from greg0ire@68fe000 is a BC break. I wouldn't consider it for 2.10.x (and for any newer release either) even in theory.

morozov · 2020-09-06T15:53:08Z

AppVeyor fail is unrelated

Offtopic: it fails regularly on AppVeyor due to not being able to submit code coverage to CodeCov. The question is, does it only happen on AppVeyor due to connectivity issues between AppVeyor and CodeCov (but not between GitHub and CodeCov or Travis and CodeCov) or because it's the only place where we fail the build that cannot submit code coverage.

This all instability results in fluctuation of the code coverage and misleading coverage diffs.

greg0ire · 2020-09-06T16:48:33Z

The change from greg0ire/dbal@68fe000 is a BC break. I wouldn't consider it for 2.10.x (and for any newer release either) even in theory.

Me neither to be clear: it's just here to demonstrate that the issue that will be left after this is merged has to do with implicit indexes.

lib/Doctrine/DBAL/Schema/SqliteSchemaManager.php

tests/Doctrine/Tests/DBAL/Functional/Schema/SqliteSchemaManagerTest.php

morozov

It looks like both new tests pass on 2.10.x without the changes in lib/.

lib/Doctrine/DBAL/Schema/SqliteSchemaManager.php

beberlei · 2020-09-09T16:18:26Z

@morozov thats bad, for the compare foreign key that is expected, because I added it as a guard for the new continue 2; statements, but for the functional test that means the meaning was lost while converting it from the specific Sqlite test to the generic test. Will look what happened, but I have an idea.

beberlei · 2020-09-09T16:27:49Z

@morozov it has to be an sqlite specific test, because the schema that breaks it cannot be created with the Schema abstraction, it will always generate a constraint name.

morozov · 2020-09-09T16:46:05Z

@beberlei I think I understand what you're saying. To make it documented, could you post the SQL that needs to be generated to fail the test (it should exist in the earlier versions of this PR) and the closest possible SQL that could be produced by the Schema API. This way, it will be clear why it's an SQLite-only scenario.

beberlei · 2020-09-09T17:00:43Z

@morozov i would rather revert to the old test with the SQL and document it, wouldn't it be better to have a working regression test for this?

The test is only relevant on SQLite in anyways, because the other databases did not have this bug, because they don't have supportsCreateDropForeignKey returning false. So its not problematic that we don't cover the abstraction here, as its more a regression test for Sqlite.

It seems i deprecated the necessary method on Table addUnnamedForeignKeyConstraint, though it wouldn't work anymore anyways at it automatically generates a name now. It would be possible using another deprecated method, but thats also a funky way:

$referencingTable->addNamedForeignKeyConstraint(null, ...);

beberlei · 2020-09-09T17:08:21Z

@greg0ire shouldn't the SQL in https://github.com/doctrine/dbal/pull/4256/files#diff-ffff5b2690b3aab7bb35b745b6ca89b9R261 already lead to the DROP INDEX problem as well? wondering how to expand the test to get that failure as well.

morozov · 2020-09-09T17:26:42Z

i would rather revert to the old test with the SQL and document it

Absolutely. I just wanted it to be clarified in the discussion of this PR. So, as I understand, it can't be tested via the abstraction because it doesn't allow to create an anonymous FK for SQLite? If that's the case, then it's a clear enough explanation. Let's have it clearly stated in the PR header and possibly in the comment to the test.

morozov · 2020-09-09T17:32:54Z

The next question (not blocking the work on the fix) is, why does DBAL force naming the object (e.g. an FK) if the underlying platform allows it to be anonymous? Even if it's to simplify the schema management, there's still a wrong assumption that such objects should be identified by name. They should be identified by value (which is implemented to some extent for indices).

When introducing SQLite foreign key support a case was not considered where not Doctrine DBAL is creating the schema, but some other code that is not naming foreign key constraints. This leads to triggr a bug that the code already had but was never before executed.

beberlei · 2020-09-09T17:40:06Z

@morozov are the Postgresql failures unrelated? https://travis-ci.org/github/doctrine/dbal/jobs/725648695 they suddenly started to pop up, 2.10.x last commit is green.

morozov · 2020-09-09T17:46:09Z

@beberlei I've never seen these specific failures on Postgres. I restarted the job to see if they persist. UPD: the restarted job failed in the same way.

It looks like the server-side errors are expected but they are not handled properly.

morozov · 2020-09-09T17:49:19Z

Is it expected that testCompareForeignKeysOfTable() passes even without the change in the comparator?

greg0ire · 2020-09-09T21:03:35Z

@greg0ire shouldn't the SQL in https://github.com/doctrine/dbal/pull/4256/files#diff-ffff5b2690b3aab7bb35b745b6ca89b9R261 already lead to the DROP INDEX problem as well? wondering how to expand the test to get that failure as well.

Having any FK should be enough I think, the way this bug happens is DBAL wrongly supposes the index is supposed to exist because who says FK in SQLite says DBAL-implicit index, but if the table is created with Laravel blueprint that will not be the case. Maybe there should be a separate test case where we attempt the same thing that is attempted in https://github.com/uuf6429/doc-sqlite-poc/blob/master/database/migrations/2030_09_02_4_alter_documents_table.php

morozov · 2020-09-12T18:59:20Z

Irrelevant as of #4255.

beberlei requested review from morozov and greg0ire September 6, 2020 07:38

beberlei changed the base branch from 2.11.x to 2.10.x September 6, 2020 07:38

beberlei force-pushed the GH-4243-MoreSqliteSchemaManagerFixes branch 2 times, most recently from 9a324e1 to f47f593 Compare September 6, 2020 07:50

beberlei mentioned this pull request Sep 6, 2020

SQLite: Escape numbers as keywords #4247

Closed

greg0ire reviewed Sep 6, 2020

View reviewed changes

tests/Doctrine/Tests/DBAL/Functional/Schema/SqliteSchemaManagerTest.php Show resolved Hide resolved

beberlei force-pushed the GH-4243-MoreSqliteSchemaManagerFixes branch from 11e36f0 to 010f69b Compare September 6, 2020 13:59

beberlei commented Sep 6, 2020

View reviewed changes

greg0ire previously approved these changes Sep 6, 2020

View reviewed changes

morozov reviewed Sep 6, 2020

View reviewed changes

beberlei dismissed greg0ire’s stale review via 0748561 September 6, 2020 17:10

beberlei force-pushed the GH-4243-MoreSqliteSchemaManagerFixes branch from 0748561 to 16eab09 Compare September 6, 2020 17:15

morozov reviewed Sep 6, 2020

View reviewed changes

lib/Doctrine/DBAL/Schema/SqliteSchemaManager.php Outdated Show resolved Hide resolved

morozov reviewed Sep 6, 2020

View reviewed changes

tests/Doctrine/Tests/DBAL/Functional/Schema/SqliteSchemaManagerTest.php Show resolved Hide resolved

beberlei force-pushed the GH-4243-MoreSqliteSchemaManagerFixes branch from 017ebfc to f5ccf9c Compare September 6, 2020 19:17

morozov reviewed Sep 6, 2020

View reviewed changes

tests/Doctrine/Tests/DBAL/Functional/Schema/SqliteSchemaManagerTest.php Outdated Show resolved Hide resolved

greg0ire mentioned this pull request Sep 9, 2020

Carry #4256 #4262

Closed

beberlei force-pushed the GH-4243-MoreSqliteSchemaManagerFixes branch from 8f8a24a to 0c384b4 Compare September 9, 2020 12:31

morozov reviewed Sep 9, 2020

View reviewed changes

lib/Doctrine/DBAL/Schema/SqliteSchemaManager.php Outdated Show resolved Hide resolved

beberlei force-pushed the GH-4243-MoreSqliteSchemaManagerFixes branch from 308247d to 523cdc0 Compare September 9, 2020 17:39

morozov closed this Sep 12, 2020

morozov self-assigned this Sep 12, 2020

github-actions bot locked as resolved and limited conversation to collaborators Jul 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GH-4243] more sqlite schema manager fixes #4256

[GH-4243] more sqlite schema manager fixes #4256

beberlei commented Sep 6, 2020

greg0ire commented Sep 6, 2020 •

edited

Loading

beberlei Sep 6, 2020

morozov Sep 6, 2020

greg0ire Sep 8, 2020

morozov Sep 8, 2020

greg0ire Sep 8, 2020

morozov Sep 8, 2020

beberlei commented Sep 6, 2020

greg0ire commented Sep 6, 2020 •

edited

Loading

morozov commented Sep 6, 2020 •

edited

Loading

morozov commented Sep 6, 2020

greg0ire commented Sep 6, 2020

morozov left a comment

beberlei commented Sep 9, 2020

beberlei commented Sep 9, 2020

morozov commented Sep 9, 2020

beberlei commented Sep 9, 2020

beberlei commented Sep 9, 2020

morozov commented Sep 9, 2020

morozov commented Sep 9, 2020

beberlei commented Sep 9, 2020

morozov commented Sep 9, 2020 •

edited

Loading

morozov commented Sep 9, 2020

greg0ire commented Sep 9, 2020

morozov commented Sep 12, 2020

[GH-4243] more sqlite schema manager fixes #4256

[GH-4243] more sqlite schema manager fixes #4256

Conversation

beberlei commented Sep 6, 2020

Summary

greg0ire commented Sep 6, 2020 • edited Loading

beberlei Sep 6, 2020

Choose a reason for hiding this comment

morozov Sep 6, 2020

Choose a reason for hiding this comment

greg0ire Sep 8, 2020

Choose a reason for hiding this comment

morozov Sep 8, 2020

Choose a reason for hiding this comment

greg0ire Sep 8, 2020

Choose a reason for hiding this comment

morozov Sep 8, 2020

Choose a reason for hiding this comment

beberlei commented Sep 6, 2020

greg0ire commented Sep 6, 2020 • edited Loading

morozov commented Sep 6, 2020 • edited Loading

morozov commented Sep 6, 2020

greg0ire commented Sep 6, 2020

morozov left a comment

Choose a reason for hiding this comment

beberlei commented Sep 9, 2020

beberlei commented Sep 9, 2020

morozov commented Sep 9, 2020

beberlei commented Sep 9, 2020

beberlei commented Sep 9, 2020

morozov commented Sep 9, 2020

morozov commented Sep 9, 2020

beberlei commented Sep 9, 2020

morozov commented Sep 9, 2020 • edited Loading

morozov commented Sep 9, 2020

greg0ire commented Sep 9, 2020

morozov commented Sep 12, 2020

greg0ire commented Sep 6, 2020 •

edited

Loading

greg0ire commented Sep 6, 2020 •

edited

Loading

morozov commented Sep 6, 2020 •

edited

Loading

morozov commented Sep 9, 2020 •

edited

Loading