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

Ranged selects always miss the last range #12624

Merged
merged 1 commit into from
Feb 1, 2018

Conversation

AntonEvers
Copy link
Contributor

I found this bug because some of my categories were empty. catalog_category_product_index did not contain all category product relations that are in catalog_category_product. Per category type (anchor, non-anchor) the highest category id's were missing from the index.

Description

Ranged selects always miss the last range:

// 2.1-develop:
        $this->isValid = ($this->batchSize + $this->currentOffset) < $this->totalItemCount;

// 2.2-develop:
        $this->isValid = ($this->batchSize + $this->currentOffset) <= $this->totalItemCount;

Suppose you have a total item count of 530 items:
Batch size: 500
Offset: 0
(batch size + offset) = 500
500 <= 530 is TRUE, so generate a query with offset 0 and limit 500
Returns item 1 - 500

Batch size: 500
Offset: 500
(batch size + offset) = 1000
1000 <= 530 is FALSE, so DO NOT generate a query with offset 500 and limit 500
Query is not created
Does not return items 501 - 530

The actual check should be:

        $this->isValid = $this->currentOffset < $this->totalItemCount;

Suppose you have a total item count of 530 items:
Batch size: 500
Offset: 0
0 <= 530 is TRUE, so generate a query with offset 0 and limit 500
Returns item 1 - 500

Batch size: 500
Offset: 500
500 <= 530 is TRUE, so generate a query with offset 500 and limit 500
Returns item 500 - 530

Batch size: 500
Offset: 1000
1000 <= 530 is FALSE, so DO NOT generate a query with offset 1000 and limit 500
Query is not created

Fixed Issues (if relevant)

Manual testing scenarios

  1. catalog_category_product_index should contain all category product relations that are also in catalog_category_product.

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@osrecio
Copy link
Member

osrecio commented Dec 11, 2017

Hi @ajpevers ,
thanks for your pull request! Please can you have a look at the failing unit test?

@osrecio osrecio self-assigned this Dec 11, 2017
@AntonEvers
Copy link
Contributor Author

AntonEvers commented Dec 12, 2017

@osrecio fixed it.

@osrecio
Copy link
Member

osrecio commented Dec 12, 2017

Thanks @ajpevers . I will make some tests, because I see a before commit from core team and I will check why was introduced.

Meantime, can you squash your commits to have only one?. If you don't know how, feel free to contact with me and I will help you.

Fix unit test. 11 batches expected to handle 105 items:

1: 1-10
2: 11-20
3: 21-30
4: 31-40
5: 41-50
6: 51-60
7: 61-70
8: 71-80
9: 81-90
10: 91-100
11: 101-105
@AntonEvers
Copy link
Contributor Author

@osrecio done, sorry for the delay.

@osrecio
Copy link
Member

osrecio commented Jan 11, 2018

No problem, until this week I could'nt work for process PR. 👍 Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants