EZP-30027: [REST] Fixed logical op. parsing for Criterions of the same type #2527

RozbehSharahi · 2019-01-18T18:35:19Z

Question	Answer
JIRA issue	EZP-30027
Bug/Improvement	yes
New feature	no
Target version	6.7
BC breaks	no
Tests pass	yes
Doc needed	no

TODO:

Implement feature / fix a bug.
Implement tests.
Fix new code according to Coding Standards ($ composer fix-cs).
Ask for Code Review.

RozbehSharahi · 2019-01-18T19:28:33Z

How shall I ask for a code review? Do I have to do something else apart from creating the PR?

alongosz · 2019-01-21T09:37:06Z

How shall I ask for a code review? Do I have to do something else apart from creating the PR?

Usually by reaching out at our Community Slack or pinging relevant people if you know who to ping. But don't worry, since I saw this PR, I got you covered here :)

alongosz

This looks promising :)

In general things to change:

This PR should rather target 6.7 branch (LTS) as bug probably exists there also.
While the tests cases you've implemented are quite valuable, in case of REST, foremost we need some integration/functional tests in eZ/Bundle/EzPublishRestBundle/Tests/Functional namespace.

Other remarks and questions:

alongosz · 2019-01-21T09:40:29Z

eZ/Publish/Core/REST/Server/Input/Parser/Criterion/LogicalAnd.php

+    {
+        $criteria = [];
+        foreach ($criteriaByType as $type => $criterion) {
+            if (is_array($criterion) && $this->isNumericArray($criterion)) {


Hmm, this is usually called zero-indexed array. But could you elaborate on what happens here, so this code is required?
(this is just a question, not change request. I need to have full understanding of what happens here and right now I don't follow 😅 ).

Hey.

Of course. When parsing a view-input query

<Query> <OR> <Criterion1>some-value</Criterion1> <Criterion2>some-value</Criterion2> </OR> </Query>

EZ will not parse this into a numeric array instead create an associative array with criteria name (Criterion1 & Criterion2) as indexes.

[ "Criterion1" => "some-value", "Criterion2" => "some-value" ]

This is a problem when there is two criteria of same type, like:

<Query> <OR> <ContentTypeIdentifierCriterion>author</ContentTypeIdentifierCriterion> <ContentTypeIdentifierCriterion>book</ContentTypeIdentifierCriterion> <SomeOtherCriterion>some-value</SomeOtherCriterion> </OR> </Query>

The decoder will transform this into a nested array:

[ "ContentTypeIdentifierCriterion" => [ 0 => "author", 1 => "book", ], "SomeOtherCriterion" => "some-value" ]

The method transforms this into a flattend criterion=>configuration array:

[ 0 => [ "type" => "ContentTypeIdentifierCriterion", "data" => "author" ], 1 => [ "type" => "ContentTypeIdentifierCriterion", "data" => "book" ], 2 => [ "type" => "SomeOtherCriterion", "data" => "some-value" ], ]

In case you have a better explaining name for the method please tell me and i will change that.

This looks promising :)

In general things to change:

This PR should rather target 6.7 branch (LTS) as bug probably exists there also.

While the tests cases you've implemented are quite valuable, in case of REST, foremost we need some integration/functional tests in eZ/Bundle/EzPublishRestBundle/Tests/Functional namespace.

Other remarks and questions:

I will add a functional test and change the target branch (probably tomorrow).

I will add a functional test and change the target branch (probably tomorrow).

👍 that will alos help understanding this

@andrerom @alongosz

I changed the target version to 6.7 and implemented functional tests for the View POST route and LogicalAnd, LogicalOr and nesting of those.

[ x ] PHP Code Fixer applied
[ x ] Functional Tests implemented
[ x ] Unit Tests implemented
[ x ] Bug fixed
[ x ] Target version is now 6.7

Side notice:
I realized that on 6.7 the FieldCriterion Parser is not yet implemented and to curiosity backported the current one to it and it works perfect with all tests passing, though I was not sure if it is a good idea to add this feature on this bugfix branch. If you think that backporting the FieldCriterion is an option i would rather merge this:
https://github.com/RozbehSharahi/ezpublish-kernel/commits/backup/fix/rest-api-logical-operators

So people can also use the FieldCriterion on REST and 6.7.

@alongosz WDYT?

I realized that on 6.7 the FieldCriterion Parser is not yet implemented and to curiosity backported the current one to it and it works perfect with all tests passing, though I was not sure if it is a good idea to add this feature on this bugfix branch.

No, we're not backporting features unless we have to due to technical constraints :)

eZ/Publish/Core/REST/Server/Tests/Input/Parser/Criterion/LogicalAndTest.php

eZ/Publish/Core/REST/Server/Tests/Input/Parser/Criterion/LogicalOrTest.php

alongosz

Ok, so I've dug into this a little bit deeper. The root cause of a bug lays within XML processing of input (in \eZ\Publish\Core\REST\Common\Input\Handler\Xml::convertDom). It passes, through several layers, inconsistent data to Criteria.
Basically both cases:

<Query>
  <OR>
    <ContentTypeIdentifierCriterion>author</ContentTypeIdentifierCriterion>
    <ContentTypeIdentifierCriterion>book</ContentTypeIdentifierCriterion>
  </OR>
</Query>

and

<Query>
  <OR>
    <ContentTypeIdentifierCriterion>author</ContentTypeIdentifierCriterion>
  </OR>
</Query>

should produce an array of elements before passing it to LogicalAnd and LogicalOr criterion.
So something like that, for the latter case, should already be given:

[
  "ContentTypeIdentifierCriterion" => [
    0 => "author",
  ],
]

However the mentioned XML handler is too generic to be able to distinguish what should be parsed how. We might attempt to fix it in the future, but probably rather by redesigning entire input handling.

So given that I'll accept current solution. 🎉

Few final remarks:

eZ/Bundle/EzPublishRestBundle/Tests/Functional/ViewTest.php

eZ/Publish/Core/REST/Server/Input/Parser/Criterion/LogicalAnd.php

RozbehSharahi · 2019-02-08T18:33:54Z

@alongosz Hey, i did all changes you requested but still added some final comments above to clarify some stuff.

andrerom · 2019-02-18T15:31:31Z

@alongosz maybe you can pass this to QA?

mnocon

Sorry for the delay, but hopefully it will be in time for 1.7.9 🙂

Looks ok to me, QA approved - ping @alongosz

…f same type)

alongosz · 2019-05-14T09:00:17Z

Great! This is few months old, so rebased. Waiting for CI before merging.

alongosz · 2019-05-14T10:12:37Z

Merged up to 6.13, 7.4, 7.5 and master // cc @ViniTou REST bug-fix inside ;)

Thank you @RozbehSharahi

RozbehSharahi changed the title ~~Fix/rest api logical and and or~~ [EZP-30027] LogicalAnd & LogicalOr not working when using two times the same criterion type Jan 18, 2019

RozbehSharahi changed the title ~~[EZP-30027] LogicalAnd & LogicalOr not working when using two times the same criterion type~~ EZP-30027: LogicalAnd & LogicalOr not working when using two times the same criterion type Jan 19, 2019

ezsystems deleted a comment from ezrobot Jan 21, 2019

alongosz added Ready for review Bug labels Jan 21, 2019

alongosz requested review from ViniTou and alongosz January 21, 2019 09:37

alongosz requested changes Jan 21, 2019

View reviewed changes

andrerom requested a review from Nattfarinn January 21, 2019 20:55

RozbehSharahi force-pushed the fix/rest-api-logical-and-and-or branch from c456005 to 4eb5f4b Compare January 27, 2019 12:54

RozbehSharahi changed the base branch from master to 6.7 January 27, 2019 12:55

RozbehSharahi force-pushed the fix/rest-api-logical-and-and-or branch from 4eb5f4b to ead8990 Compare January 27, 2019 13:48

alongosz requested changes Jan 28, 2019

View reviewed changes

RozbehSharahi force-pushed the fix/rest-api-logical-and-and-or branch from fba1d9a to 917c961 Compare February 9, 2019 15:25

alongosz approved these changes Feb 18, 2019

View reviewed changes

andrerom approved these changes Feb 18, 2019

View reviewed changes

alongosz added Ready for QA and removed Ready for review labels Feb 18, 2019

mnocon self-assigned this Mar 8, 2019

mnocon approved these changes May 14, 2019

View reviewed changes

mnocon removed their assignment May 14, 2019

mnocon added QA approved and removed Ready for QA labels May 14, 2019

rozbehsharahitxb added 2 commits May 14, 2019 10:58

[FIX] Logical Operator on REST Server (not possible to use criteria o…

0b5a0a4

…f same type)

[TASK] Do refactoring according to code review

a2605af

alongosz force-pushed the fix/rest-api-logical-and-and-or branch from 917c961 to a2605af Compare May 14, 2019 08:59

alongosz changed the title ~~EZP-30027: LogicalAnd & LogicalOr not working when using two times the same criterion type~~ EZP-30027: [REST] Fixed logical op. parsing for Criterions of the same type May 14, 2019

alongosz merged commit 75fc90e into ezsystems:6.7 May 14, 2019

alongosz mentioned this pull request Nov 26, 2019

EZP-30806: Fixed processing REST logical op. input with list of objects #2883

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-30027: [REST] Fixed logical op. parsing for Criterions of the same type #2527

EZP-30027: [REST] Fixed logical op. parsing for Criterions of the same type #2527

RozbehSharahi commented Jan 18, 2019 •

edited

Loading

RozbehSharahi commented Jan 18, 2019

alongosz commented Jan 21, 2019

alongosz left a comment

alongosz Jan 21, 2019

RozbehSharahi Jan 21, 2019

RozbehSharahi Jan 21, 2019

andrerom Jan 21, 2019

RozbehSharahi Jan 27, 2019 •

edited

Loading

andrerom Jan 28, 2019

alongosz Jan 28, 2019

alongosz left a comment •

edited

Loading

RozbehSharahi commented Feb 8, 2019

andrerom commented Feb 18, 2019

mnocon left a comment

alongosz commented May 14, 2019

alongosz commented May 14, 2019

EZP-30027: [REST] Fixed logical op. parsing for Criterions of the same type #2527

EZP-30027: [REST] Fixed logical op. parsing for Criterions of the same type #2527

Conversation

RozbehSharahi commented Jan 18, 2019 • edited Loading

RozbehSharahi commented Jan 18, 2019

alongosz commented Jan 21, 2019

alongosz left a comment

Choose a reason for hiding this comment

alongosz Jan 21, 2019

Choose a reason for hiding this comment

RozbehSharahi Jan 21, 2019

Choose a reason for hiding this comment

RozbehSharahi Jan 21, 2019

Choose a reason for hiding this comment

andrerom Jan 21, 2019

Choose a reason for hiding this comment

RozbehSharahi Jan 27, 2019 • edited Loading

Choose a reason for hiding this comment

andrerom Jan 28, 2019

Choose a reason for hiding this comment

alongosz Jan 28, 2019

Choose a reason for hiding this comment

alongosz left a comment • edited Loading

Choose a reason for hiding this comment

RozbehSharahi commented Feb 8, 2019

andrerom commented Feb 18, 2019

mnocon left a comment

Choose a reason for hiding this comment

alongosz commented May 14, 2019

alongosz commented May 14, 2019

RozbehSharahi commented Jan 18, 2019 •

edited

Loading

RozbehSharahi Jan 27, 2019 •

edited

Loading

alongosz left a comment •

edited

Loading