Improve qb validation #553

lsmith77 · 2014-08-29T09:42:23Z

fixes some bugs in the tests, also fixes #541

note Doctrine\Tests\ODM\PHPCR\Query\Builder\BuilderConverterPhpcrTest::testOrderBy is failing .. but honestly I don't really think such a test makes sense. It is way too complex to test as it requires so much mocking. IMHO we are better off testing this via the various existing functional test. So I vote for removing that test.

/cc @dantleech

dantleech · 2014-08-29T12:59:31Z

If we can cover it in a functional test (or switch to using prophecy maybe) then thats fine. Will havecloser look later.

Lukas Kahwe Smith notifications@github.com wrote:

fixes some bugs in the tests, also fixes
#541

note
Doctrine\Tests\ODM\PHPCR\Query\Builder\BuilderConverterPhpcrTest::testOrderBy
is failing .. but honestly I don't really think such a test makes
sense. It is way too complex to test as it requires so much mocking.
IMHO we are better off testing this via the various existing functional
test. So I vote for removing that test.

/cc @dantleech

You can merge this Pull Request by running:

git pull https://github.com/doctrine/phpcr-odm improve_qb_validation

Or you can view, comment on it, or merge it online at:

#553

-- Commit Summary --

fixed tests

typo fix

pushed field/orderby validation into the QB

-- File Changes --

M lib/Doctrine/ODM/PHPCR/DocumentRepository.php (27)
M lib/Doctrine/ODM/PHPCR/Query/Builder/BuilderConverterPhpcr.php (38)
M tests/Doctrine/Tests/ODM/PHPCR/Functional/DocumentRepositoryTest.php
(14)
M
tests/Doctrine/Tests/ODM/PHPCR/Query/Builder/BuilderConverterPhpcrTest.php
(2)

-- Patch Links --

https://github.com/doctrine/phpcr-odm/pull/553.patch
https://github.com/doctrine/phpcr-odm/pull/553.diff

Reply to this email directly or view it on GitHub:
#553

Sent from my Android device with K-9 Mail. Please excuse my brevity.

dbu · 2014-08-30T15:14:07Z

|Doctrine\Tests\ODM\PHPCR\Query\Builder\BuilderConverterPhpcrTest::testOrderBy|
is failing .. but honestly I don't really think such a test makes sense.
It is way too complex to test as it requires so much mocking. IMHO we
are better off testing this via the various existing functional test. So
I vote for removing that test.

+1 for remove if you are sure all cases are covered by functional tests.

dantleech · 2014-08-30T18:22:56Z

I will have a look

dantleech · 2014-08-30T18:54:26Z

lib/Doctrine/ODM/PHPCR/Query/Builder/BuilderConverterPhpcr.php

Not all nodes have the getField method, this is why the test failed.

so the tests exposed a bug?

Yeah, that happrens sometimes :P

dantleech · 2014-08-30T19:33:35Z

I made a PR against this PR.

The tests were not as bad as I thought and I think that the complexity is fine and they were infact revealing a bug. Only the DynamicField operator has the ->getField method, so I moved the validation to the method that deals with objects of that type.

dantleech · 2014-08-30T19:34:14Z

#554

lsmith77 · 2014-08-31T06:18:34Z

lib/Doctrine/ODM/PHPCR/Query/Builder/BuilderConverterPhpcr.php

guess this can be reverted now?

Looks that way

dantleech · 2014-08-31T13:19:28Z

btw, the tests can certainly be improved (e.g. prophecy would avoid that nasty callback hack), and it was by chance, not design, that the test caught that bug. But I think that Unit tests are valuable, even if they don't seem to be sometimes.

lsmith77 · 2014-08-31T15:44:04Z

i am just not a fan of unit tests with complex mocks. any refactoring requires huge refactorings in the tests which just increases the work and reduced certainty. more over .. those unit tests are simply way too hard to comprehend. if you have to mock more than 2-3 methods imho its not worth it and functional tests are the better choice.

dantleech · 2014-08-31T16:08:20Z

I suppose its a matter of approach. When I write functional tests they are usually A-B. The aim of unit tests is to cover every pathway. By writing unit tests with code coverage analysis we can uncover bugs that we didn't expect to be there.

I guess you can do the same with functional tests, but its not as responsive (re. latatency), the scope of functional tests is far more vague and subject to abuse and you have cross-cutting concerns.

I agree that unit tests which are so complicated that they are impossible to maintain are worthless. But this just menas we need to write better unit tests or use better tools. Like I said before I don't think this unit test is impossible to maintain (although some of them might be). It didn't need huge refactoring (it just needed an additional test and a modification of the factory mock),

In general the tests could certainly be improved and made more comprehensive and comprehensible. But so far I am quite pleased with the stability of the query builder, I don't think there have been too many bugs, and I think thats thanks to these tests.

lsmith77 · 2014-08-31T17:33:07Z

lib/Doctrine/ODM/PHPCR/Query/Builder/BuilderConverterPhpcr.php

maybe the method name is not ideal, but is this method just for handling order by?

No iirc, its for handling all dynamic operands, e.g. ->field('a.foo') (i.e. PropertyValueInterface in PHPCR) or ->length('a.foo') (PHPCR LengthInterface).

but why did you assume in the error that the method is being called for an ordering?

in other words. i am still unsure if your changes make sense or just please the unit tests

in my original commit we only checked for nodetype and associations inside the foreach for orderings:
5fa4e2c#diff-0f74c7373c7c111b598f851ef2957d0eR760

I am still not sure where the issue was with that version of the PR

In your version you call ->getField in the ordering walker. But ->getField only applies to the OperandDynamicField node, not to any of the other DynamicOperands (length, lowercase, localname) etc.

The error message should be about not being able to use the nodename (or an association) field as a dynamic operand.

In fact we should probably just simply check to see if the field is mapped to a something which corresponds to a PHPCR property?

yeah .. for ordering it needs to be a "simple name", ie. a property.
for filtering it should additionally not allow node names and associations.
for reference associations i am actually not quite sure if we should not allow equality conditions.

So by having this here we also prevent:

$qb->where()->eq()->field('a.nodenamefield')->literal('foo');

because the user should be doing

$qb->where()->eq()->localname('a')->literal('foo');

I think.

I have pushed an improvement for this.

Fixed validation

lsmith77 · 2014-09-01T07:36:20Z

lib/Doctrine/ODM/PHPCR/Query/Builder/BuilderConverterPhpcr.php

tweaked the exception messages in this method

The DynamicOperand could also be the right-hand side of the operation, would this still be called filtering?

"Cannot use nodename property "%s->%s" in a field operand" ?

btw .. why don't we just convert it to the equivalent sql2 function call?

You mean automatically convert it to a localname operand? I think that would just give people the wrong idea. We could suggest to the developer that they change it in the exception message however.

I guess it might also be possible to add more abstraction, but it would require some thought. The design principle behind this QB was to not making any abstractions as far as possible so as to keep the full power of the PHPCR query system.

lsmith77 · 2014-09-02T17:11:20Z

@dantleech would appreciate some feedback on this

dantleech · 2014-09-02T17:28:11Z

@lsmith77 will try and have a look tomorrow morning before the summercamp :)

dantleech · 2014-09-03T05:47:27Z

lib/Doctrine/ODM/PHPCR/Query/Builder/BuilderConverterPhpcr.php

lsmith77 · 2014-09-04T11:45:14Z

@dantleech can you have another look?

Improve qb validation

dantleech · 2014-09-07T17:13:38Z

@lsmith77 why did you merge this without commenting on my last comment? I don't agree with the code in this PR in its current state.

lsmith77 · 2014-09-07T17:22:12Z

sorry .. I didn't see the comment and you didn't reply to my "have another look" comment.
as for your specific criticism .. honestly we need to get this stuff done, so I am a bit hesitant to "boost the query builders self-awareness" at this point. I do think we need to make error message specific as possible to make it easier for people to actually find the problem in larger code basis, especially as the QB will trigger this stuff at execution and not while configuring the QB.

so in the end .. if you can provide a better solution until tomorrow that doesn't do a huge refactoring, then awesome! otherwise at this point I rather get something stable release soon, which means doing an RC in the next 1-2 days. though before we do the next RC I hope we also fix #557

dantleech · 2014-09-07T17:50:26Z

Ah, I assumed you would get a notification anyway. Its unfortunate as we could have probably worked out a solution.

But as I said I am not happy with this - it only covers the orderBy walker and it duplicates code. If we care about maintaining quality code we should not have incomplete features imo.

As I said I would be happy with this if we remove the validation from the orderBy and have only in its proper place. This already improves the situation without compromising the consistency. If you agree I will make a PR.

lsmith77 · 2014-09-07T17:52:49Z

like I said, if you can do a PR by tomorrow that doesn't contain a large refactoring of the code, then that is awesome!

lsmith77 added 3 commits August 29, 2014 11:36

fixed tests

2416752

typo fix

a56b830

pushed field/orderby validation into the QB

5fa4e2c

lsmith77 added this to the 1.2 milestone Aug 29, 2014

dantleech reviewed Aug 30, 2014
View reviewed changes

Fixed validation

e0ad9f0

lsmith77 reviewed Aug 31, 2014
View reviewed changes

Merge pull request #554 from dantleech/improve_qb_validation

8fc1db1

Fixed validation

lsmith77 force-pushed the improve_qb_validation branch from 1b13202 to 8fc1db1 Compare August 31, 2014 17:33

lsmith77 reviewed Sep 1, 2014
View reviewed changes

dantleech reviewed Sep 3, 2014
View reviewed changes

lib/Doctrine/ODM/PHPCR/Query/Builder/BuilderConverterPhpcr.php Outdated

Copy link

Contributor

dantleech Sep 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

lsmith77 added the in progress label Sep 4, 2014

specifically handle order by error cases in the QB

9bab892

lsmith77 force-pushed the improve_qb_validation branch from 6857611 to 9bab892 Compare September 4, 2014 11:44

lsmith77 added a commit that referenced this pull request Sep 7, 2014

Merge pull request #553 from doctrine/improve_qb_validation

7c6adee

Improve qb validation

lsmith77 merged commit 7c6adee into master Sep 7, 2014

lsmith77 deleted the improve_qb_validation branch September 7, 2014 16:56

lsmith77 removed the in progress label Sep 7, 2014

Uh oh!

Improve qb validation #553

Improve qb validation #553

Uh oh!

Conversation

lsmith77 commented Aug 29, 2014

Uh oh!

dantleech commented Aug 29, 2014

Uh oh!

dbu commented Aug 30, 2014

Uh oh!

dantleech commented Aug 30, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dantleech commented Aug 30, 2014

Uh oh!

dantleech commented Aug 30, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dantleech commented Aug 31, 2014

Uh oh!

lsmith77 commented Aug 31, 2014

Uh oh!

dantleech commented Aug 31, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lsmith77 commented Sep 2, 2014

Uh oh!

dantleech commented Sep 2, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lsmith77 commented Sep 4, 2014

Uh oh!

dantleech commented Sep 7, 2014

Uh oh!

lsmith77 commented Sep 7, 2014

Uh oh!

dantleech commented Sep 7, 2014

Uh oh!

lsmith77 commented Sep 7, 2014

Uh oh!

Uh oh!