Skip to content

fix: isWriteType() to recognize CTE; always excluding RETURNING - #8599

Merged
kenjis merged 8 commits into
codeigniter4:developfrom
markconnellypro:database-isWriteType
Mar 7, 2024
Merged

kenjis merged 8 commits into
codeigniter4:developfrom
markconnellypro:database-isWriteType

Conversation

@markconnellypro

@markconnellypro markconnellypro commented Mar 2, 2024

Copy link
Copy Markdown
Contributor

Description

This pull request attempts to fix some issues I ran into with queries being miscategorized as isWriteType or not, particularly with PostgreSQL:

  • Operations with CTE (flagged by WITH) were not being counted as isWriteType, even if followed by an appropriate operator like INSERT/UPDATE.
  • In Postgre, INSERT/UPDATE operations with RETURNING were being counted as isWriteType if there was whitespace before INSERT/UPDATE.
  • In Postgre, DELETE operations with RETURNING were always being counted as isWriteType.

Edit: this pull request has been modified to remove the Postgre override of isWriteType, and to incorporate RETURNING handling in isWriteType on a uniform basis for all database types.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Mar 2, 2024
@kenjis

kenjis commented Mar 2, 2024

Copy link
Copy Markdown
Member

Thank you!
Please fix coding style. Run composer cs-fix.

Comment thread tests/system/Database/Live/WriteTypeQueryTest.php
Comment thread tests/system/Database/Live/WriteTypeQueryTest.php Outdated
@kenjis

kenjis commented Mar 4, 2024

Copy link
Copy Markdown
Member

WriteTypeQueryTest is not a good test code to begin with. Do not use it as a reference.
One test method should not contain multiple assertions.
It becomes difficult to understand what you want to test.

@kenjis

kenjis commented Mar 4, 2024

Copy link
Copy Markdown
Member

@markconnellypro Can you add new test methods to test WITH clause on Postgre, instead of adding tests to the existing methods?

@kenjis kenjis added the database Issues or pull requests that affect the database layer label Mar 4, 2024
add testAssertTypeReturning function for which databases support RETURNING
@markconnellypro

markconnellypro commented Mar 4, 2024

Copy link
Copy Markdown
Contributor Author

Each test is now in its own function. The code for determining whether CodeIgniter supports RETURNING is now centralized in one function, so that any future expansions of functionality only need to be fixed in one location, instead of in 15. (To separate out Postgre, I would have had to add an additional 15 tests.)

Comment thread tests/system/Database/Live/WriteTypeQueryTest.php Outdated
@markconnellypro markconnellypro changed the title fix: isWriteType() to recognize CTE; [Postgre] allow beginning whitespace at query start, RETURNING with DELETE fix: isWriteType() to recognize CTE; always excluding RETURNING Mar 4, 2024
Omit RETURNING from isWriteType for all database types;
Modify tests for uniform handling for all database types
public function isWriteType($sql): bool
{
return (bool) preg_match('/^\s*"?(SET|INSERT|UPDATE|DELETE|REPLACE|CREATE|DROP|TRUNCATE|LOAD|COPY|ALTER|RENAME|GRANT|REVOKE|LOCK|UNLOCK|REINDEX|MERGE)\s/i', $sql);
return (bool) preg_match('/^\s*(WITH\s.+(\s|[)]))?"?(SET|INSERT|UPDATE|DELETE|REPLACE|CREATE|DROP|TRUNCATE|LOAD|COPY|ALTER|RENAME|GRANT|REVOKE|LOCK|UNLOCK|REINDEX|MERGE)\s(?!.*\sRETURNING\s)/is', $sql);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you know why "? is needed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't, but since it was already there, I left it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, it came from CI3.
https://github.com/bcit-ci/CodeIgniter/blob/70d0a0edbee5e5814aefddb77f67628e68de080f/system/database/DB_driver.php#L999
So it is not easy to remove it.

But I don't imagine a use case. And it seems there is no test case for it.

@kenjis kenjis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you! The test code is now much cleaner.

@kenjis
kenjis merged commit fd28a3f into codeigniter4:develop Mar 7, 2024
@kenjis

kenjis commented Mar 7, 2024

Copy link
Copy Markdown
Member

@markconnellypro Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Verified issues on the current code behavior or pull requests that will fix them database Issues or pull requests that affect the database layer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants