Flowpack.ElasticSearch.ContentRepositoryAdaptor icon indicating copy to clipboard operation
Flowpack.ElasticSearch.ContentRepositoryAdaptor copied to clipboard

nodeindex:build only based on default dimension

Open regniets opened this issue 5 years ago • 11 comments

We have a setup with a dimension "zz" that only shares the rootpage for language selection, but is defaultPreset. In the latest version of the Adaptor (on Neos 5.2 & ES 7) nodeindex:build will index only the nodes that are found in the zz dimension and their respective dimension representatives. So we end up with only Home being indexed in all dimensions, but no further pages.

A change introduced that we found is in NodeIndexCommandController: Instead of calling $this->workspaceIndexer->index(), $this->workspaceIndexer->indexWithDimensions() is being called, which doesn't loop over dimensions initially.

But in order for the nodes to be indexed independently in all dimensions, it still doesnt work properly, since the rootNode still is "dimensionless": https://github.com/Flowpack/Flowpack.ElasticSearch.ContentRepositoryAdaptor/blob/0660ba2ddd14a7968436eb0d76a00b9b9b93d6c4/Classes/Indexer/WorkspaceIndexer.php#L89

regniets avatar Jun 18 '20 12:06 regniets

Uh that sounds bad. The problem was, that Dimensions were cycled twice, once in the controller and once in https://github.com/Flowpack/Flowpack.ElasticSearch.ContentRepositoryAdaptor/blob/master/Classes/Indexer/NodeIndexer.php#L308 So I checked both loops and removed one of them. But as I understand your setup, traversing the tree is also not possible if the controller would loop over dimensions. So how did the old version found your nodes?

daniellienert avatar Jun 18 '20 13:06 daniellienert

Yes, i still haven't found out why and how it worked in the old version.

The new approach seems to work only if you have 1:1 relations in dimensions and nodes, but doesn't take "loose dimensions" into account. I can provide you with a PR how we worked around this issue (which is very pragmatic, not sure if it's the right way to go) and might make the second loop redundant (you might lose some weird orphan nodes though).

(i.e. dimensions taken into account on root level, not on node level)

regniets avatar Jun 18 '20 13:06 regniets

After thinking about. Really helpful would be to add a lightweight test scenario with some nodes, that resamples your setup. The test could just run the indexer and test the result. An example can be found here https://github.com/Flowpack/Flowpack.ElasticSearch.ContentRepositoryAdaptor/blob/master/Tests/Functional/Eel/ElasticSearchQueryTest.php#L407. That would make debugging and fixing it for good much easier.

daniellienert avatar Jun 19 '20 06:06 daniellienert

Sounds good. Is there a practical guide on running the tests?

regniets avatar Jun 19 '20 07:06 regniets

Kinda like this? https://github.com/regniets/Flowpack.ElasticSearch.ContentRepositoryAdaptor/blob/bugfix/multi-dimension-test/Tests/Functional/Indexer/NodeIndexerDimensionsTest.php Not refined and not tested, that would be my next task :)

regniets avatar Jun 19 '20 11:06 regniets

Yeah! Just open a PR and the test will run automatically. (Will fail because of createNodesForDimensionsTest != createNodesForLanguageDimensions).

You can run the test locally by running

FLOW_CONTEXT="Testing/ElasticVersion6" bin/phpunit --colors --stop-on-failure -c Build/BuildEssentials/PhpUnit/FunctionalTests.xml Packages/Application/Flowpack.ElasticSearch.ContentRepositoryAdaptor/Tests/Functional;

daniellienert avatar Jun 20 '20 05:06 daniellienert

Ok, the test fails (as it should, but not because of the wrong method). Took out the var_dump, just wanted to check if the error is the same as i keep getting and it is, so my setup seems to be somewhat correct. Actually not the error we keep getting in our project, so something might be still wrong in how i adress the problem - although the error is somewhat related and it does state Workspace "live" without dimensions done, which is definitely wrong. It should have indexed the dimension ['language' => 'zz'] for the hash 20cc3bfb2e34875a938ea2b8015f24eb

regniets avatar Jun 22 '20 07:06 regniets

This now is a (probably) working test scenario: https://github.com/Flowpack/Flowpack.ElasticSearch.ContentRepositoryAdaptor/pull/338

I had to adjust it to the neos/demo package which is the basis of the functional tests and its dimension-presets.

regniets avatar Jun 22 '20 12:06 regniets

btw - it is not only nodeindex:build, but also nodeindex:indexnode, i'll try to cover this one as well...

regniets avatar Jun 23 '20 09:06 regniets

I am also experiencing this behaviour. Our setup is:

  • 1 Dimension "language", 4 values: en, de, fr, es - no fallbacks Behaviour: Only nodes are indexed that are in the default dimension combination "en" and the variants of them in other dimension. But nodes that only exists in other languages - and not in the default language - are not indexed.

By changing $this->workspaceIndexer->indexWithDimensions($workspace, $dimensionsValuesArray, $limit, $workspaceLogger); to $this->workspaceIndexer->index($workspace, $limit, $workspaceLogger); the problem seems to be solved. As well not sure if this (re-)introduces redundant indexing of nodes, which is a big issue in the (old) fork-version of this package.

DrillSergeant avatar Aug 05 '20 07:08 DrillSergeant

With the merged PR #350 (version 7.0.3) this issue is gone in my current project.

DrillSergeant avatar Sep 08 '20 12:09 DrillSergeant