EZP-26368: As a developer I want to index custom data with Solr Search Engine #67

pspanja · 2016-09-27T14:30:56Z

This PR resolves https://jira.ez.no/browse/EZP-26368

This is #42 continued.
As was discussed previously LocationTranslation mapper is here removed, to be proposed in a separate PR.

This implements document field mappers, aka indexing plugins feature.
Three types of base plugins are implemented:

Content::mapFields(Content $content)
ContentTranslation::mapFields(Content $content, $languageCode)
Location::mapFields(Location $location)

For each of these an Aggregate plugin is implemented and used (6 places total):

Content::mapFields(Content $content)
1. Block - indexes fields on the block document level -- meaning to each Content and Location document in the block
2. Content - indexes fields to each Content document
ContentTranslation::mapFields(Content $content, $languageCode)
1. BlockTranslation - indexes fields on the block level per translation
2. ContentTranslation - indexes fields to Content document per translation
Location::mapFields(Location $location)
1. Location - indexes field to each Location document

The same, but starting from extension points (service tags):

Block: ezpublish.search.solr.document_field_mapper.block
- Accepts Content::mapFields(Content $content)
Block translation: ezpublish.search.solr.document_field_mapper.block_translation
- Accepts ContentTranslation::mapFields(Content $content, $languageCode)
Content: ezpublish.search.solr.document_field_mapper.content
- Accepts Content::mapFields(Content $content)
Content translation: ezpublish.search.solr.document_field_mapper.content_translation
- Accepts ContentTranslation::mapFields(Content $content, $languageCode)
Location: ezpublish.search.solr.document_field_mapper.location
- Accepts Location::mapFields(Location $location)

Concrete plugins are registered to Aggregate plugin in a container compiler pass, using service tags.
At the moment no concrete plugins are implemented, followup to this would be refactoring DocumentMapper to use only plugins to map fields. How that would look like can be seen here: pspanja#1
This is DocumentMapper fully refactored to use plugins: https://github.com/pspanja/ezplatform-solr-search-engine/blob/3a439bcf098b7e5a6c397204fc5849737cb1a60b/lib/DocumentMapper/NativeDocumentMapper.php

pspanja · 2016-09-27T16:58:36Z

Ready for review, ping @andrerom @alongosz @bdunogier

alongosz

+1
(I would remove @version //autogentag// from all new files, beside that it looks good)

andrerom

+1, however do you also have updated PR with the followup changes where inline logic is removed in favor of using this plugin system?

bdunogier · 2016-09-28T21:02:38Z

lib/Container/Compiler/BaseDocumentFieldMapperPass.php

+/**
+ * Base compiler pass for aggregate document field mappers.
+ */
+abstract class BaseDocumentFieldMapperPass implements CompilerPassInterface


Not a real review (I don't request the change), but we have similar compiler passes over the place. Shouldn't we try to have a common one that we can re-use in all of our packages ?

I guess we could do that, it should probably be in ezpublish-kernel repo.

bdunogier

There you go. I hope the review ain't too far off :)

bdunogier · 2016-09-28T21:03:49Z

lib/DocumentMapper/FieldMapper.php

+/**
+ * Base class for document field mappers.
+ */
+abstract class FieldMapper


Why not an interface rather than an abstract class ? i'd actually really suggest an interface. An abstract class would have a particular behaviour, while FieldMapper hints at a "role" (e.g. what it does).

Actually I think I'll just remove this, as it serves not purpose ATM. We can introduce it later on if need be.

Removed in a433031

bdunogier · 2016-09-28T21:05:20Z

lib/DocumentMapper/FieldMapper/Content.php

+ *
+ * Content document field mapper maps Content to the search fields for Content document.
+ */
+abstract class Content extends FieldMapper


Same as above (interface vs. abstract class).

Also, I tend to avoid naming things "Content" or similar. We end up with an s'load of those. ContentFieldMapper is still short enough, is it not ?

(I won't be fanatic about it, just an improvement imho).

Not sure about the benefit of the interface here, it's perfectly fine for abstract class not to have it's own implementation. Role would be more like FieldMapping.

Will rename when moving up.

Renamed (and moved up) in 30ffce1.

bdunogier · 2016-09-28T21:07:12Z

lib/DocumentMapper/FieldMapper/Location/Aggregate.php

+/**
+ * Aggregate implementation of Location document field mapper.
+ */
+class Aggregate extends LocationMapper


Is it me or all of your Aggregate mappers have the same code ?

It's you :) They are mostly the same, but accept different mapper type.

bdunogier · 2016-09-28T21:13:00Z

lib/DocumentMapper/NativeDocumentMapper.php

+    {
+        $fields = [];
+
+        if ($this->locationTranslationFieldMapper->accept($location, $languageCode)) {


Would be shorter with a return early:

if ($this->locationTranslationFieldMapper->accept($location, $languageCode)) { return []; } return $this->locationTranslationFieldMapper->mapFields($location, $languageCode);

It would, but I would not be able to process it at a glance :)
It's really a matter of style.

bdunogier · 2016-09-28T21:16:12Z

lib/DocumentMapper/NativeDocumentMapper.php

@@ -83,6 +117,12 @@ class NativeDocumentMapper implements DocumentMapper
    /**
     * Creates a new document mapper.
     *
+     * @param \EzSystems\EzPlatformSolrSearchEngine\DocumentMapper\FieldMapper\Content $blockFieldMapper


Jokes aside, could this be refactored to be a little slimer ? :)

The only possibility I see is to move it up, on the same level as DocumentMapper.
I actually like that idea, it's the same approach I took in pspanja#4 with a number of other services.

Moved up in 30ffce1

pspanja · 2016-09-29T07:19:54Z

@alongosz

(I would remove @Version //autogentag// from all new files, beside that it looks good)

Removed in a2b3a89

pspanja · 2016-09-29T07:22:48Z

@andrerom

do you also have updated PR with the followup changes where inline logic is removed in favor of using this plugin system?

Edited: Not updated yet, I wanted to do it in a followup.

pspanja · 2016-09-29T12:00:59Z

I think I addressed all remarks.
I changed the structure a bit, moving FieldMapper up one level, so please take another look.

andrerom · 2016-09-29T16:06:22Z

What do you think @bdunogier? From extensibility perspective do we hit a good spot here?

andrerom

+1, but this implies master should be bumped to 1.2.x, so when this is merged, commit before should be branched of to 1.1 in case we need to do another 1.1 release.

pspanja · 2016-11-04T10:39:30Z

There were no significant commits to master since the last change here, so with @bdunogier's approval I can move this forward.

bdunogier · 2016-11-04T15:00:53Z

Is there an example that I can look at somewhere ? It would help a lot :)

pspanja · 2016-11-04T15:05:51Z

The one linked in the description, should be virtually identical, aside from placement, removal of LocationTranslation mapper and removal of the common abstract class. The code you see there is basically what we'll be going for next with DocumentMapper refactoring to use plugins:

https://github.com/pspanja/ezplatform-solr-search-engine/blob/3a439bcf098b7e5a6c397204fc5849737cb1a60b/lib/DocumentMapper/NativeDocumentMapper.php

bdunogier · 2016-11-04T15:08:10Z

aside from placement, removal of LocationTranslation mapper and removal of the common abstract class

"lol" ;)

Thank you for the link, I'll look into it.

pspanja · 2016-11-04T15:19:35Z

You're welcome. This is the code with consolidated field names and visitors, so basically removing unnecessary duplication even further:

https://github.com/pspanja/ezplatform-solr-search-engine/tree/3d7db66c36771fb26b5d70a94fe6fa7f496accd5/lib/DocumentMapper

Also do check the container configuration:

https://github.com/pspanja/ezplatform-solr-search-engine/blob/3d7db66c36771fb26b5d70a94fe6fa7f496accd5/lib/Resources/config/container/solr/document_field_mappers.yml

bdunogier · 2016-11-23T12:31:33Z

Looks okay from an extensibility pov. It would be much easier to review this aspect with documentation about the said extensibility (I've found out that writing this doc together with the feature really helps).

bdunogier · 2016-11-23T12:29:49Z

lib/Container/Compiler/FieldMapperPass/ContentFieldMapperPass.php

+ */
+class ContentFieldMapperPass extends BaseFieldMapperPass
+{
+    const AGGREGATE_MAPPER_SERVICE_ID = 'ezpublish.search.solr.field_mapper.content';


A bit late, so take that as an FYI, but you could have use one concrete class (FieldMapperPass), with the service id / service tag as arguments, and instantiate the Pass N times from the bundle, with the respective arguments. Less files, same result, easier to test.

andrerom · 2016-11-23T13:59:30Z

doc/specifications/document_field_mappers.md

@@ -0,0 +1,100 @@
+# Document Field Mappers
+
+As a website developer you will often find the need to index some additional data in the search


website developer => developer

andrerom · 2016-11-23T14:02:47Z

doc/specifications/document_field_mappers.md

+
+As a website developer you will often find the need to index some additional data in the search
+engine. The use cases for this are wide, for example the data could come from the external source
+(for example from recommendation system), or from the internal source.


can be italic: (for example from recommendation system)

MarioBlazek · 2016-11-24T10:09:38Z

Excellent, thanks @pspanja .

pspanja · 2016-11-30T08:12:38Z

@andrerom

+1, but this implies master should be bumped to 1.2.x, so when this is merged, commit before should be branched of to 1.1 in case we need to do another 1.1 release.

A little bit late with this, now created 1.1, alias bumped in #73

pspanja force-pushed the indexing_plugins2 branch from 6bab612 to 6fcdbdf Compare September 27, 2016 16:38

pspanja changed the title ~~Indexing plugins2~~ EZP-26368: As a developer I want to index custom data with Solr Search Engine Sep 27, 2016

pspanja mentioned this pull request Sep 27, 2016

Implement indexing plugins #42

Closed

1 task

alongosz approved these changes Sep 28, 2016

View reviewed changes

andrerom reviewed Sep 28, 2016

View reviewed changes

bdunogier reviewed Sep 28, 2016

View reviewed changes

andrerom approved these changes Sep 30, 2016

View reviewed changes

bdunogier approved these changes Nov 23, 2016

View reviewed changes

pspanja force-pushed the indexing_plugins2 branch from 92c2f38 to 6bb041f Compare November 23, 2016 13:53

andrerom reviewed Nov 23, 2016

View reviewed changes

pspanja force-pushed the indexing_plugins2 branch from 6bb041f to 09a1927 Compare November 24, 2016 09:02

pspanja and others added 7 commits November 24, 2016 10:11

Implement indexing plugins

2789daf

Replace argument expansion operator

00001d5

Docblock: Add missing return type hint

83ec60a

Register compiler passes with container

abd9e73

Remove LocationTranslation field mapper

8c09c53

Remove version tag from file docblocks

5dca784

Remove base FieldMapper

a4da3fd

pspanja added 5 commits November 24, 2016 10:11

Use double quotes for consistency

4f63cd4

Move field mappers one level up

53df8d6

Update compiler passes

3572a8f

Update container service IDs

d3f46c4

Add speficication

6834483

pspanja force-pushed the indexing_plugins2 branch from 09a1927 to 6834483 Compare November 24, 2016 09:12

pspanja merged commit c98bff0 into master Nov 24, 2016

pspanja deleted the indexing_plugins2 branch November 24, 2016 09:44

		@@ -0,0 +1,100 @@
		# Document Field Mappers

		As a website developer you will often find the need to index some additional data in the search

EZP-26368: As a developer I want to index custom data with Solr Search Engine #67

EZP-26368: As a developer I want to index custom data with Solr Search Engine #67

Conversation

pspanja commented Sep 27, 2016 • edited

pspanja commented Sep 27, 2016

alongosz left a comment

Choose a reason for hiding this comment

andrerom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdunogier 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

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

pspanja Sep 29, 2016 • edited

Choose a reason for hiding this comment

bdunogier Sep 28, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pspanja commented Sep 29, 2016

pspanja commented Sep 29, 2016 • edited

pspanja commented Sep 29, 2016

andrerom commented Sep 29, 2016

andrerom left a comment

Choose a reason for hiding this comment

pspanja commented Nov 4, 2016

bdunogier commented Nov 4, 2016

pspanja commented Nov 4, 2016 • edited

bdunogier commented Nov 4, 2016 • edited

pspanja commented Nov 4, 2016

bdunogier commented Nov 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrerom Nov 23, 2016 • edited

Choose a reason for hiding this comment

MarioBlazek commented Nov 24, 2016

pspanja commented Nov 30, 2016

pspanja commented Sep 27, 2016 •

edited

pspanja Sep 29, 2016 •

edited

bdunogier Sep 28, 2016 •

edited

pspanja commented Sep 29, 2016 •

edited

pspanja commented Nov 4, 2016 •

edited

bdunogier commented Nov 4, 2016 •

edited

andrerom Nov 23, 2016 •

edited