Walkable queries #81

bdunogier · 2016-10-29T17:01:58Z

Status: fresh, working prototype

This adds support for a query migration type, that walks over results from a query, and apply another migration to them.

This allows operations such as multi-criteria selection of a set of items with another operation on each result. The Expression Language is used to map parameters from the query to migration parameters, with a collection_item function that has each current item of each iteration.

Usage example: for a set of content items, it adds locations to them below items from a relation list field (uses a custom function), and delete the former main location:

# Find all "Businesses" items
-
    mode: content
    type: query
    match:
        query_type: businesses
    collection: businesses
    walk:
        # For each business content item, find the locations of related content items from the "sector" field
        -
            mode: location
            type: query
            match:
                query_type: business_sectors
                parameters:
                    business: '@=relatedContentIds(collection_item("businesses"), "sector")'
            collection: sectors
            walk:
                # Add a location to each business under each sector
                -
                    mode: create
                    type: location
                    match:
                        content_id: "@=collection_item('businesses').id"
                    parent_location_id: "@=collection_item('sectors').id"

        # Delete the former main location
        -
            mode: delete
            type: location
            match:
                location_id: "@=collection_item('businesses').contentInfo.mainLocationId"

Custom expression language functions, such as relatedContentIds, can be registered with a service tag.

TODO

Handle the QueryType problem. The feature did not exist in older kernel versions. Needs to be handled cleanly (this is why Travis fails)
Cleanup copyright headers ;)
Add the new requirements (expression language ? anything else ?)
Add an example of walk to the DSL docs

Added phpdoc, or fixed obvious errors.

Allows stateless executors (almost), thus nested executors.

Allows storage of non string references.

Allows to walk query results, and run a step for each result: ``` - mode: location type: query match: query_type: some_query_type collection: my_collection walk: # run another query on each location - mode: location type: query match: query_type: get_x_children parameters: location_id: "@=collection_item(my_collection).id" collection: some_other_collection walk: - ... # Do something to the location - mode: update type: location match: location_id: "@=collection_item('my_collection')" # .. ```

crevillo · 2016-10-29T17:53:53Z

very interesting!!

gggeek · 2016-10-31T10:22:14Z

Core/Executor/RepositoryExecutor.php

@@ -100,7 +100,7 @@ public function execute(MigrationStep $step)

            $previousUserId = $this->loginUser(self::ADMIN_USER_ID);
            try {
-                $output = $this->$action();
+                $output = $this->$action($step->dsl);


mmm, ce n'est pas dejà dispo qq part ?

It is available, but we can't use a stateful variable (explained that in the commit message). Since all executors have the DSL stored as a property, changes by one affect the DSL read by the other layers. And bam.

It is kind of a big change, but I don't see big counterparts.

bdunogier · 2016-10-31T13:46:07Z

I doubt that this failure is on me: https://travis-ci.org/kaliop-uk/ezmigrationbundle/jobs/171643923 :-)

But a little problem I have here: this implementation requires QueryTypes. And those were added in eZ Platform, meaning kernel >= 6.0. I guess I must disable the feature for earlier versions somehow ?

gggeek · 2016-10-31T15:26:40Z

Yeah, the Travis failure on eZ6 is weird, I pin it down to Travis caching some bits of Sf somehow... it makes little sense at first sight, as in theory it fails with an SF3 error, but we are running that test case on SF2.8... So I just tagged testing on eZ6 as 'allow fail' for now, while I carve out time for a deeper investigation.

As for the need to be able to run the test suite on eZP versions starting 2014.3 yes, please :-)

gggeek · 2016-10-31T15:29:11Z

ps: as a side note: it was my original intention to allow the existing 'matcher' classes to be able to work on an array of conditions passed in, not just a single one (in short: turn matchers into query builders). I never came around to implementing that. At first sight, it seems that your code does something similar but in a different way... did you take a look at extending the matchers ?

bdunogier · 2016-10-31T16:36:55Z

I first started by implementing queries on matchers, and it turned out to be tedious and repetitive. It requiredwhich seem far from efficient. Rather than heavily changing the existing, this implementations sits around it.

I found out that there are a few advantages to this structure:

it allows nested calls, something you could not do by using the matchers
it makes iteration over collections more natural, as you only work on one content item at a time

The nesting part is really important with the use-case i'm working with, and I think that it allows for complex migrations with the comfort and ease of use of yaml.

gggeek · 2016-10-31T16:57:19Z

I see what you mean.
But is it compatible at all with eZP 5 / 2014.11 ?
(also: in terms of efficiency, does is still build an efficient solr/db filter when nesting filters ?)

bdunogier · 2016-10-31T17:02:07Z

QueryTypes: no, they're not. That's why I was hinting about earlier :)

gggeek · 2016-10-31T17:03:03Z

ps: could you add an example DSL in the doc folder ?

lolautruche · 2016-10-31T18:38:04Z

Hi

As for QueryTypes, they could be backported somewhere like EzCoreExtraBundle. I intended to do so sooner or later.

lolautruche · 2016-10-31T18:39:08Z

Sorry for my ignorance, but what is collection: businesses? Is this a way to label collections ? Or something else that I missed?

bdunogier · 2016-10-31T23:04:36Z

It is how collections are set as references, to be used later on with collection_item().

lolautruche · 2016-11-01T08:53:14Z

Core/Executor/QueryManager.php

+    public function addExecutor(ExecutorInterface $executor)
+    {
+        foreach($executor->supportedTypes() as $type) {
+            $this->executors[$type] = $executor;


Shouldn't this be $this->executors[$type][]?

lolautruche · 2016-11-01T08:55:00Z

Core/Executor/QueryManager.php

+     * @param $searchResult
+     * @return boolean
+     */
+    protected function setReferences($args)


Why not using proper arguments here?

lolautruche · 2016-11-01T09:08:16Z

DependencyInjection/CompilerPass/QueryManagerCompilerPass.php

+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Reference;
+
+class QueryManagerCompilerPass implements CompilerPassInterface


Please briefly explain what this compiler pass is meant for

lolautruche · 2016-11-01T09:10:22Z

Resources/config/services.yml

+
+    ez_migration_bundle.expression_language:
+        class: Symfony\Component\ExpressionLanguage\ExpressionLanguage
+        lazy: true


lolautruche · 2016-11-01T09:11:23Z

DependencyInjection/CompilerPass/QueryManagerCompilerPass.php

+     */
+    public function process(ContainerBuilder $container)
+    {
+        if ($container->has('ez_migration_bundle.executor.query_manager') && $container->has('ez_migration_bundle.migration_service')) {


Better using return asap pattern here :-)

lolautruche · 2016-11-01T09:24:20Z

Core/ExpressionLanguage/CollectionItemFunctionProvider.php

+                function() {
+                    throw new \Exception('ref function does not support compilation');
+                },
+                function($foo, $identifier) {


$foo represents the variables from the expression context. This is actually a hash, identical to what we pass to a twig view when rendering it.
So we'd better name it $variables and type hint it as array 😉

lolautruche · 2016-11-01T09:28:30Z

Core/Executor/QueryManager.php

+
+    protected function getSearchService()
+    {
+        return $this->repository->getSearchService();


Is this really needed?

lolautruche · 2016-11-01T09:29:05Z

Core/Executor/QueryManager.php

+    {
+        foreach ($this->matchers as $matcher) {
+            $result = $matcher->match($dsl['match']);
+            if (count($result) == 1) {


What if count is > 1?

lolautruche · 2016-11-01T09:29:15Z

Core/Executor/QueryManager.php

+        $this->referenceResolver->addReference('collection_' . $collection, $value, true);
+    }
+
+    protected function getQuery($dsl)


PHPDoc please 😃

lolautruche · 2016-11-01T09:31:28Z

Core/Executor/QueryManager.php

+use Symfony\Component\ExpressionLanguage\ExpressionFunction;
+use Symfony\Component\ExpressionLanguage\ExpressionLanguage;
+
+class QueryManager extends RepositoryExecutor


What about QueryExecutor instead?
Besides, class name isn't quite self-explanatory. Maybe a phpdoc comment is needed 😉

OK forget it, it's perfectly inline with other managers 😊

Allows usage of '%parameter%' in any field that supports references.

gggeek · 2017-02-06T00:02:44Z

About the QueryType class not existing in older kernels: what if we split off this PR into its own bundle & repo, and only try to push into the migrationbundle the minimal changes required to support it?

Apart from solving the requirements problem, I think that there might be other, smaller benefits to that approach:

as much as I like the flexibility given by your solution, I am also loath to add such a huge chunk of code (at least until I feel comfortable enough to efficiently maintain it);
I also fear that the apparent complexity of the walkable-queries approach and example might put off some less-technical users of the bundle, or give them bad experience by leading them into using a tool which will shoot them in the foot (of course I might be completely wrong on this point).

gggeek · 2020-10-29T19:06:26Z

Status update:

the 'walkable collections' part has been implemented, imho, via the generic loop migration step. This part can probably be removed from the PR
adding support for 'QueryType' seems like a worthy addition... maybe it could be implemented simply via a couple of new methods in the existing ContentMatcher/LocationMatcher? (I confess to never having used QueryTypes, I assume they can be used to fetch either Nodes or Locations only?)
I am still undecided about having new resolvers enabled by default (such as fe. for container parameters or env vars). On one hand they seem useful, on the other hand I don't want to break existing migrations which have in their strings/contents anything which matches a new introduced prefix... (of course we could introduce them in code but leave their services inactive. I think that this solution would not make them very discoverable by end users though...)
...speaking about which: I am thinking to massively revamp the whole 'resolver' concept in the next major release (v6) by making each string in a migration element resolvable as if it was a twig template. This would give us access to the whole symfony expression language syntax, and would make it easier to add resolvers without risking breaking existing migrations
anything else I am missing?

gggeek · 2020-11-16T13:47:36Z

Status update: matching based on QueryType has been implemented, and will be in 5.14.
No feedback was given for more than 2 weeks since my last comment, which probably means that either everything is fine, or the original dev lost interest in this ;-)
Closing.

Bertrand Dunogier added 6 commits October 29, 2016 17:08

Fixed misc errors + phpdoc

5fc095a

Added phpdoc, or fixed obvious errors.

Passed the DSL as an execution argument

545a74b

Allows stateless executors (almost), thus nested executors.

Non strings are not references

2051ef1

Allows storage of non string references.

Added 'overwrite' argument to addReference()

0abddd0

Added ExpressionLanguage Resolver

46366d9

gggeek reviewed Oct 31, 2016

View reviewed changes

lolautruche reviewed Nov 1, 2016

View reviewed changes

Bertrand Dunogier added 5 commits November 10, 2016 23:50

Added a ContainerParameter ReferenceResolver

cc84799

Allows usage of '%parameter%' in any field that supports references.

Added reference support location remote id and fields

2547036

fixup! Added ExpressionLanguage Resolver

b18cb6e

fixup! Added 'query' step type

f9dc8c2

fixup! Added 'query' step type

231ef53

gggeek pushed a commit that referenced this pull request Nov 20, 2016

Merge the non-intrusive bits from PR #81

6a717d8

Bertrand Dunogier added 2 commits December 12, 2016 21:37

Added location remote_id support

756696e

Added id/remote_id parent_location_id loading

b6a02af

gggeek mentioned this pull request Jun 22, 2018

Allow to run for loops over collections of items #163

Closed

gggeek mentioned this pull request Oct 29, 2020

Add support to match using existing QueryTypes #239

Closed

gggeek closed this Nov 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Walkable queries #81

Walkable queries #81

bdunogier commented Oct 29, 2016 •

edited

crevillo commented Oct 29, 2016

gggeek Oct 31, 2016

bdunogier Oct 31, 2016

bdunogier commented Oct 31, 2016 •

edited

gggeek commented Oct 31, 2016

gggeek commented Oct 31, 2016 •

edited

bdunogier commented Oct 31, 2016 •

edited

gggeek commented Oct 31, 2016 •

edited

bdunogier commented Oct 31, 2016

gggeek commented Oct 31, 2016

lolautruche commented Oct 31, 2016

lolautruche commented Oct 31, 2016

bdunogier commented Oct 31, 2016 •

edited

lolautruche Nov 1, 2016

lolautruche Nov 1, 2016

lolautruche Nov 1, 2016

lolautruche Nov 1, 2016

lolautruche Nov 1, 2016

lolautruche Nov 1, 2016

lolautruche Nov 1, 2016

lolautruche Nov 1, 2016

lolautruche Nov 1, 2016

lolautruche Nov 1, 2016

lolautruche Nov 1, 2016

gggeek commented Feb 6, 2017

gggeek commented Oct 29, 2020 •

edited

gggeek commented Nov 16, 2020

Walkable queries #81

Walkable queries #81

Conversation

bdunogier commented Oct 29, 2016 • edited

TODO

crevillo commented Oct 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdunogier commented Oct 31, 2016 • edited

gggeek commented Oct 31, 2016

gggeek commented Oct 31, 2016 • edited

bdunogier commented Oct 31, 2016 • edited

gggeek commented Oct 31, 2016 • edited

bdunogier commented Oct 31, 2016

gggeek commented Oct 31, 2016

lolautruche commented Oct 31, 2016

lolautruche commented Oct 31, 2016

bdunogier commented Oct 31, 2016 • edited

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

Choose a reason for hiding this comment

gggeek commented Feb 6, 2017

gggeek commented Oct 29, 2020 • edited

gggeek commented Nov 16, 2020

bdunogier commented Oct 29, 2016 •

edited

bdunogier commented Oct 31, 2016 •

edited

gggeek commented Oct 31, 2016 •

edited

bdunogier commented Oct 31, 2016 •

edited

gggeek commented Oct 31, 2016 •

edited

bdunogier commented Oct 31, 2016 •

edited

gggeek commented Oct 29, 2020 •

edited