Allow calling getQuery() multiple times, and other improvements

[vlakoff]

Main fix:

• getQuery() can now be called multiple times without trouble, compileBinds() will be called only on original SQL string. Previously, calling getQuery() additional times was executing compileBinds() again and again, and on the already compiled SQL string…

Additional improvements and notes:

• [Performances] If getQuery() is called multiple times, compileBinds() is executed only the first time.
• Calling setQuery() or setBinds() discard finalQueryString, so they can be called multiple times with new data, and the SQL will be generated with this new data.
• swapPrefix() will always be executed on the original SQL string (with the bind placeholders), which is a bit safer than on the final SQL string (as the search/replace just operates on the whole SQL string).
• [BC] As previously, it is possible to call swapPrefix() multiple times, to execute multiple prefix swaps.
• swapPrefix() can be called before or after setBinds() (but not before setQuery(), as it operates on originalQueryString).
• debugToolbarDisplay() would fail if called before finalQueryString is assigned, as compileBinds() was relying on getQuery() to assign this property beforehand. (refs [Query] Do not recompile binds if already compiled #4534)

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

[vlakoff]

I added a handful of tests, as a first draft. Suggestions are welcome.

Maybe it could be polished. Maybe some tests are redundant, maybe some cases are still not covered (combinations)…

Note I couldn't (yet?) write a test where simple binds markers could be erroneously replaced twice, as the method Query::matchSimpleBinds() ensures avoiding bind markers in strings.

[vlakoff]

To summarize the approach of this PR:

I'm adding an optional, intermediary state "swappedQueryString". Starting from the original query string, two changes can be applied to this original string: swapping the db prefix, and compiling the binds. We have to always keep record of the original string, and we also need a state to know if the binds have been compiled (here "finalQueryString" fills this purpose, by being discarded when compiling has to be done again). We can't apply the swaps to "finalQueryString" as the property would no longer let us know when bind compiling is needed. And of course we should not apply the swaps to "originalQueryString", as its purpose is to keep the user-supplied string untouched. Thus, we need to record an additional state, which is "swappedQueryString" here.

An alternative would have been to store an array of prefix swaps to make (a list of "from → to" pairs), and apply them to the original string, before the binds, when compiling. But nevertheless this would make us record an additional state as well, and the "swappedQueryString" approach is simpler.

[sfadschm]

Please rebase and remove the following line:

CodeIgniter4/tests/system/Database/BaseQueryTest.php

Line 395 in 26e0025

$query->getQuery();

This not needed anymore with this change.

[vlakoff]

Rebased :)

[vlakoff]

On a related note, here is the behavior when setQuery() is called again:

• In all cases, the original query string is replaced, and the prefix swaps are discarded.
• If $binds is an array, it replaces the previous binds.
• If $binds is an empty array, it replaces the previous binds as well, which means the previous binds are deleted.
• If $binds is not provided or null, the previous binds are kept.

That handling of $binds is exactly the same as with the current code.

So, in the following example, we still have the "42" bind after the second line:

$query->setQuery('SELECT * FROM users WHERE id = ?', [42]);
$query->setQuery('SELECT * FROM users WHERE name = ?');

I don't know what would be the best to do when setQuery() is called again, with only $sql parameter. Should it just update the original query string and don't touch the binds, or should it also reset the binds? Anyway, if ever an change is wanted about this, it should be the subject of another PR.

[vlakoff]

The code itself should be ready. I'm just a bit unsure that the tests are perfect (for instance: remaining uncovered edge cases, and where I split hairs: redundant tests!). I should have a fresh look at these, and I'm open to reviews.

[paulbalandan]

This has been at draft status for couple of months now. Where are we on this? Or should this be closed?

[vlakoff]

getQuery(), as its name implies, is expected to be a getter, but currently, executing it several times triggers various errors.

I think it's a serious issue, therefore even if this PR is stale it shouldn't be closed.

Even if I noticed and fixed an error afterwards ("Fix swapPrefix() not effective when called after a first call to getQuery()"), the code should be quite stable.

Some alternative approaches I thought of:

• instead of the intermediate, optional string swappedQueryString, use an array to store an ordered list of "from/to" prefix replacements.
• discard the finalQueryString "cache" and just compile every time getQuery() is called.

Feel free to review the code (in particular to ensure the tests are clean!) or even to push changes.

[kenjis]

@vlakoff Do you mean this PR is completed and ready for review?
Or do you still have something to do?

[vlakoff]

I have squashed the commits.
Information provided in this PR should be quite exhaustive (and exhausting).
Test coverage seems to be pretty complete, all the cases I'm thinking of are covered.

Therefore, marking the PR for review.

[kenjis]

Why do you remove it?

[vlakoff]

This had been noticed previously, see above.

It's because getQuery() can now be called multiple times without error, I could remove the ugly hack here. That hack was in place so that getQuery() isn't called an additional time.

But… it required that getQuery() had been be called beforehand, so that finalQueryString is set, as compileBinds() needed it to be set beforehand… hence the "artificial" getQuery() here, just so that finalQueryString is set.

[kenjis]

I see.

[kenjis]

I think there is no use case that calls swapPrefix() more than once.

[vlakoff]

It is possible to swap prefix several times with the current code, so I made sure it is still possible with my new code, just in order to not introduce any regression.

Still, it should be worth covering this with a test, to ensure the behaviour remains consistent.

[kenjis]

Okay.

[kenjis]

I think the user guide should be updated. Just remove the "final" ?

swapPrefix()
Replaces one table prefix with another value in the final SQL. The first parameter is the original prefix that you want replaced, and the second parameter is the value you want it replaced with:
https://codeigniter4.github.io/CodeIgniter4/database/queries.html?highlight=swapprefix#working-with-query-objects

[kenjis]

@vlakoff You missed GPG sign.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/signing.md

[kenjis]

[BC] As previously, it is possible to call swapPrefix() multiple times, to execute multiple prefix swaps.

Why do you need to make it possible?
I think there is no use case.

[kenjis]

The branch coverage is 84.75%, good.

[cijagani]

@kenjis how to do this setup? Can you please guide me to do this? Which editor you are using?

[kenjis]

@cijagani The coverage report? It is a HTML file generated by PHPUnit.

$ php -d memory_limit=-1 vendor/bin/phpunit --path-coverage tests/system/Database/BaseQueryTest.php --coverage-html build/coverage

[cijagani]

@kenjis oh wow. thank you.

[vlakoff]

It has been a bit of a hassle to configure, but these commits are now signed.

[kenjis]

My final comment: #5127 (comment)

[vlakoff]

I think the user guide should be updated. Just remove the "final" ?

Done: 0839468

[kenjis]

Thank you!
LGTM

[kenjis]

@vlakoff @samsonasik Thank you!