IBX-5385: Add option content-type to reindex command #370

papcio122 · 2023-04-24T09:32:02Z

Question	Answer
JIRA issue	IBX-5385
Type	bug/improvement
Target Ibexa version	`v3.3`+
BC breaks	no

Checklist:

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

eZ/Bundle/EzPublishCoreBundle/Command/ReindexCommand.php

Changed from VALUE_OPTIONAL to VALUE_REQUIRED for content-type option Co-authored-by: Paweł Niedzielski <[email protected]>

eZ/Publish/SPI/Search/Content/IndexerGateway.php

Steveb-p · 2023-04-24T10:23:02Z

eZ/Publish/SPI/Search/Content/IndexerGateway.php

@@ -40,6 +40,18 @@ public function getContentInSubtree(string $locationPath, int $iterationCount):
     */
    public function countContentInSubtree(string $locationPath): int;

+    /**
+     * @throws \Doctrine\DBAL\Exception


As far as the indexing is concerned, I'm not sure if we want to declare that this can throw this exception. The fact that we are using Doctrine is irrelevant for indexing (at least for the interface). @alongosz ?

As far as the indexing is concerned, I'm not sure if we want to declare that this can throw this exception. The fact that we are using Doctrine is irrelevant for indexing (at least for the interface). @alongosz ?

You're right. On SPI level we can throw SPI or API exceptions.

Steveb-p · 2023-04-24T10:24:36Z

eZ/Bundle/EzPublishCoreBundle/Command/ReindexCommand.php

+                'content-type',
+                null,
+                InputOption::VALUE_REQUIRED,
+                'Content type identifier to refresh (deleted/updated/added). Implies "no-purge", cannot be combined with "since", "subtree" or "content-ids"'


Side note, outside of the scope of this PR:
We are not actually checking that conflicting options are passed.

Relaxed constraints for getContentWithContentTypeIdentifier in IndexerGateway interface. Co-authored-by: Paweł Niedzielski <[email protected]>

sonarqubecloud · 2023-04-26T07:35:21Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

No Coverage information
0.0% Duplication

Steveb-p · 2023-07-19T08:00:58Z

eZ/Publish/Core/Search/Legacy/Content/IndexerGateway.php

@@ -63,6 +63,22 @@ public function countContentInSubtree(string $locationPath): int
        return (int)$query->execute()->fetchOne();
    }

+    public function getContentWithContentTypeIdentifier(string $contentTypeIdentifier, int $iterationCount): Generator


The issue reported is relevant. Narrowing down of return type is available for PHP 7.4+, and this version of ezplatform-kernel supports PHP 7.3, which means you cannot use this feature.

Suggested change

public function getContentWithContentTypeIdentifier(string $contentTypeIdentifier, int $iterationCount): Generator

public function getContentWithContentTypeIdentifier(string $contentTypeIdentifier, int $iterationCount): iterable

Steveb-p · 2023-07-19T08:04:04Z

eZ/Publish/SPI/Search/Content/IndexerGateway.php

+    public function getContentWithContentTypeIdentifier(string $contentTypeIdentifier, int $iterationCount): iterable;
+
+    /**
+     * @throws \Doctrine\DBAL\Exception
+     */
+    public function countContentWithContentTypeIdentifier(string $contentTypeIdentifier): int;


Are we actually ok with introducing new method into this class @alongosz? It is a contract.

Are we actually ok with introducing new method into this class @alongosz? It is a contract.

No, this is strict SPI, any additions to contracts there (except for SPI\Persistence) are hard BC break.

OT: Though, TBH I'm not sure what idea I had in mind when placing this in SPI. Maybe I thought it's gonna be implemented by each search engine, but AFAICS it should just fetch data related to indexing directly from Repository. Maybe the purpose was rather Legacy Storage strategy...

alongosz

This improvement is not directly related to a bug fix, but it's rather a new feature making easier to reindex data after a bug fix, right? Moreover AFAICS related CS tickets are 4.x. We fix bugs and performance issues on the oldest supported version, but we don't add features to it.

As a compromise, I would accept this solution for 4.5 instead of 4.6.x-dev, if @webhdx agrees.

Review remarks:

alongosz · 2023-07-19T08:14:22Z

eZ/Bundle/EzPublishCoreBundle/Command/ReindexCommand.php

@@ -128,23 +128,28 @@ protected function configure()
                'since',
                null,
                InputOption::VALUE_OPTIONAL,
-                'Refresh changes since a time provided in any format understood by DateTime. Implies "no-purge", cannot be combined with "content-ids" or "subtree"'
+                'Refresh changes since a time provided in any format understood by DateTime. Implies "no-purge", cannot be combined with "content-ids", "subtree" or "content-type"'


Punctuation: use Oxford comma here and in the other related places please

Suggested change

'Refresh changes since a time provided in any format understood by DateTime. Implies "no-purge", cannot be combined with "content-ids", "subtree" or "content-type"'

'Refresh changes since a time provided in any format understood by DateTime. Implies "no-purge", cannot be combined with "content-ids", "subtree", or "content-type"'

alongosz · 2023-07-19T08:56:46Z

eZ/Publish/SPI/Search/Content/IndexerGateway.php

@@ -40,6 +40,18 @@ public function getContentInSubtree(string $locationPath, int $iterationCount):
     */
    public function countContentInSubtree(string $locationPath): int;

+    /**
+     * @throws \Doctrine\DBAL\Exception


As far as the indexing is concerned, I'm not sure if we want to declare that this can throw this exception. The fact that we are using Doctrine is irrelevant for indexing (at least for the interface). @alongosz ?

You're right. On SPI level we can throw SPI or API exceptions.

alongosz · 2023-07-19T09:00:16Z

eZ/Publish/SPI/Search/Content/IndexerGateway.php

+    public function getContentWithContentTypeIdentifier(string $contentTypeIdentifier, int $iterationCount): iterable;
+
+    /**
+     * @throws \Doctrine\DBAL\Exception
+     */
+    public function countContentWithContentTypeIdentifier(string $contentTypeIdentifier): int;


Are we actually ok with introducing new method into this class @alongosz? It is a contract.

No, this is strict SPI, any additions to contracts there (except for SPI\Persistence) are hard BC break.

OT: Though, TBH I'm not sure what idea I had in mind when placing this in SPI. Maybe I thought it's gonna be implemented by each search engine, but AFAICS it should just fetch data related to indexing directly from Repository. Maybe the purpose was rather Legacy Storage strategy...

alongosz · 2023-07-19T09:18:22Z

eZ/Bundle/EzPublishCoreBundle/Command/ReindexCommand.php

+            $count = $this->gateway->countContentWithContentTypeIdentifier($contentType);
+            $generator = $this->gateway->getContentWithContentTypeIdentifier($contentType, $iterationCount);
+            $purge = false;


Given you can't add anything to that Gateway without breaking BC promise (see previous comment), I think better solution would be to either use Repository Filtering (\eZ\Publish\API\Repository\ContentService::find) or make a dedicated method in \eZ\Publish\SPI\Persistence\Content\Handler. The latter option would be more performant and require implementing cache layer (which is good).

papcio122 · 2023-09-08T08:04:02Z

Closing this PR, moving changes to ibexa/core#259 in order to merge this to 4.5

IBX-5385 add option content-type to reindex command

06a3b9d

github-actions bot assigned adamwojs, ciastektk, mikadamczyk, Steveb-p and alongosz Apr 24, 2023

Steveb-p reviewed Apr 24, 2023

View reviewed changes

eZ/Bundle/EzPublishCoreBundle/Command/ReindexCommand.php Outdated Show resolved Hide resolved

papcio122 requested review from a team April 24, 2023 09:47

papcio122 marked this pull request as ready for review April 24, 2023 09:47

Update eZ/Bundle/EzPublishCoreBundle/Command/ReindexCommand.php

112b69d

Changed from VALUE_OPTIONAL to VALUE_REQUIRED for content-type option Co-authored-by: Paweł Niedzielski <[email protected]>

Steveb-p reviewed Apr 24, 2023

View reviewed changes

adamwojs changed the title ~~IBX-5385 add option content-type to reindex command~~ IBX-5385: Add option content-type to reindex command Apr 24, 2023

Update eZ/Publish/SPI/Search/Content/IndexerGateway.php

a477932

Relaxed constraints for getContentWithContentTypeIdentifier in IndexerGateway interface. Co-authored-by: Paweł Niedzielski <[email protected]>

papcio122 requested a review from a team July 19, 2023 07:52

Steveb-p reviewed Jul 19, 2023

View reviewed changes

alongosz requested changes Jul 19, 2023

View reviewed changes

papcio122 closed this Sep 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IBX-5385: Add option content-type to reindex command #370

IBX-5385: Add option content-type to reindex command #370

papcio122 commented Apr 24, 2023 •

edited

Loading

Steveb-p Apr 24, 2023

alongosz Jul 19, 2023

Steveb-p Apr 24, 2023

sonarqubecloud bot commented Apr 26, 2023

Steveb-p Jul 19, 2023

Steveb-p Jul 19, 2023

alongosz Jul 19, 2023

alongosz left a comment

alongosz Jul 19, 2023

alongosz Jul 19, 2023

alongosz Jul 19, 2023

alongosz Jul 19, 2023

papcio122 commented Sep 8, 2023

	public function getContentWithContentTypeIdentifier(string $contentTypeIdentifier, int $iterationCount): Generator
	public function getContentWithContentTypeIdentifier(string $contentTypeIdentifier, int $iterationCount): iterable

	'Refresh changes since a time provided in any format understood by DateTime. Implies "no-purge", cannot be combined with "content-ids", "subtree" or "content-type"'
	'Refresh changes since a time provided in any format understood by DateTime. Implies "no-purge", cannot be combined with "content-ids", "subtree", or "content-type"'

IBX-5385: Add option content-type to reindex command #370

IBX-5385: Add option content-type to reindex command #370

Conversation

papcio122 commented Apr 24, 2023 • edited Loading

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Apr 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alongosz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papcio122 commented Sep 8, 2023

papcio122 commented Apr 24, 2023 •

edited

Loading