magento2 icon indicating copy to clipboard operation
magento2 copied to clipboard

Published 20 hours ago • verified publisher icon magento

#36336 Prefix all Indexes and Constraints

Open lbajsarowicz opened this issue 2 years ago • 15 comments

Description (*)

Pull Request enforces Magento to prefix MySQL object names, not only hash. This change answers a need of having multiple objects inside of a single Table (e.g. Constraint and Index).

The change also simplifies the trimHash method to use hashes in a way known for Git commit hash (use the first 8 letters), instead of a method that... returns empty string.

Additionally, added the test coverage for "Just a hash" return.

Related Pull Requests

N/A

Fixed Issues (if relevant)

  1. Fixes magento/magento2#36336

Manual testing scenarios (*)

  1. Create two objects inside of the same table (db_schema.xml)
<table name="quote_item">
  <constraint xsi:type="unique" referenceId="ThisDoesNotMatter">
    <column name="quote_item_id"/>
  </constraint>
  <index referenceId="ThisDoesNotMatterToo" indexType="btree">
    <column name="quote_item_id"/>
  </index>
</table>
  1. Execute bin/magento setup:upgrade

Current Behavior

SQLSTATE[42000]: Syntax error or access violation: 1061 Duplicate key name 'XXX_QUOTE_ITEM_QUOTE_ITEM_ID', query was: ALTER TABLE `xxx_quote_item` ADD INDEX `XXX_QUOTE_ITEM_QUOTE_ITEM_ID` (`quote_item_id`)

Expected Behavior

Execution setup:upgrade should pass correctly.

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)
  • [ ] README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
  • [ ] All automated tests passed successfully (all builds are green)

lbajsarowicz avatar Sep 08 '23 12:09 lbajsarowicz

Hi @lbajsarowicz. Thank you for your contribution! Here are some useful tips on 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

:exclamation: Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s) For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:
  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests
  13. Semantic Version Checker

You can find more information about the builds here :information_source: Run only required test builds during development. Run all test builds before sending your pull request for review.

For more details, review the Code Contributions documentation. Join Magento Community Engineering Slack and ask your questions in #github channel.

m2-assistant[bot] avatar Sep 08 '23 12:09 m2-assistant[bot]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

magento-automated-testing[bot] avatar Sep 08 '23 12:09 magento-automated-testing[bot]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

magento-automated-testing[bot] avatar Sep 08 '23 13:09 magento-automated-testing[bot]

@magento run Unit Tests

lbajsarowicz avatar Sep 08 '23 13:09 lbajsarowicz

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

magento-automated-testing[bot] avatar Sep 08 '23 13:09 magento-automated-testing[bot]

@magento run all tests

By the way, I tried to run Unit Tests locally, but... ~5% of them fail with whitespace mismatch:

13) Magento\Framework\Stdlib\Test\Unit\DateTime\DateTimeFormatterTest::testFormatObjectNumericFormat with data set #4 (1, 'March 30, 2022 at 12:01:02 AM...n Time')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'March 30, 2022 at 12:01:02 AM Greenwich Mean Time'
+'March 30, 2022 at 12:01:02 AM Greenwich Mean Time'

/var/www/html/lib/internal/Magento/Framework/Stdlib/Test/Unit/DateTime/DateTimeFormatterTest.php:165

14) Magento\Framework\Stdlib\Test\Unit\DateTime\DateTimeFormatterTest::testFormatObjectNumericFormat with data set #5 (0, 'Wednesday, March 30, 2022 at ...n Time')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Wednesday, March 30, 2022 at 12:01:02 AM Greenwich Mean Time'
+'Wednesday, March 30, 2022 at 12:01:02 AM Greenwich Mean Time'

/var/www/html/lib/internal/Magento/Framework/Stdlib/Test/Unit/DateTime/DateTimeFormatterTest.php:165

15) Magento\Framework\Stdlib\Test\Unit\DateTime\TimezoneTest::testGetDatetimeFormat with data set #0 ('en_US', 3, 'M/d/yy h:mm a')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'M/d/yy h:mm a'
+'M/d/yy h:mm a'

lbajsarowicz avatar Sep 10 '23 23:09 lbajsarowicz

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

magento-automated-testing[bot] avatar Sep 10 '23 23:09 magento-automated-testing[bot]

@magento run all tests

lbajsarowicz avatar Oct 26 '23 21:10 lbajsarowicz

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

magento-automated-testing[bot] avatar Oct 26 '23 21:10 magento-automated-testing[bot]

@magento run all tests

engcom-Hotel avatar Sep 23 '24 05:09 engcom-Hotel

@magento run all tests

engcom-Dash avatar Sep 24 '24 09:09 engcom-Dash

Hello @lbajsarowicz,

Thanks for the contribution!

I'm getting an open search error with your PR code. When I revert it, it works as expected. Could you please check it?

To reproduce the issue, run the bin/magento set:up command.

Please see the screenshot below. image

engcom-Dash avatar Sep 25 '24 08:09 engcom-Dash

Addressing your feedback @engcom-Hotel

lbajsarowicz avatar Oct 02 '24 16:10 lbajsarowicz

@magento run all tests

lbajsarowicz avatar Oct 02 '24 16:10 lbajsarowicz

As discussed with @lbajsarowicz over Slack, he mentioned that he will address this comment.

engcom-Dash avatar Oct 21 '24 07:10 engcom-Dash

Closing this PR since it has not been updated for a period of 14 days. Please feel free to reopen it if needed.

engcom-Dash avatar Nov 04 '24 07:11 engcom-Dash

C'mon. People have work here, enough billable work to queue non-billable Test Coverage for later. 😠

lbajsarowicz avatar Nov 04 '24 08:11 lbajsarowicz

Hello @lbajsarowicz,

Thank you for your contributions.

I am reopening this PR. Please address this comment, as it is blocking us from proceeding further: #37975 (comment).

Thanks!

engcom-Dash avatar Nov 04 '24 16:11 engcom-Dash

Hi @lbajsarowicz. Thank you for your contribution! Here are some useful tips on how you can test your changes using Magento test environment. :exclamation: Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s) For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:
  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests
  13. Semantic Version Checker

You can find more information about the builds here :information_source: Run only required test builds during development. Run all test builds before sending your pull request for review.

For more details, review the Code Contributions documentation. Join Magento Community Engineering Slack and ask your questions in #github channel.

m2-assistant[bot] avatar Nov 04 '24 16:11 m2-assistant[bot]

Hello @lbajsarowicz,

Thank you for your contributions!

We are putting this PR on hold for now. Once the issue referenced in https://github.com/magento/magento2/pull/37975#issuecomment-2373370709 has been resolved, we’ll proceed with the next steps.

Thank you for your understanding!

engcom-Dash avatar Nov 13 '24 07:11 engcom-Dash

Hi @lbajsarowicz,

This is a reminder to please address this https://github.com/magento/magento2/pull/37975#issuecomment-2373370709 so that we can proceed further with this.

Thank you!

engcom-Dash avatar Nov 27 '24 12:11 engcom-Dash

Update Working on it.

lbajsarowicz avatar Nov 29 '24 16:11 lbajsarowicz

@magento run all tests

lbajsarowicz avatar Nov 29 '24 17:11 lbajsarowicz

@engcom-Dash Fixed. The root cause was unexpected prefix added to Table Names in \Magento\Framework\DB\Adapter\Pdo\Mysql::getTableName.

@magento run all tests

lbajsarowicz avatar Nov 30 '24 22:11 lbajsarowicz

@magneto run all tests

Update

  1. Database Compare are false negatives (we do change structure in this PR)
  2. Functional Tests B2B - clear false negative,
  3. Regenerated DB Whitelists.

lbajsarowicz avatar Dec 01 '24 16:12 lbajsarowicz

@magento run all tests

lbajsarowicz avatar Dec 01 '24 19:12 lbajsarowicz

@engcom-Dash Fixed. The root cause was unexpected prefix added to Table Names in \Magento\Framework\DB\Adapter\Pdo\Mysql::getTableName.

@magento run all tests

Thanks @lbajsarowicz, for the updates.

Moving it to "Pending Review" as new files are being added to the PR.

engcom-Dash avatar Dec 02 '24 09:12 engcom-Dash

There is one more issue with the Integration Tests. The DB whitelists would fail for non-CommunityEdition tables, as these need to be regenerated after my changes (their indexes and constraints would have the prefix)

lbajsarowicz avatar Dec 02 '24 09:12 lbajsarowicz

Hello @lbajsarowicz,

Since its related issue has been marked as a Feature Request, we need a PO approval to move forward for this. Please allow us some time for the same.

Till then we are moving this PR On Hold.

Thanks for the contribution.

engcom-Hotel avatar Dec 09 '24 10:12 engcom-Hotel

Hello @lbajsarowicz,

We have received a response from the PO, indicating that renaming database table columns, indexes, or constraints is considered a breaking change. This could disrupt extensions and custom scripts that reference the original names. To adopt such changes, customers would need to implement database migration scripts to maintain data integrity and consistency, which carries a risk of data loss.

Given these implications, I believe it would be best not to proceed with this PR.

However, we sincerely appreciate your valuable contributions.

Thanks

engcom-Hotel avatar Dec 10 '24 07:12 engcom-Hotel