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 BaseBuilder setAlias() and RawSql use with key value pairs #6741

Merged
merged 13 commits into from
Oct 26, 2022

Conversation

sclubricants
Copy link
Member

@sclubricants sclubricants commented Oct 22, 2022

This fixes an issue when using an alias with setAlias() method. When using escape identifiers it was adding database prefix to alias. This was fixed.

Also, a small change to use of RawSql in constraints for updateBatch() and upsertBatch(). RawSql can be used for the entire expression or used as a key value pair. This is how I originally intended it but somehow it didn't get in correctly.

Can we expedite merging this? These fixes are needed for my other PRs.

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

Added $this->db->addTableAlias($alias) to setAlias(). This keeps track of the alias and prevents adding the database prefix when using $this->db->protectIdentifiers(). There was some issues with this.

Also made some fixes to updateBatch() and use of RawSql. Just a small fix that allows key value pairs with RawSql.
use RawSql with key value pairs on constraints.
@sclubricants sclubricants added 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 4.3 labels Oct 22, 2022
@sclubricants
Copy link
Member Author

The bug can be seen in UpdateTest testRawSqlConstraint()

Before I had to use the database prefix when defining the alias so that protectIdentifiers matched. This is no longer the case.

@sclubricants sclubricants requested a review from kenjis October 22, 2022 20:55
@sclubricants
Copy link
Member Author

@kenjis Can we expedite this fix?

@sclubricants sclubricants mentioned this pull request Oct 22, 2022
5 tasks
@kenjis
Copy link
Member

kenjis commented Oct 22, 2022

Fix use of setAlias()

Added $this->db->addTableAlias($alias) to setAlias(). This keeps track of the alias and prevents adding the database prefix when using $this->db->protectIdentifiers(). There was some issues with this.

Also made some fixes to updateBatch() and use of RawSql. Just a small fix that allows key value pairs with RawSql.
36f2432

Make only one change per commit if possible.
I would also like you to add test code to prove that the bug has been fixed.

When fixing a bug, it is recommended that you first write a test case that fails because of the bug.

kenjis
kenjis previously approved these changes Oct 23, 2022
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.

I have not looked at the details of the specific DB drivers, but the test code is okay and the test passes.

LGTM.

@sclubricants
Copy link
Member Author

When fixing a bug, it is recommended that you first write a test case that fails because of the bug.

This would be the difference of the test that I altered. I had hacked the bug in the test and now you see the test as it should be.

@sclubricants
Copy link
Member Author

sclubricants commented Oct 23, 2022

I think one more thing I should add is to throw an exception if you use an alias for upsert with Postgres and SQLite. They are always Identified by "excluded". This way a developer doesn't get lost trying to figure out why their alias is not working.

...DONE

These are forced by DB to always be referred to as "excluded". A custom alias cannot be set.
If the error is not thrown here it will through one on query execution.
@kenjis kenjis dismissed their stale review October 24, 2022 01:03

Please fix Exceptions for upsert with Postgres and SQLite.

@kenjis kenjis merged commit ef8646a into codeigniter4:4.3 Oct 26, 2022
@kenjis
Copy link
Member

kenjis commented Oct 26, 2022

@sclubricants Thank you!

@sclubricants sclubricants deleted the FixAliasBatch branch October 26, 2022 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.3 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.

3 participants