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 SQL query quoting/casting when type is passed to where function #27980

Merged
merged 5 commits into from
Sep 23, 2020

Conversation

tmotyl
Copy link
Contributor

@tmotyl tmotyl commented Apr 24, 2020

Framework/DB/Select where function doesn't handle the "type" correctly.

Description (*)

The $type variable can be both string or int, so before comparing it to
'TYPE_CONDITION' string it has to be casted to avoid comparing integer zero
with string (0 == 'TYPE_CONDITION') which will wrongly return true,
and remove the information about type.

Pass type provided to where function down the chain to allow automatic
casting of arrays of values e.g. to int.

This fixes following cases:
1)
$select-->where('attr_table.store_id IN (?)', $storeIds, Zend_Db::INT_TYPE);
2)
$select-->where('attr_table.store_id = ?', $storeId, Zend_Db::INT_TYPE);
In both cases now passed value is correctly casted to int
(either single value, or each value from array)

Related Pull Requests

Fixed Issues (if relevant)

  1. magento/magento2#<issue_number>: Issue title

Manual testing scenarios (*)

  1. Make custom select like
    $select->from(['catalog_product_entity'], '*')->where('entity_id in (?)', ['1', 2, 3], \Zend_Db::INT_TYPE);
  2. Check sql
    $select->__toString()

Expected result (*)

SELECT catalog_product_entity.* FROM catalog_product_entity WHERE (entity_id in (1, 2, 3));

Actual result (*)

SELECT catalog_product_entity.* FROM catalog_product_entity WHERE (entity_id in ('1', 2, 3));

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

Resolved issues:

  1. resolves [Issue] Fix SQL query quoting/casting when type is passed to where function #29590: Fix SQL query quoting/casting when type is passed to where function

@m2-assistant
Copy link

m2-assistant bot commented Apr 24, 2020

Hi @tmotyl. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@tmotyl
Copy link
Contributor Author

tmotyl commented Apr 27, 2020

The test failures are unrelated to the change "MySQL server has gone away". Can somebody restart the tests?

@ihor-sviziev
Copy link
Contributor

Related to #27129 as it also partically doing the same.

@ihor-sviziev ihor-sviziev added the Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests label May 28, 2020
@ihor-sviziev
Copy link
Contributor

@magento run all tests

@tmotyl
Copy link
Contributor Author

tmotyl commented May 28, 2020

Related to #27129 as it also partically doing the same.

yes, I believe this patch is prerequisite to the other, as without it types for array are not interpreted correctly

@ihor-sviziev
Copy link
Contributor

@magento run all tests

@tmotyl
Copy link
Contributor Author

tmotyl commented May 29, 2020

hmm these test failures look unrelated to the change

Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

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

Hi @tmotyl,
Please fix static tests. All others are passing now

@tmotyl
Copy link
Contributor Author

tmotyl commented Jun 3, 2020

@ihor-sviziev thanks for the review. The static tests are failing that the line

     * @param array|null|int|string|float|\Magento\Framework\DB\Sql\Expression|\Magento\Framework\DB\Select|\DateTimeInterface $value The value to quote.

is too long.
I can either add a use statement on top of the file and shorter the types in doc comment, or add //phpcs:ignore
Whatever your prefer.

@ihor-sviziev
Copy link
Contributor

@tmotyl I think mixed there will be just more suitable, as potentially any object with '__toString()" method could be passed there. Is it make sense?

@tmotyl
Copy link
Contributor Author

tmotyl commented Jun 3, 2020

I can put mixed as it was before.
I just think mixed is not helpful neither for humans nor for static code analysis tools.
All these types have special handling somewhere in the function (or parent function) this is why
I think its beneficial to have them specified explicitly.
Otherwise developer who want to pass value to ->where don't know what type can be passed into and have to dig through multiple levels of code until he finds it.

Please make the decision, I will follow it. I do not want to spend too much time on secondary topic in this patch.

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Jun 3, 2020

@tmotyl ok, make sense. Could you try to import Expression and Select classes? I think it should fit the limit. If it will not fit - please use phpcs:ignore for specific lines

The $type variable can be both string or int, so before comparing it to
'TYPE_CONDITION' string it has to be casted to avoid comparing integer zero
with string (0 == 'TYPE_CONDITION') which will wrongly return true,
and remove the information about type.

Pass type provided to where function down the chain to allow automatic
casting of arrays of values e.g. to int.

This fixes following cases:
1)
->where('attr_table.store_id IN (?)', $storeIds, Zend_Db::INT_TYPE);
2)
->where('attr_table.store_id = ?', $storeId, Zend_Db::INT_TYPE);
In both cases now passed value is correctly casted to int
(either single value, or each value from array)

Co-authored-by: Ihor Sviziev <ihor-sviziev@users.noreply.github.com>
@tmotyl
Copy link
Contributor Author

tmotyl commented Jun 3, 2020

thanks, done

@ihor-sviziev
Copy link
Contributor

@magento run Static Tests

@engcom-Charlie
Copy link
Contributor

✔️ QA Passed
Checked by the following steps, the results are the same:

  1. Make custom select like
    $select->from(['catalog_product_entity'], '*')->where('entity_id in (?)', ['1', 2, 3], \Zend_Db::INT_TYPE);
  2. Check sql
    $select->__toString()

before ✖️:
SELECT catalog_product_entity.* FROM catalog_product_entity WHERE (entity_id in ('1', 2, 3));

after ✔️:
SELECT catalog_product_entity.* FROM catalog_product_entity WHERE (entity_id in (1, 2, 3));

@engcom-Charlie engcom-Charlie added the QA: Ready to add to Regression Scope Should be analyzed and added to Regression Testing Scope(if applicable) label Sep 1, 2020
@ihor-sviziev
Copy link
Contributor

Would like to add that basically the same changes were already added as part of #27129, so this PR will introduce phpdoc cleanup only.

@engcom-Alfa engcom-Alfa self-assigned this Sep 17, 2020
@engcom-Alfa engcom-Alfa added QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope and removed QA: Ready to add to Regression Scope Should be analyzed and added to Regression Testing Scope(if applicable) labels Sep 17, 2020
@engcom-Charlie
Copy link
Contributor

@magento run all tests

@engcom-Charlie
Copy link
Contributor

@magento run all tests

@gabrieldagama gabrieldagama added Priority: P3 May be fixed according to the position in the backlog. Severity: S4 Affects aesthetics, professional look and feel, “quality” or “usability”. and removed Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Severity: S2 Major restrictions or short-term circumventions are required until a fix is available. labels Sep 22, 2020
@magento-engcom-team magento-engcom-team merged commit 7948822 into magento:2.4-develop Sep 23, 2020
@m2-assistant
Copy link

m2-assistant bot commented Sep 23, 2020

Hi @tmotyl, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Component: DB improvement Priority: P3 May be fixed according to the position in the backlog. Progress: accept QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope Release Line: 2.4 Severity: S4 Affects aesthetics, professional look and feel, “quality” or “usability”.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Issue] Fix SQL query quoting/casting when type is passed to where function
7 participants