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.

Sign up for GitHub

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

Jump to bottom

Fix SQLite temp table name must not contain dot #6315

Merged
merged 1 commit into from
May 3, 2024
Merged

Fix SQLite temp table name must not contain dot #6315

merged 1 commit into from
May 3, 2024

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Feb 23, 2024 •
edited
Loading

Q A
Type bug
Fixed issues

Summary

Fix invalid SQL generated when table with schema is given. As it was invalid, this PR is fully BC in sense of the exact generated SQL.

@mvorisek mvorisek force-pushed the fix_empty_sqlite_dbname branch 2 times, most recently from 4aadd00 to 7a7c3ac Compare February 24, 2024 11:39
@mvorisek mvorisek changed the title Fix DB name passing in SqliteSchemaManager::listTableForeignKeys() Fix various SQLite explicit schema related issues Feb 24, 2024
@derrabus
Copy link
Member

You know the drill: no test, no merge. 🤗

@mvorisek
Copy link
Contributor Author

@derrabus how would you like me tested the 1st commit, if the DB name is currently unused?

@derrabus
Copy link
Member

You've filed this PR as a bugfix, so that first commit of yours must fix some kind of bug that you can reproduce in a functional test, right?

@mvorisek mvorisek marked this pull request as draft March 18, 2024 13:22
@mvorisek
Copy link
Contributor Author

I am splitting this PR into multiple ones. I will update this PR later.

@mvorisek mvorisek force-pushed the fix_empty_sqlite_dbname branch 2 times, most recently from a2db19f to d12624e Compare April 5, 2024 18:48
@mvorisek mvorisek changed the title Fix various SQLite explicit schema related issues Fix SQLite temp table name must not contain dot Apr 5, 2024
@mvorisek mvorisek force-pushed the fix_empty_sqlite_dbname branch 2 times, most recently from 2591f16 to 4ec0586 Compare April 5, 2024 18:53
@mvorisek mvorisek marked this pull request as ready for review April 5, 2024 19:15
@mvorisek
Copy link
Contributor Author

mvorisek commented Apr 6, 2024 •
edited
Loading

This PR is done and has been reworked to contain only a single change with a test.

@greg0ire
Copy link
Member

greg0ire commented Apr 6, 2024

As per the testing guidelines, this change must be covered by an integration test:

Integration (a.k.a. functional) tests are required when the behavior under test is dictated by the logic defined outside of the DBAL. It could be:

  • The underlying database platform.
  • The underlying database driver.
  • SQL syntax and the standard as such.

@mvorisek
Copy link
Contributor Author

mvorisek commented Apr 6, 2024

@greg0ire functional test added.

@greg0ire
Copy link
Member

greg0ire commented Apr 6, 2024

Great. Now please improve your commit message. One shouldn't have to read the code to understand what you're talking about.

Build temporary table name from unqualified table name

In some situations, SqlitePlatform::getAlterTableSQL() relies on
a temporary table to store data from the original table while it
gets recreated. The name of this temporary table is built by
concatenating a prefix (__temp__) with the name of the original table.
When the original table name is foo.bar, there is an issue because
then the temporary table name is becomes __temp__foo.bar, and
__temp__foo does not exist. Let us use the unqualified table name
so that the temporary table becomes __temp__bar.

@mvorisek
Build temporary table name for SQLite from unqualified table name
87289d4 
In some situations, SqlitePlatform::getAlterTableSQL() relies on
a temporary table to store data from the original table while it
gets recreated. The name of this temporary table is built by
concatenating a prefix (__temp__) with the name of the original table.
When the original table name is foo.bar, there is an issue because
then the temporary table name becomes __temp__foo.bar which implies schema.table. Use the unqualified table name
so that the temporary table becomes __temp__bar.
@mvorisek
Copy link
Contributor Author

mvorisek commented Apr 6, 2024

Thank you for reviewing this PR. Commit message amended.

@greg0ire greg0ire requested a review from derrabus April 6, 2024 20:32
@mvorisek
Copy link
Contributor Author

Two approving reviews, can this PR be merged?

@greg0ire
Copy link
Member

greg0ire commented Apr 10, 2024 •
edited
Loading

I'd love to get a review from Alexander before we do that.

@mvorisek
Copy link
Contributor Author

mvorisek commented May 2, 2024

Hi @derrabus, can I ask you for your approval too?

@derrabus
Copy link
Member

derrabus commented May 3, 2024

Shouldn't the temporary data table be created in the same schema as the table we want to alter? Right now we cut off the schema and always create the table in the main schema which feels like we're shoveling data between SQLite files for no reason.

@mvorisek
Copy link
Contributor Author

mvorisek commented May 3, 2024

Shouldn't the temporary data table be created in the same schema as the table we want to alter? Right now we cut off the schema and always create the table in the main schema which feels like we're shoveling data between SQLite files for no reason.

see https://sqlite.org/forum/forumpost/7b70c81a35488424

In https://dbfiddle.uk/CXMRsuHM repro I have confirmed it - CREATE TEMPORARY TABLE always creates temporary table in temp schema, it makes sense, as the temporary table is created in memory and never stored anywhere.

@derrabus
Copy link
Member

derrabus commented May 3, 2024

CREATE TEMPORARY TABLE always creates temporary table in temp schema

TIL, thanks for clarifying. 🙂

@derrabus derrabus added this to the 3.8.5 milestone May 3, 2024
@derrabus derrabus merged commit 008a802 into doctrine:3.8.x May 3, 2024
92 checks passed
@mvorisek mvorisek deleted the fix_empty_sqlite_dbname branch May 3, 2024 07:54
derrabus added a commit that referenced this pull request May 16, 2024
@derrabus
Merge branch '4.1.x' into 5.0.x
a96c52c 
* 4.1.x:
  Fix example for QB delete and update in doc block
  Bump doctrine/.github from 5.0.0 to 5.0.1 (#6391)
  PHPStan 1.10.67, PHPUnit 9.6.19, PHPCS 3.9.2 (#6387)
  Fix "Plugin 'mysql_native_password' is not loaded" (#6388)
  Run tests against MySQL 8.4 (#6386)
  Use 3.8.x as a target for dependabot version updates (#6384)
  Setup dependabot
  Fix SQLiteSchemaManagerTest case
  Switch to substr implementation
  fix merge
  Bump workflow actions
  Fix SQL Server "extended property" SQL generation (#6353)
  Fix DB name passing in SqliteSchemaManager::listTableForeignKeys() (#6338)
  Fix SQLite temp table name must not contain dot (#6315)
  Provide CODECOV_TOKEN
  Upgrade to codecov/codecov-action v4
  Set fail_ci_if_error flag to true
  Specify the minor version number
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derrabus derrabus derrabus approved these changes

@SenseException SenseException SenseException approved these changes

@greg0ire greg0ire greg0ire approved these changes

Assignees
No one assigned
Labels
Bug Schema Management SQLite
Projects
None yet
Milestone
3.8.5
Development

Successfully merging this pull request may close these issues.
4 participants
@mvorisek @derrabus @greg0ire @SenseException