Products loaded unnecessarily during rule validation

[vbuck]

When any integration uses the Magento rule system (even core sales rules), products may be loaded unnecessarily during rule validation.

Reference:  \Magento\Rule\Model\Condition\AbstractCondition::validate

Preconditions (*)

1. Any Magento OS/EE version >= 2.1 (reference), maybe even earlier
2. Any active sales rule using product-attribute based conditions
3. Any attribute of any type supporting and set to Values Required = No

Steps to reproduce (*)

1. Configure a sales rule according to the preconditions
2. For example, create a new attribute, test_attribute, type text, values required: no
3. Make sure to configure for use in promo rule conditions
4. Save the attribute (may require assignment to attribute set)
5. Create a new, active sales rule with coupon code (for ease of testing)
6. Configure attribute conditions as shown below and save the rule
7. Ensure sales rule index has been updated (this is assumed)

1. Go to the storefront
2. Add any item to the cart
3. At the cart, enter the coupon code (even a wrong code will work)
4. Probe method \Magento\Rule\Model\Condition\AbstractCondition::validate
5. Observe check for presence of test_attribute key on product object
6. Observe product is loaded from the DB

This illustrates the problem. To prove it another way:

1. Remove the item from your cart
2. In the admin panel, edit the same product
3. Set any value for Test Attribute (does not have to be "X")
4. Save the product
5. Verify any necessary index operations are completed
6. Return to the storefront
7. Add the same item to the cart
8. Again probe method \Magento\Rule\Model\Condition\AbstractCondition::validate
9. Observe product is not loaded at all

Expected result (*)

1. The product model is not loaded during rule validation when it holds no value for the target attribute of the current condition
2. The product model initialized and assigned to the quote item should contain all information necessary to complete rule validation

Actual result (*)

1. When using attribute-based conditions, the product may be loaded during rule validation

Additional Information

I discovered this issue in a vendor integration (Amasty Shipping Restrictions) when reviewing performance profiles. Curiously, Magento 1 does not behave this way (see \Mage_Rule_Model_Condition_Abstract::validate). I'm not a historian but given the refactor for M2, I'd suggest the developer was either actively solving for a known issue or else attempted a QoL improvement when translating the code.

I don't personally believe that rule validation should have any part in loading models. The requestor should be responsible for supplying all needed information to the validator. And if it isn't present, it should be treated as "not available," but not attempt to go load the value. I would even take that so far as to say the validate method should expect a more generic DataObject or plain DTO as a way to more strictly enforce this idea.

As an extra technical detail, the root cause for this situation is the attribute persister mechanism: \Magento\Eav\Model\ResourceModel\UpdateHandler::execute This acts as a self-cleaning mechanism to delete EAV rows that hold "empty-ish" values when saving products. While I agree with this approach, it requires the developer to be aware of its effects (which previously I was not!). For example, I would expect that every attribute assigned to a set has a property initialized (even if empty) on product models that are members of the attribute set. Therefore, DataObject::hasData should always return true for my attribute check; ie DataObject::hasData('test_attribute'). Or else, we redefine the meaning of "has data" (but that's another story).

My point is, the logic in \Magento\Rule\Model\Condition\AbstractCondition::validate seems misguided. I believe that a blind check for the value of the attribute here is sufficient. We don't need to know whether the property has been initialized. Let the mixed return type of getData and loose type comparison suffice for completing validation. That would be in agreement with the broad usability of the abstract condition validator.

Until this is addressed, large quotes, large catalogs, and shops with many active rules using attribute-based conditions are subject to major performance degradation at scale. And I'll show you what I mean by leaving a screenshot of one transaction trace captured in New Relic.

I am showing a 3rd party extension here but please don't close the ticket on account of it! Study the trace and you'll see it leads into the core. And I was also able to reproduce the problem without this vendor integration.

[m2-assistant[bot]]

Hi @vbuck. Thank you for your report. To help us process this issue please make sure that you provided the following information:

• [x] Summary of the issue
• [x] Information on your environment
• [x] Steps to reproduce
• [x] Expected and actual results

Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:

@magento give me 2.4-develop instance - upcoming 2.4.x release

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

@vbuck do you confirm that you were able to reproduce the issue on vanilla Magento instance following steps to reproduce?

• [ ] yes
• [x] no

---

• Join Magento Community Engineering Slack and ask your questions in #github channel.

[vbuck]

Following up on this to request attention.

Also, I checked "no" incorrectly to the reproduction steps. I was able to reproduce it on a vanilla 2.3 instance.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 14 days if no further activity occurs. Is this issue still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? Thank you for your contributions!