Type Mismatch in Data Comparison when checking if data has changes

[balvesh]

Preconditions and environment

• 2.4.5-p5 (Applicable for all 2.4.5 versions)
  The type of the value retrieved from the database is always a string, even if the original value in the database is of type decimal/float. This results in incorrect detection of data changes, triggering unnecessary updates in the save function.

Steps to reproduce

Here are the step-by-step reproduction steps:

Step 1: Add a product to the cart

• Go to the website and add a product to the cart.

Step 2: Confirm entry creation and check tax values

• Confirm that an entry is created in the quote and quote_item tables.
• Check the values for tax_amount in both the quote and quote_item tables.

Step 3: Add breakpoints to specific functions

• Open the following files:
  a. vendor/magento/framework/Model/ResourceModel/Db/AbstractDb.php
  - Locate the save function and add a breakpoint.
  b. vendor/magento/framework/Model/AbstractModel.php
  - Find the setData function and add a breakpoint.

Step 4: Proceed to the cart page

• Navigate to the cart page.

Step 5: Trace requests and set breakpoints

• Observe requests related to quote_item or quote tax_amount during the setData function call.
• Set breakpoints around the collectTotals event.

Step 6: Compare DB and incoming request values

• Intercept the request in question.
• Compare the values in the database with the incoming request.
• Pay attention to the setData function in vendor/magento/framework/Model/AbstractModel.php.

Step 7: Understand the data difference condition

• In the setData function, look for the condition:

  if (!array_key_exists($key, $this->_data) || $this->_data[$key] !== $value) {

• Understand that this condition considers data to be different, especially when dealing with tax_amount where the DB value is a string and the incoming request value is a float.

Step 8: Execute the condition leading to changes

• Realize that the condition in step 7 sets _hasDataChanges to true.
• This triggers the logic in the save function of vendor/magento/framework/Model/ResourceModel/Db/AbstractDb.php.

Step 9: Save logic not executed

• Observe that the save logic does not go into the condition:

  if (!$this->isModified($object)) {
      $this->processNotModifiedSave($object);
      $this->commit();
      $object->setHasDataChanges(false);
      return $this;
  }

• This indicates that the save logic to avoid duplicate update is skipped due to the _hasDataChanges being set to true in the previous steps.

Expected result

A duplicate update call to DB should not be triggered if the data has not changed for the following function:

vendor/magento/framework/Model/ResourceModel/Db/AbstractDb.php
/**
 * Save object object data
 *
 * @param \Magento\Framework\Model\AbstractModel $object
 * @return $this
 * @SuppressWarnings(PHPMD.CyclomaticComplexity)
 * @throws \Exception
 * @throws AlreadyExistsException
 */
public function save(\Magento\Framework\Model\AbstractModel $object)
{
    if ($object->isDeleted()) {
        return $this->delete($object);
    }

    $this->beginTransaction();

    try {
        if (!$this->isModified($object)) {
            $this->processNotModifiedSave($object);
            $this->commit();
            $object->setHasDataChanges(false);
            return $this;
        }

if (!$this->isModified($object)) { should return true and the if condition logic shall be executed.

Result: No DB Update query should have run.

Actual result

if (!$this->isModified($object)) { returns false and MySQL update query is ran. Which should not be the case.

Additional information

This issue affects the accuracy of detecting data changes, resulting in unnecessary database updates, impacting performance and potentially causing data integrity concerns.

investigated the save( function in depth and how Magento check if the object data has changed or not.

vendor/magento/framework/Model/ResourceModel/Db/AbstractDb.php

/**
 * Save object object data
 *
 * @param \Magento\Framework\Model\AbstractModel $object
 * @return $this
 * @SuppressWarnings(PHPMD.CyclomaticComplexity)
 * @throws \Exception
 * @throws AlreadyExistsException
 */
public function save(\Magento\Framework\Model\AbstractModel $object)
{
    if ($object->isDeleted()) {
        return $this->delete($object);
    }

    $this->beginTransaction();

    try {
        if (!$this->isModified($object)) {
            $this->processNotModifiedSave($object);
            $this->commit();
            $object->setHasDataChanges(false);
            return $this;
        }

if (!$this->isModified($object)) { this part is crucial has this further calls the following:

This checks for the following function:

vendor/magento/framework/Model/AbstractModel.php

/**
 * Check if initial object data was changed.
 *
 * Initial data is coming to object constructor.
 * Flag value should be set up to true after any external data changes
 *
 * @return bool
 */
public function hasDataChanges()
{
    return $this->_hasDataChanges;
}

The value _hasDataChanges is set in the following function:

/**
     * Overwrite data in the object.
     *
     * The $key parameter can be string or array.
     * If $key is string, the attribute value will be overwritten by $value
     *
     * If $key is an array, it will overwrite all the data in the object.
     *
     * @param string|array $key
     * @param mixed $value
     * @return $this
     */
    public function setData($key, $value = null)
    {
        if ($key === (array)$key) {
            if ($this->_data !== $key) {
                $this->_hasDataChanges = true;
            }
            $this->_data = $key;
        } else {
            if (!array_key_exists($key, $this->_data) || $this->_data[$key] !== $value) {
                $keyValue = $this->_data[$key] ?? '';
                $valueData = $value ;
                $this->_hasDataChanges = true;
            }
            $this->_data[$key] = $value;
        }
        return $this;
    }

Looking closely at this line of code:

if (!array_key_exists($key, $this->_data) || $this->_data[$key] !== $value) {

• The following screenshot shows how the existing quote item tax amount saved in DB is 0.
• But when another save with 0type - float tax amount value is executed the data loaded from DB for that object has tax value as 0.0000 type - string.
• Therefore they do not match in their type that is the reason the value _hasDataChanges is set as true.
• And this happens for every save, causing multiple duplicate MySQL updates.

The type of value retrieved from DB will always be of type string. Even if the value set in the DB column is of type decimal/float.

Suppose we call -> setTaxAmount (2.99), setting to a float. Current tax amount is already 2.99 but it's returned via getData as string "2.9900" so Magento thinks data has changed, even though it's the same value.

This causes the following save function to always execute an update query to DB even though values have not changed actually:
public function save(\Magento\Framework\Model\AbstractModel $object)

Release note

No response

Triage and priority

• Severity: S0 - Affects critical data or functionality and leaves users without workaround.
• Severity: S1 - Affects critical data or functionality and forces users to employ a workaround.
• Severity: S2 - Affects non-critical data or functionality and forces users to employ a workaround.
• Severity: S3 - Affects non-critical data or functionality and does not force users to employ a workaround.
• Severity: S4 - Affects aesthetics, professional look and feel, “quality” or “usability”.

[m2-assistant]

Hi @balvesh. Thank you for your report.
To speed up processing of this issue, make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, Add a comment to the issue:

• @magento give me 2.4-develop instance - upcoming 2.4.x release
• For more details, review the Magento Contributor Assistant documentation.
• Add a comment to assign the issue: @magento I am working on this
• To learn more about issue processing workflow, refer to the Code Contributions.

---

Join Magento Community Engineering Slack and ask your questions in #github channel.
⚠️ According to the Magento Contribution requirements, all issues 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 issues happens in the queue order. If you want to speed up the delivery of your contribution, join the Community Contributions Triage session to discuss the appropriate ticket.

[m2-assistant]

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: 👇

• 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).
• 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue.
• 3. Add Area: XXXXX label to the ticket, indicating the functional areas 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.

[engcom-November]

Hello @balvesh,

Thank you for the report and collaboration!

Verified this issue on 2.4-develop.
In the function setData with condition if (!array_key_exists($key, $this->_data) || $this->_data[$key] !== $value), when tax_amount is passed, the data is retrieved in the form of string and value in DB is in decimal/float.
This makes the $this->_hasDataChanges = true and in turn causes the if (!$this->isModified($object)) condition false and skips the save logic.

Please take a look at the screenshot below:

Hence confirming the issue.

Thank you.

[github-jira-sync-bot]

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

[m2-assistant]

✅ 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.