Skip to content
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

Move schema part to the index #6226

Merged
merged 1 commit into from
Nov 24, 2023

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Nov 21, 2023

In SQLite, creating an index in another schema is done by prepending the
index name with the schema, while the table name must be unprefixed.

See https://www.sqlite.org/lang_createindex.html

@greg0ire greg0ire force-pushed the index-other-schema-sqlite branch from f944006 to e7f76c2 Compare November 21, 2023 19:35
@greg0ire greg0ire changed the title Create failing tests for index creation in other schema for SQLite Move schema part to the table Nov 21, 2023
@greg0ire
Copy link
Member Author

This is prompted by doctrine/data-fixtures#452 , and now I think my PR looks so wrong that the issue must be somewhere else.

@greg0ire greg0ire closed this Nov 21, 2023
@greg0ire greg0ire deleted the index-other-schema-sqlite branch November 21, 2023 20:13
@greg0ire greg0ire restored the index-other-schema-sqlite branch November 21, 2023 20:18
@greg0ire greg0ire reopened this Nov 21, 2023
@greg0ire
Copy link
Member Author

Actually, reopening, because my tests don't involve the ORM and should really work. So maybe the fix is wrong, but I don't see how the tests are.

@greg0ire greg0ire changed the title Move schema part to the table Move schema part to the index Nov 21, 2023
@greg0ire greg0ire force-pushed the index-other-schema-sqlite branch from e7f76c2 to 41e155c Compare November 21, 2023 20:22
@greg0ire greg0ire marked this pull request as ready for review November 21, 2023 20:22
@greg0ire greg0ire force-pushed the index-other-schema-sqlite branch from 41e155c to 5be9228 Compare November 21, 2023 22:57
Comment on lines 783 to 784
'CREATE TABLE other_schema.foo (id INTEGER NOT NULL)',
'CREATE INDEX other_schema.IDX_D9A8AD0DBF396750 ON foo (id)',
Copy link
Member

Choose a reason for hiding this comment

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

I would not introduce this test. We do not care what the resulting SQL looks like as long as the functionality is covered by a test. Furthermore, this test is definitely not the source of truth about what the index name should be.

Copy link
Member

Choose a reason for hiding this comment

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

The problem is that the functional test would pass if the platform decided to create no index at all.

I agree about the generated name though. If we make assertions on the name if the index, we should explicitly declare a name for the index.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I remove the unit test and modify the functional test to assert that an index does exist? If the platform decides not to create it, then introspecting the table won't show the index.

Copy link
Member Author

@greg0ire greg0ire Nov 22, 2023

Choose a reason for hiding this comment

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

Actually, I stand corrected. This fails for some reason 🤯 :

        $onlineTable = $this->connection->createSchemaManager()->introspectTable('other.test_other_schema');
        self::assertTrue($onlineTable->hasIndex('id'));

Doctrine\DBAL\Schema\Exception\TableDoesNotExist: There is no table with name "other.test_other_schema" in the schema.

I think there must be another bug. Or maybe it's just that it's not supported. If I use a separate connection, it works.

Copy link
Member

Choose a reason for hiding this comment

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

If we need to ensure that the index exists, we can introspect the schema.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep I think I did exactly that:)

@greg0ire greg0ire force-pushed the index-other-schema-sqlite branch from 5be9228 to 719134e Compare November 22, 2023 07:19
@greg0ire greg0ire closed this Nov 22, 2023
@greg0ire greg0ire reopened this Nov 22, 2023
In SQLite, creating an index in another schema is done by prepending the
index name with the schema, while the table name must be unprefixed.
@greg0ire greg0ire force-pushed the index-other-schema-sqlite branch from 719134e to bc7afc1 Compare November 23, 2023 22:09
@greg0ire greg0ire added this to the 3.7.3 milestone Nov 24, 2023
@greg0ire greg0ire added the Bug label Nov 24, 2023
@greg0ire greg0ire merged commit ef0576b into doctrine:3.7.x Nov 24, 2023
90 checks passed
@greg0ire greg0ire deleted the index-other-schema-sqlite branch November 24, 2023 06:36
@derrabus
Copy link
Member

Thank you @greg0ire!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants