-
Notifications
You must be signed in to change notification settings - Fork 10.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ensuring only single product creation with unique SKU for concurrent requests #47476
base: trunk
Are you sure you want to change the base?
Conversation
Test using WordPress PlaygroundThe changes in this pull request can be previewed and tested using a WordPress Playground instance. 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. |
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: |
* @param string $sku SKU. | ||
* @return bool | ||
*/ | ||
public function does_sku_exist( $sku ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
SELECT meta_id | ||
FROM $wpdb->postmeta | ||
WHERE meta_key = '_sku' AND meta_value = %s | ||
LIMIT 1 FOR UPDATE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is FOR UPDATE
necessary, probably a reminiscent of your testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Relevant slack conversation: p1715603112152869-slack-C0E1AV8T0 |
* @param string $sku Will be slashed to work around https://core.trac.wordpress.org/ticket/27421. | ||
* @return bool | ||
*/ | ||
public function is_existing_sku( $product_id, $sku ) { | ||
public function is_existing_sku( $sku ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
// Delete prodocts post | ||
$wpdb->query( "DELETE FROM $wpdb->posts WHERE ID IN ($ids_to_delete)" ); | ||
|
||
// Delete products post meta | ||
$wpdb->query( "DELETE FROM $wpdb->postmeta WHERE post_id IN ($ids_to_delete)" ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
f738017
to
4fb2d37
Compare
7ea50a5
to
fb8e369
Compare
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that this currently does not handle updates? I think we would need to add similar capability there too.
plugins/woocommerce/includes/data-stores/class-wc-product-data-store-cpt.php
Outdated
Show resolved
Hide resolved
|
||
$query = $wpdb->prepare( | ||
"INSERT INTO $wpdb->wc_product_meta_lookup (product_id, sku) | ||
SELECT %d, %s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch added it here: 55954d3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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? |
plugins/woocommerce/includes/data-stores/class-wc-product-data-store-cpt.php
Outdated
Show resolved
Hide resolved
plugins/woocommerce/includes/rest-api/Controllers/Version3/class-wc-rest-crud-controller.php
Outdated
Show resolved
Hide resolved
This sounds good to me! |
a4c4586
to
06bf729
Compare
235b221
to
7707b23
Compare
e1747ba
to
83fe620
Compare
@coreymckrill @vedanshujain, thank you for the feedback so far; I've addressed it all and also added a test. |
if ( ! empty( $sku ) && ! $this->obtain_lock_on_sku_for_concurrent_requests( $product ) ) { | ||
$product->delete( true ); | ||
// translators: 1: SKU, 2: Product Id. | ||
throw new Exception( esc_html( sprintf( __( 'The SKU (%1$s) you are trying to insert with Product Id (%2$s) is already under processing', 'woocommerce' ), $sku, $id ) ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being discussed here: p1717092314975919-slack-C0E1AV8T0
346a712
to
8b84562
Compare
8b84562
to
b89c15d
Compare
Submission Review Guidelines:
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:
woocommerce>settings>advanced>REST API
trunk
first, check thelog_error_counts.txt
file, note the outputlog_error_counts.txt
file the output should be 1, 1I've created this bash script to test the change
Changelog entry
Significance
Type
Message
Comment