Skip to content
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

ProductRepository returns wrong product if SKU matches entity ID of another product #136

Closed
ktruehl opened this issue Apr 9, 2019 · 4 comments

Comments

2 participants
@ktruehl
Copy link

commented Apr 9, 2019

Hello again! I found another nice one.

In TechDivision\Import\Product\Repositories\ProductRepository in the method findOneBySku(), first the cache is checked to see if a product has already been loaded. The thing is that the cache is checked using the SKU as cache key, while storing product data uses the entity ID as cache key. You use references to make this possible. However, it might happen that the SKU [sku] of product A matches the entity ID of product B. Furthermore assume that product B is loaded before product A. In that case, if the method findOneBySku() is called with [sku] as parameter, the call $this->resolveReference($cacheKey) will return [sku], since no reference for product A exists yet. But than $this->cache[$this->resolveReference($cacheKey)] will return the product data for B, which has interesting (as in horrible) side-effects. For example, the URL rewrite update observer will try to insert the URL rewrite for A with the SKU instead of the entity ID as entity_pk. And since product B has already been processed, a rewrite with that primary key already exists. Boom!

Now, I created a patch, which checks whether the product data returned from the cache is actually the one we want. In I replaced

if ($this->isCached($sku)) {
    return $this->fromCache($sku);
}

with

if ($this->isCached($sku)) {
    $product = $this->fromCache($sku);
    if ($product[MemberNames::SKU] == $sku)
        return $product;
}

But in general I think that the whole reference mechanism for accessing cached items is pretty dangerous, since the assumption cannot be made that there are no conflicts between actual cache keys and references.

@wagnert

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

Hi, thanks for reporting👍🏻I’ll check this as soon as possible, but not before next week as i’m out of office till then. Hope this is ok! 🙂

@wagnert wagnert self-assigned this Apr 9, 2019

@wagnert wagnert added the bug label Apr 9, 2019

@ktruehl

This comment has been minimized.

Copy link
Author

commented Apr 9, 2019

No worries. The patch I use is simple enough and above all has no side-effects. So I'm good for now.

@wagnert wagnert pinned this issue Apr 15, 2019

@wagnert wagnert added this to Selected in M2IF May 3, 2019

@wagnert

This comment has been minimized.

Copy link
Member

commented May 3, 2019

@ktruehl Hi, i've tested it and figured out, that the referencing is not the problem in general, but resolving the references works recursive which leads to the problem. I'll check if we need the recursive resolving, if not i'll remove it with the next version.

@wagnert

This comment has been minimized.

Copy link
Member

commented May 4, 2019

Closed with version 3.3.0 + 2.4.0.

@wagnert wagnert closed this May 4, 2019

@wagnert wagnert moved this from Selected to Done in M2IF May 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.