Ensuring only single product creation with unique SKU for concurrent requests

[naman03malhotra]

Submission Review Guidelines:

• I have followed the WooCommerce Contributing Guidelines and the WordPress Coding Standards.
• I have checked to ensure there aren't other open Pull Requests for the same update/change.
• I have reviewed my code for security best practices.
• Following the above guidelines will result in quick merges and clear and detailed feedback when appropriate.

Changes proposed in this Pull Request:

Closes #27295

Added a method in the V2 product controller that check unique SKUs in the post meta before going ahead and creating the product.

Before (on trunk)

After

How to test the changes in this Pull Request:

1. Copy the following script locally.
2. Now enter your credentials, you can get them from woocommerce>settings>advanced>REST API
3. Create new API keys if you don't have one already.
4. Add the CK and CS in the respective placeholders.
5. Replace the URL to the endpoint
6. Change the initial product SKU to something which is already not present on the table
7. Now run the script in trunk first, check the log_error_counts.txt file, note the output
8. Check the products page in WC admin, you'll see multiple products created with the same SKU.
9. Run the script in this branch, check the log_error_counts.txt file the output should be 1, 1
10. Check the products page in WC admin, you'll see only one product created with the same SKU.

I've created this bash script to test the change

#!/bin/bash

# Define the URL and authentication
URL="https://d.w.test/wp-json/wc/v3/products/batch"
CK="ck_7b87bfb00d141081ea0b2be4c443b939ac60677e"
CS="cs_0fbab889a09db5d83af5f2096ec553cf011c47c0"
CONTENT_TYPE="Content-Type: application/json"


INITIAL_SKU=4048

ROUNDS=5

CONCURRENT_REQUESTS=10

echo "Sending $ROUNDS rounds of $CONCURRENT_REQUESTS concurrent requests..."

count_errors() {
    local round=$1
    local error_count=0

    for i in $(seq 1 $CONCURRENT_REQUESTS); do
        if grep -q "error" "log_${round}_${i}.txt"; then
            error_count=$((error_count + 1))
        fi
    done

    echo $((CONCURRENT_REQUESTS - $error_count))
}

for j in $(seq 1 $ROUNDS); do
    SKU=$(($INITIAL_SKU + $j - 1)) 
    echo "Round $j with SKU $SKU..."

    for i in $(seq 1 $CONCURRENT_REQUESTS); do
        JSON_DATA=$(cat <<EOF
{
    "create": [
        {
            "name": "${j}-${i} New Test Product",
            "type": "simple",
            "sku": "${SKU}"
        }
    ]
}
EOF
)
        curl -X POST $URL \
            -u "$CK:$CS" \
            -H "$CONTENT_TYPE" \
            -d "$JSON_DATA" -k > "log_${j}_${i}.txt" 2>&1 &
    done

    wait
    echo "Completed round $j."

    error_count=$(count_errors $j)
    echo "Total products created in round $j with sku $SKU: $error_count" >> log_error_counts.txt
done

echo "All requests sent. Check log files for output."

Changelog entry

• Automatically create a changelog entry from the details below.

Significance

• Patch
• Minor
• Major

Type

• Fix - Fixes an existing bug
• Add - Adds functionality
• Update - Update existing functionality
• Dev - Development related task
• Tweak - A minor adjustment to the codebase
• Performance - Address performance issues
• Enhancement - Improvement to existing functionality

Message

Comment

[github-actions]

Test using WordPress Playground

The changes in this pull request can be previewed and tested using a WordPress Playground instance.
WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Test this pull request with WordPress Playground.

Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit.

[github-actions]

Hi @coreymckrill, @vedanshujain,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

[vedanshujain]

We should replace the method is_existing_sku in the data store CPT, instead of adding a new method with same functionality in a controller.

[vedanshujain]

is FOR UPDATE necessary, probably a reminiscent of your testing

[naman03malhotra]

Thanks for the review, it makes sense, I will update the existing method, moving the query from lookup table to postmeta did help, but still duplicate products are being created, which is very annoying tbh, and I do feel the people commenting in the issue.

So to solve this, I created a cleanup function to cleanup duplicate products whenever we encounter a duplicate SKU error during concurrent requests, I've tested it multiple time and it works well.

I plan to move it behind the action scheduler to keep things async, but let me know what you folks think about this.

[naman03malhotra]

Relevant slack conversation: p1715603112152869-slack-C0E1AV8T0

[coreymckrill]

Removing $product_id from the method signature seems like it would break backcompat, since it is public. Two alternatives I can think of off the top:

• Rather than removing, we could change the name of the param to $deprecated and just not use it in the method
• Create a new method, and then deprecate this one and have it call the new method

[coreymckrill]

Is it safe here to do the product deletions with raw SQL? This will bypass actions/filters that run during normal programmatic production deletion, such as woocommerce_pre_delete_product and woocommerce_delete_product.

[naman03malhotra]

@coreymckrill @vedanshujain, this PR is ready to have another look with a changed approach, so far it does the job. I am yet to write a test for it.

[vedanshujain]

I see that this currently does not handle updates? I think we would need to add similar capability there too.

[vedanshujain]

Note that many hosts won't allow a select without specifying a table name, (for example when DB clustering is being used). So this needs a FROM statement for any existing table.

Maybe we can change this line to something like

SELECT %d, %s FROM $wpdb->wc_product_meta_lookup

[naman03malhotra]

Good catch added it here: 55954d3

[naman03malhotra]

SELECT %d, %s FROM $wpdb->wc_product_meta_lookup

used $wpdb->options instead of this, as tests were failing as $wpdb->wc_product_meta_lookup was empty

[naman03malhotra]

I see that this currently does not handle updates? I think we would need to add similar capability there too.

Good idea, though I suggest that we roll out this enhancement first and see how it fairs in the real world and if everything looks good, we can also push it for updates, wdyt?

[coreymckrill]

I suggest that we roll out this enhancement first and see how it fairs in the real world and if everything looks good, we can also push it for updates

This sounds good to me!

[naman03malhotra]

@coreymckrill @vedanshujain, thank you for the feedback so far; I've addressed it all and also added a test.

[coreymckrill]

Sorry I didn't think of this earlier, but it occurs to me that throwing an exception in this existing public method might be breaking backcompat, because other codebases that use the method wouldn't be wrapping it in a try/catch (thus risking an "uncaught exception" fatal error). In fact, there is at least one other place in this codebase besides the REST CRUD controller that is using it that this PR doesn't update with a try/catch (example).

I don't have a better idea off the top of my head for passing the result of the SKU lock check on to the caller, but I do think it's worth considering the risks here. Wdyt?

[naman03malhotra]

Being discussed here: p1717092314975919-slack-C0E1AV8T0