ISSUE-30304 - Mark index removal as non-destructive so indexes can be removed without being present in whitelist json

[zaximus84]

Description (*)

Previous state: When an index is present in the database but not in declarative schema, it's funneled into \Magento\Framework\Setup\Declaration\Schema\Operations\DropElement for evaluation. DropElement is considered destructive, so it fails \Magento\Framework\Setup\Declaration\Schema\Diff\Diff::canBeRegistered unless the index is present in a db_schema_whitelist.json file. An index must be present in the whitelist json to be automatically removed during declarative schema processing.

After this PR: The index removal is funneled into a new DropIndex class instead. This is considered non-destructive, so it passes \Magento\Framework\Setup\Declaration\Schema\Diff\Diff::canBeRegistered. Indexes present in the database but not declarative schema are automatically removed, regardless of what's in db_schema_whitelist.json.

Fixed Issues (if relevant)

1. Fixes Indexes are not removed unless added to whitelist #30304

Manual testing scenarios (*)

1. Directly in MySQL, add an index to a column in a core table.
2. Run bin/magento setup:upgrade.
3. Confirm that the index was automatically removed because it doesn't exist in any declarative schema.

Questions or comments

I was uncertain if this needs tests written as there seem to be other schema operation classes that aren't tested. If it does need tests, I will need advice on what test class should be updated, and what the test should check for. I understand the concept of unit tests, but haven't written any before.

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)

[m2-assistant]

Hi @zaximus84. 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

❗ 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

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

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

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

[sidolov]

@zaximus84 changes look good to me! Could you please cover the solution with an automated test? Thanks!

[sidolov]

Please, make const private

[zaximus84]

@sidolov
I have a basic familiarity with unit tests, but I've never actually written one. Can you give any advice? A suggestion on what the test needs to test for and what class it should be in would help me get started. Thanks.

[sidolov]

@zaximus84 you may check our testing documentation: https://devdocs.magento.com/guides/v2.4/test/testing.html
In this case, it would be good to cover changes with an integration test or at least with unit.

[zaximus84]

@sidolov
It turns out there was already a unit test for the index management, and it began failing after my changes due to the new operation. I have updated it to reference the new operation name. I believe this is a sufficient test, but let me know if additional criteria need to be covered.

Regarding the constant, it can't be private as it's referenced in DiffManager.php, but I explicitly marked it with public visibility.

[sidolov]

@zaximus84 got it! Thanks for your contribution! We will proceed with the delivery of PR as soon as possible!

[magento-engcom-team]

Hi @sidolov, thank you for the review.
ENGCOM-8480 has been created to process this Pull Request
✳️ @sidolov, could you please add one of the following labels to the Pull Request?

Label	Description
Auto-Tests: Covered	All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered	Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required	Changes in Pull Request does not require coverage by auto-tests

[sidolov]

@magento run all tests

[engcom-Delta]

✔️ QA passed
Was checked cases:
Case 1:

• Directly in MySQL, add an index to a column in a core table.
• Run bin/magento setup:upgrade.

Result:
✔️ The index was automatically removed because it doesn't exist in any declarative schema.

Case 2:

• Add an index to a column in a core table using declarative schema( e.g. captcha_log table)

<index referenceId="CAPTCHA_LOG_VALUE" indexType="btree">
            <column name="value"/>
        </index>

• Run bin/magento setup:upgrade.
• Index was added into table
  
• Run bin/magento setup:upgrade again

Result:
✔️ The index is not removed because it exists in declarative schema.

[engcom-Delta]

Note: Functional Tests B2B and Integration Tests are failed

[engcom-Foxtrot]

Hello, @zaximus84. Thanks for your contribution. Can you please fix the failed integartion test?

[zaximus84]

I've spent quite a bit of time investigating the failing integration test, and I think I've hit a dead end determining how to resolve the issue. MODULE8_INSTALL_INDEX_TEMP is the first cause of failure, so I've been focusing on it. It's being added by the install script and removed by the upgrade script, so the final expected state in the schema xml for comparison is disabled="true". However, after the change in this PR, the automatic schema handling removes the index before the upgrade script for \Magento\Developer\Console\Command\SetupUpgradeTest. As a result, the dropIndex call in the upgrade script returns early and never registers the index removal, leaving the index in the final XML without being marked as disabled.

I think someone with more expertise in integration tests will need to pick this up.

[engcom-Foxtrot]

I've spent quite a bit of time investigating the failing integration test, and I think I've hit a dead end determining how to resolve the issue. MODULE8_INSTALL_INDEX_TEMP is the first cause of failure, so I've been focusing on it. It's being added by the install script and removed by the upgrade script, so the final expected state in the schema xml for comparison is disabled="true". However, after the change in this PR, the automatic schema handling removes the index before the upgrade script for \Magento\Developer\Console\Command\SetupUpgradeTest. As a result, the dropIndex call in the upgrade script returns early and never registers the index removal, leaving the index in the final XML without being marked as disabled.

I think someone with more expertise in integration tests will need to pick this up.

@zaximus84 thanks for investigating. I think I could try to resolve this issue.

[gabrieldagama]

@magento run all tests

[gabrieldagama]

Hi @zaximus84, thanks for contributing!

I believe we need to have some internal discussion on this issue, on my local tests the indexes were removed at the upgrade step of the integration tests, I believe this may be changing the behavior more than we would like.

I will put this on hold and start a discussion internally, will add comments with updates as soon as we decide how to proceed with it.

Thanks.

[ihor-sviziev]

@gabrieldagama, are there any updates after discussions?

[engcom-Hotel]

Hello,

We have tried to reproduce the issue in the latest development branch i.e. 2.4-develop and it seems the issue is still relevant.

We have followed the below steps in order to reproduce the issue:

1. Added an index in table catalog_log in the updated_at column:

2. Ran `bin/magento setup:upgrade` command.

3. Index has not been removed, it is there only.

Let me connect with its PO for further clarification on this.

Thanks

[engcom-Charlie]

As bot moved it from On Hold to Ready for Testing, moving it back.

Thank you!