magento2 icon indicating copy to clipboard operation
magento2 copied to clipboard

Missing unique key on eav_attribute_option_value table

Open tzyganu opened this issue 4 years ago • 17 comments

This is similar to #24581 but for the table eav_attribute_option_value

Summary (*)

The table eav_attribute_option_value does not have a unique index on the column pair (option_id, store_id).
There is no reason for an option to have 2 values for the same store so this should be restricted.

Also layered navigation disappears on frontend.

Relates issues: https://github.com/magento/magento2/issues/24581 https://github.com/magento/magento2/issues/24718

Examples (*)

Same as #24581, if by any faulty code an option gets 2 values for the same store view this causes a lot of issues when editing product / attributes / maybe others.

Proposed solution

add a unique key on the column pair (option_id, store_id) on the table eav_attribute_option_value

    <table name="eav_attribute_option_value">
       ......
        <constraint xsi:type="unique" referenceId="EAV_ATTRIBUTE_OPTION_VALUE_OPTION_ID_STORE_ID_UNIQUE">
            <column name="store_id"/>
            <column name="option_id"/>
        </constraint>
    </table>

tzyganu avatar Sep 25 '19 07:09 tzyganu

Hi @engcom-Charlie. Thank you for working on this issue. In order to make sure that issue has enough information and ready for development, please read and check the following instruction: :point_down:

  • [ ] 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).

    DetailsIf the issue has a valid description, the label Issue: Format is valid will be added to the issue automatically. Please, edit issue description if needed, until label Issue: Format is valid appears.

  • [ ] 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue. If the report is valid, add Issue: Clear Description label to the issue by yourself.

  • [ ] 3. Add Component: XXXXX label(s) to the ticket, indicating the components it may be related to.

  • [ ] 4. Verify that the issue is reproducible on 2.3-develop branch

    Details- Add the comment @magento give me 2.3-develop instance to deploy test instance on Magento infrastructure.
    - If the issue is reproducible on 2.3-develop branch, please, add the label Reproduced on 2.3.x.
    - If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!

  • [ ] 5. Add label Issue: Confirmed once verification is complete.

  • [ ] 6. Make sure that automatic system confirms that report has been added to the backlog.

m2-assistant[bot] avatar Sep 25 '19 07:09 m2-assistant[bot]

:white_check_mark: Confirmed by @engcom-Charlie Thank you for verifying the issue. Based on the provided information internal tickets MC-20408 were created

Issue Available: @engcom-Charlie, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.

magento-engcom-team avatar Sep 25 '19 07:09 magento-engcom-team

Hi @tzyganu. Thank you for your report. The issue has been fixed in magento/magento2#24720 by @UncleTioma in 2.3-develop branch Related commit(s):

The fix will be available with the upcoming 2.3.4 release.

magento-engcom-team avatar Oct 03 '19 12:10 magento-engcom-team

Reopening... either still missing or fix deleted.

When this is available in upcoming Release 2.3.4, why am I not seeing this fix in module version magento/module-eav 102.0.5 which is used in magento 2.3.5.-p1 ?

Where is it gone ?

stkrelax avatar Jun 12 '20 08:06 stkrelax

Fix for this issue was reverted in https://github.com/magento/magento2/commit/e1958fece28f2726d2d7cb6a48b1b25cb5b3862e

Explanation from @lenaorobei in https://github.com/magento/magento2/issues/28703#issuecomment-645357575

internal ticket says that it was reverted due to backward incompatible change that was not approved.

I re-opened this issue. Just checked - fix for this issue wasn't added to 2.3.4 and 2.3.5 releases.

ihor-sviziev avatar Jun 18 '20 09:06 ihor-sviziev

I don't understand where this is breaking BC, but anyway, the beauty of the declarative schema is that it does not matter if it's in the core or not.
If you have this problem, just add the constraint in one of your custom modules and it will work. And if this reaches the core it will still work even if it's duplicated in your module

        <constraint xsi:type="unique" referenceId="EAV_ATTRIBUTE_OPTION_VALUE_STORE_ID_OPTION_ID">
            <column name="store_id"/>
            <column name="option_id"/>
        </constraint>

tzyganu avatar Jun 18 '20 13:06 tzyganu

@tzyganu this is backward incompatible change in terms that some DBs could have duplicated data, and adding unique key might potentially fail.

Seems like there should be some migration that should be executed before adding this unique key, but not sure how to do that

ihor-sviziev avatar Jun 19 '20 07:06 ihor-sviziev

@ihor-sviziev Thanks for the explanation. You are right... from the abstract code point of view.
But if there are duplicate values nothing works. You will get errors when retrieving attribute options. You can try it and see. I think it is completely safe to fix this.

tzyganu avatar Jun 22 '20 07:06 tzyganu

@tzyganu anyway, I created PR with the same changes, so we'll see when someone from maintainers will approve / reject it.

ihor-sviziev avatar Jun 22 '20 08:06 ihor-sviziev

this is backward incompatible change in terms that some DBs could have duplicated data, and adding unique key might potentially fail.

@ihor-sviziev:

I encountered this just today actually, while writing a Data patch I had made an assumption that such a key would exist and therefore was performing $connection->insertOnDuplicate($table, $data, $fields). Due to the lack of the UNIQUE constraint however, duplicated rows of store_id and option_id combinations were created.

In any case, I'd also like to report that any store out there that does have such records in their database, already have a broken installation. As a collection object for the attribute's options will fail when \Magento\Framework\Data\Collection::addItem() is invoked on a secondary record with the same store_id and option_id. This is because the query for the collection:

SELECT `main_table`.*, `sort_alpha_value`.`value` 
FROM `eav_attribute_option` AS `main_table`  
LEFT JOIN `eav_attribute_option_value` AS `sort_alpha_value` 
  ON sort_alpha_value.option_id = main_table.option_id 
  AND sort_alpha_value.store_id = 0 
WHERE (`main_table`.`attribute_id` = 'REPLACE_THIS_WITH_ATTRIBUTE_ID_VALUE') 
ORDER BY
  main_table.sort_order ASC,
  sort_alpha_value.value ASC;

Will return two records per option_id in this scenario. Example:

+-----------+--------------+------------+-------------------+
| option_id | attribute_id | sort_order | value             |
+-----------+--------------+------------+-------------------+
|         5 |          872 |         10 | Black             |
|         5 |          872 |         10 | Black             |
+-----------+--------------+------------+-------------------+

Since option_id is the unique identifier for the collection's item model object, an exception is thrown.

So... Long story short... while I technically agree that adding such a UNIQUE constraint breaks backwards incompatibility... any installation with such data, would have had to gone through some wild extra lengths to have their site continue to work with such junk data.

@tzyganu thank you for the suggested fix, applying to my project in 2.4.0.

dfelton avatar Nov 09 '20 00:11 dfelton

@sidolov @gabrieldagama could we increase P3 -> P2, as we have basically the same issue for another table and it has higher priority - https://github.com/magento/magento2/issues/24581?

ihor-sviziev avatar Nov 19 '20 13:11 ihor-sviziev

@gabrieldagama as we discussed in Slack - this is really terrible issue, it's causing not obvious issues. In my case it was caused by some custom module, and behavior was like was "I just pressed some button and whole website went down", but real issue came from NOT OBVIOUS missing unique key.

We have 2 the same issues for two tables: https://github.com/magento/magento2/issues/24581 - P2 S2 (as for me this is right priority) https://github.com/magento/magento2/issues/24718 - P3 S3

Also for them we have open PRs: https://github.com/magento/magento2/pull/28796 (in "to approval" status since end of July) https://github.com/magento/magento2/pull/30981 (in review currently, waiting for test results, but I'm ready to approve it)

Would be good to move them forward together

BTW one issue was already fixed in October 2019, but then fix was reverted https://github.com/magento/magento2/issues/24581#issuecomment-537933794

ihor-sviziev avatar Nov 19 '20 14:11 ihor-sviziev

Hi @ihor-sviziev , it makes sense, I have changed the priority, thank you for the input!

sidolov avatar Nov 19 '20 15:11 sidolov

Hi @engcom-November. Thank you for working on this issue. In order to make sure that issue has enough information and ready for development, please read and check the following instruction: :point_down:

  • [ ] 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).

    DetailsIf the issue has a valid description, the label Issue: Format is valid will be added to the issue automatically. Please, edit issue description if needed, until label Issue: Format is valid appears.

  • [ ] 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue. If the report is valid, add Issue: Clear Description label to the issue by yourself.

  • [ ] 3. Add Component: XXXXX label(s) to the ticket, indicating the components it may be related to.

  • [ ] 4. Verify that the issue is reproducible on 2.4-develop branch

    Details- Add the comment @magento give me 2.4-develop instance to deploy test instance on Magento infrastructure.
    - If the issue is reproducible on 2.4-develop branch, please, add the label Reproduced on 2.4.x.
    - If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!

  • [ ] 5. Add label Issue: Confirmed once verification is complete.

  • [ ] 6. Make sure that automatic system confirms that report has been added to the backlog.

m2-assistant[bot] avatar Sep 22 '22 04:09 m2-assistant[bot]

Verified the issue on latest Magento 2.4-develop instance and the issue is reproducible. Hence confirming this issue. There is no restriction to add duplicate values for option_id. Adding duplicate values for option_id with same store_id is returning no error in eav_attribute_option_value DB table. Adding so is breaking Layered Navigation on Front end Search Results page. Hence confirming this issue. image Layered navigation disappeared on store front. image

engcom-November avatar Sep 22 '22 04:09 engcom-November

:white_check_mark: Jira issue https://jira.corp.adobe.com/browse/AC-6738 is successfully created for this GitHub issue.

github-jira-sync-bot avatar Sep 22 '22 04:09 github-jira-sync-bot

:white_check_mark: Confirmed by @engcom-November. Thank you for verifying the issue.
Issue Available: @engcom-November, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.

m2-assistant[bot] avatar Sep 22 '22 04:09 m2-assistant[bot]