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

IBX-7149: Refactored content type-based indexing to rely on a dedicated strategy #296

Merged
merged 16 commits into from
Mar 20, 2024

Conversation

mateuszdebinski
Copy link
Contributor

@mateuszdebinski mateuszdebinski commented Nov 23, 2023

Question Answer
JIRA issue IBX-7149
Type bug
Target Ibexa version v4.5+
BC breaks no

Maintainer update:

Given how complex and error-prone content id batch generating logic is, the fix includes the first portion of refactoring, introducing internal, command-dedicated \Ibexa\Bundle\Core\Command\Indexer\ContentIdListGeneratorStrategyInterface which produces \Ibexa\Core\Search\Indexer\ContentIdBatchList result wrapper object containing iterable batches of content Ids (meaning content id list chunks/slices) and total count of content ids.

This is the first in a series of refactoring steps, making the command more stable, as reindexing is mission-critical part of content delivery system. The refactoring allows for better test coverage of crucial parts of the logic, aiming towards more single-responsibility-oriented approach.

QA

Sanity check of ibexa:reindex command for various argument lists of the command, including different iteration count (meaning really different batch sizes) on an instance with test fixtures data.
There are foremost 2 edge cases:

  • list of content items of a given content type, which exceeds the batch size (let's say are all 12 folders in the system indexed when using --content-type=folder option? - see that before the fix they weren't)
  • empty list of content items for given criteria (can be e.g. --subtree, but should be tested against --content-type as well)

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly (main for features, the oldest supported for bugs).
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review (ping @ibexa/engineering).

@mateuszdebinski mateuszdebinski added Bug Something isn't working Ready for review labels Nov 23, 2023
@mateuszdebinski mateuszdebinski requested a review from a team November 23, 2023 12:03
@mateuszdebinski mateuszdebinski self-assigned this Nov 23, 2023
phpstan-baseline.neon Outdated Show resolved Hide resolved
@mateuszdebinski mateuszdebinski force-pushed the IBX-7149_reindex_problem_with_content_type branch from 21bfd4d to cbbd512 Compare December 19, 2023 16:51
Copy link

sonarcloud bot commented Dec 19, 2023

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@alongosz alongosz requested a review from a team February 8, 2024 12:56
Copy link

sonarcloud bot commented Feb 9, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@alongosz
Copy link
Member

alongosz commented Feb 9, 2024

Update:
Resolved #296 (comment) via 4aef887 + be2af07 + tests & PHPStan alignment.

Introducing ContentIdBatchList result object which is iterable and wraps both the batch list and total count, so the strategy-implementing service itself is stateless and returns that object instead. It will make more sense once other parts of if-elseif-... expression are refactored into its corresponding strategies, but that's out of scope for the bugfix.

To achieve uniform handling of iterable arrays, Generators, and Traversable objects, the ReindexCommand::runParallelProcess method now accepts iterable<int, array<int>> $batchList and repacks it into a \Generator internally.

@alongosz alongosz changed the title IBX-7149: Changed fetchIterationFromContentList function to get all contents by content type by iteration IBX-7149: Refactored content type-based indexing to rely on a dedicated strategy Feb 9, 2024
Comment on lines +113 to +117
static function (SearchHit $searchHit): Content {
self::assertInstanceOf(Content::class, $searchHit->valueObject);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I'd leave the mapping function as is, and use self::assertContainsOnlyInstancesOf() to prevent test framework from increasing it's assertion count. But that is only a detail 😅

@mateuszdebinski mateuszdebinski force-pushed the IBX-7149_reindex_problem_with_content_type branch from 13fbe41 to 4419bbf Compare March 19, 2024 13:50
Copy link

sonarcloud bot commented Mar 19, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

@tomaszszopinski tomaszszopinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA approved on IbexaDXP 4.5 exp.

@alongosz alongosz merged commit 1fd57b2 into 4.5 Mar 20, 2024
21 checks passed
@alongosz alongosz deleted the IBX-7149_reindex_problem_with_content_type branch March 20, 2024 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working QA approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants