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

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

Merged
merged 8 commits into from
Mar 7, 2024
Merged

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

merged 8 commits into from
Mar 7, 2024

Conversation

markconnellypro
Copy link
Contributor

@markconnellypro markconnellypro commented Mar 2, 2024

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
Copy link
Member

kenjis commented Mar 2, 2024

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

@kenjis
Copy link
Member

kenjis commented Mar 4, 2024

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
Copy link
Member

kenjis commented Mar 4, 2024

@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
Copy link
Contributor Author

markconnellypro commented Mar 4, 2024

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.)

@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
@@ -1657,7 +1657,7 @@ public function resetDataCache()
*/
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
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
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
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.

Copy link
Member

@kenjis kenjis left a comment

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
62 checks passed
@kenjis
Copy link
Member

kenjis commented Mar 7, 2024

@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