[WIP] Query Builder V2 #318

dantleech · 2013-08-13T16:56:29Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	yes
Deprecations?	no
Tests pass?	no
Fixed tickets	N/A
License	MIT
Doc PR	--

Todo

Part 1

Ability to build on existing QB (implementations/equivilents of addSelect, addJoin, etc)
Make selector names mandatory, move selector argument before property_name argument in relevant classes.
Require source (which means selecting mixed documents is not supported by this QB - you can use a PHPCR query for that)
Drop lop and rop. They add about 28 characters to every comparison and are only useful for AutoComplete
Finish off functional tests and PHPCR-ODM integration.

Part 2

Migrate seperate property/selector arguments to compact single argument with dot notation.
Update API method names to reflect ODM (not PHPCR) and to be more intuitive.
Change namespace from Query\QueryBuilder to Query\Builder

lsmith77 · 2013-08-13T18:54:01Z

just to make sure .. you are basing this on PHPCR master where the selector name is no longer optional? phpcr/phpcr#68 .. there are related PRs on phpcr-utils, jackalope*

dantleech · 2013-08-14T08:01:48Z

I will do, I havn't started mapping anything to the PHPCR QOM yet. Although I do imagine that we will always have a selector one way or another.

The question is what should come first:

User must define - first argument:

$qb->select()->column('s', 'property_name');

or selector is optional second argument, but we automatically generate one:

$qb->select('property_name');
$qb->where()->equals()->propertyValue('propety_name', 'a');

So question is, in general: $propertyName before $selectorName or vice-versa?

lsmith77 · 2013-08-14T08:06:53Z

I think should have a default selector name (f.e. source), that by default is used for the FROM.

explicit selector

$qb->select()
    ->column('foobar', null, 'source')
    ->from()->document('Doctrine\Fqn\Foobar', 'source')

equivalent using the implicit selector:

$qb->select()
    ->column('foobar')
    ->from()->document('Doctrine\Fqn\Foobar')

dantleech · 2013-08-14T08:46:01Z

ok, makes sense.

Another question: In my current model I do not include the column name in "column". In an ODM this seems (to me) to be pointless, as indeed does the "column" terminology.

I imagine th euse case is to partially hydrate objects?

If so, then $qb->select()->property('foo')->property('bar') would make more sense.

dbu · 2013-08-14T09:00:05Z

yeah we need to talk about object fields, not phpcr properties (or
columns, which would be the orm equivalent of properties). it should be
the field name, and use the metadata to know what phpcr property that
translates to.

dantleech · 2013-08-18T10:15:46Z

Just to update, this is alot of work (lots of classes for this architecture). Making steady progress. Don't expect this to be done before wednesday however.

dantleech · 2013-08-21T08:18:44Z

I will try and set a day (or two) aside to finish this off this following week.

dantleech · 2013-08-22T08:25:53Z

tests/Doctrine/Tests/ODM/PHPCR/Query/QueryBuilder/BuilderTest.php

+        );
+    }
+
+    public function testApi1()


This is what the API looks like now ..

dantleech · 2013-08-22T08:27:29Z

ok, have finished the complete API (or at least a first prototype). Next up is the QB => Phpcr Query converter.

lsmith77 · 2013-08-22T08:29:45Z

tests/Doctrine/Tests/ODM/PHPCR/Query/QueryBuilder/BuilderTest.php

+            ->end()
+            ->from()
+                ->joinInner()
+                    ->left()->document('foobar', 'a')->end()


like i said before .. this makes no semantical sense. a join isn't a left and a right join. you join onto an existing set of nodes and there you can use a left or right join to determine how missing nodes in the existing set or the set being joined should behave.

This is Phpcr terminology I believe, see the JoinInterdace. We can sort these little things out later, I'm still not 100% on the architecture.

Lukas Kahwe Smith notifications@github.com wrote:

array('from', 'From'),

array('orderBy', 'OrderBy'),

array('select', 'Select'),

);

}

public function testApi1()

{

$this->node

->select()

->property('foobar', 'a')

->property('barfoo', 'a')

->end()

->from()

->joinInner()

->left()->document('foobar', 'a')->end()

like i said before .. this makes no semantical sense. a join isn't a
left and a right join. you join onto an existing set of nodes and there
you can use a left or right join to determine how missing nodes in the
existing set or the set being joined should behave.

Reply to this email directly or view it on GitHub:
https://github.com/doctrine/phpcr-odm/pull/318/files#r5918711

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

Ah you are referring to https://github.com/phpcr/phpcr/blob/master/src/PHPCR/Query/QOM/JoinInterface.php#L32

As someone with a strong SQL background I find this quite confusing and also doesn't match SQL2:
http://www.h2database.com/jcr/grammar.html#join

But I guess there is some sense in following QOM for a QB.

However I still do not understand why this would be preferred over:

->from('foobar', 'a')) ->joinRightInner('barfoo', 'b')

..

->from('foobar', 'a')) ->joinRightInner('barfoo', 'b') ->joinRightInner('barfooca', 'c')

etc.

Well as you see in the API the join also needs a JoinCondition, it needs three objects. We cannot use the "where" to specify the join condition without doing some pretty confusing abstractions I imagine.

The concept does make sense to me as is, especially when you think in terms of Sources not tables. i. e. you select from a Join source.

Hopefully I will get back online soon and finish the prototype off.

Lukas Kahwe Smith notifications@github.com wrote:

array('from', 'From'),

array('orderBy', 'OrderBy'),

array('select', 'Select'),

);

}

public function testApi1()

{

$this->node

->select()

->property('foobar', 'a')

->property('barfoo', 'a')

->end()

->from()

->joinInner()

->left()->document('foobar', 'a')->end()

Ah you are referring to
https://github.com/phpcr/phpcr/blob/master/src/PHPCR/Query/QOM/JoinInterface.php#L32

As someone with a strong SQL background I find this quite confusing and
also doesn't match SQL2:
http://www.h2database.com/jcr/grammar.html#join

But I guess there is some sense in following QOM for a QB.

However I still do not understand why this would be preferred over:

->from('foobar', 'a')) ->joinRightInner('barfoo', 'b')

..

->from('foobar', 'a')) ->joinRightInner('barfoo', 'b') ->joinRightInner('barfooca', 'c')

etc.

Reply to this email directly or view it on GitHub:
https://github.com/doctrine/phpcr-odm/pull/318/files#r5953344

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

i think from()->document('Document') is a bit awkward (and join is the special case, not the standard case). i would prefer from('Document')->end() with optional ->joinSomething after that. the first from needs no condition. only the join with the next document class, so we could provide the join condition there. and i think having both left and right of a join be another join would start to create funny precedence questions that do not exist in SQL2.

i won't veto against the empty from() but i would much prefer to follow lukas' suggestion.

I think it is important to remember that this QB is directly built up from the PHPCR QOM, this is how the QOM does it, and I think abstracting this behavior would give people wrong ideas about what actually happens.

I think there might be some scope for convenience methods, i.e. fromDocument, fromJoin and addJoin[Left|Foo], but essentially I am pretty sure that there is nothing wrong with the current API.

okay, i give in :-)

dantleech · 2013-08-25T12:34:39Z

ok, this is getting closer to being finished. I think one or two more hacking sessions.

Despite that I had another (slightly painful) thought last night that perhaps the current query builder could be improved if we were to implement a flavor of DQL. i.e.

$qb->where($qb->expr()->eq('UpperCase(DocumentName(a.foobar))', 'FOOBAR'));
$qb->createQuery('SELECT sel_1.foobar FROM Document\FooBar sel_1 WHERE DocumentName(sel_1.bar) = 'foo');

That would certainly be much more consistent with the Doctrine way of doing things. Will finish this in anycase.

@lsmith77 how do join conditions work in SQL2 ?

dbu · 2013-08-25T12:42:46Z

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

+    protected function walkSourceJoin(SourceJoin $node)
+    {
+        $left = $this->dispatch($node->getChildOfType('SourceJoinLeft'));
+        $riht = $this->dispatch($node->getChildOfType('SourceJoinRight'));


why riht and not right?

Err, symmetry :) Can change..
On Sun, Aug 25, 2013 at 05:42:47AM -0700, David Buchmann wrote:

In lib/Doctrine/ODM/PHPCR/Query/QueryBuilder/BuilderConverterPhpcr.php:

$meta = $this->mdf->getMetadataFor($node->getDocumentFqn());

$this->selectorMetadata[$node->getSelectorName()] = $meta;

// get the PHPCR Selector

$selector = $this->qomf->selector(

$meta->getNodeType(),

$node->getSelectorName()

);

return $selector;

}

protected function walkSourceJoin(SourceJoin $node)

{

$left = $this->dispatch($node->getChildOfType('SourceJoinLeft'));

$riht = $this->dispatch($node->getChildOfType('SourceJoinRight'));

why riht and not right?

—
Reply to this email directly or [1]view it on GitHub.

References

Visible links

https://github.com/doctrine/phpcr-odm/pull/318/files#r5968273

dbu · 2013-08-25T12:45:36Z

well eventually a DQL for phpcr-odm would be cool, but lets not try to do this here and now, it will be another big pile of work. (defining all the syntax and implementing another parser). i think the orm also has both, a query builder and the DQL.

dantleech · 2013-08-25T13:01:50Z

My concern is that IF we have DQL then the existing QueryBuilder model
would be more valid. This query builder has its pros and cons as does
the other, but the other is more familiar to Doctrine ORM users.

On Sun, Aug 25, 2013 at 05:45:37AM -0700, David Buchmann wrote:

well eventually a DQL for phpcr-odm would be cool, but lets not try to do
this here and now, it will be another big pile of work. (defining all the
syntax and implementing another parser). i think the orm also has both, a
query builder and the DQL.

—
Reply to this email directly or [1]view it on GitHub.

References

Visible links

[WIP] Query Builder V2 #318 (comment)

dantleech · 2013-08-26T15:36:12Z

ok, this /should/ actually work now, I am half way through documenting the classes, although do not currently have sufficient internet to pull in the dependencies required for functional testing.

@dbu or anybody else who uses PHPStorm/Eclipse/other smart IDE, I imagine one of the benefits of this approach is auto-completion, will specifying the return type in the DocBlock suffice? Could somebody check if auto-completion works with this API?

lsmith77 · 2013-08-26T22:25:55Z

when you say "work" you mean in the real world .. but the tests have just not been updated?

dantleech · 2013-08-27T07:56:11Z

I mean "in theory". I havn't had the chance to do functional tresting, so some integrations with ODM may not be there, but in theory the the qb is finished. If anyone wants to test it out that would be great, if not I will finish when I have stable internet connection.

dantleech · 2013-08-31T13:32:16Z

Just an update, this is getting closer all the time, although much more work then I ever imagined.

I have droped lop() and rop() as discussed, added andWhere, orWhere, addOrdering and addSelect, although still no addJoin[Inner|Left|Outer].

dantleech · 2013-09-03T12:37:29Z

ok, this is now ready for review. all the tests pass. Still missing is an "addJoin" type of functionality. I guess such functionality would look something like the following:

$qb->addJoinInner()
  ->right()->document('..')->end()
  ->condition()->equi(..)->end();

Depending on if we keep "left(), right() and condition()" in the "join factory"

Should we abbreviate things like ->propertyValue? $where->eq()->propVal('p', 'foo')->literal('foobar') ? We have kept the doctrine expression builder abbreviations, (eq(), gte, etc).

lsmith77 · 2013-09-03T20:15:38Z

needs a rebase

dantleech · 2013-09-03T21:26:23Z

Rebased.

lsmith77 · 2013-09-03T21:40:43Z

seems like there is an issue in the tests requiring PHP 5.3

but there are also 2 failures on PHP 5.4
https://travis-ci.org/doctrine/phpcr-odm/jobs/10951457

lsmith77 · 2013-09-03T21:44:46Z

what would be the best test case to look at to see the real world use? is there more of a functional test as well?

dantleech · 2013-09-04T06:49:03Z

https://github.com/doctrine/phpcr-odm/pull/318/files#diff-55 for afunctional test, or the BuilderTest for a full exposition of the API. See als the doc PR:doctrine/phpcr-odm-documentation#29

lsmith77 · 2013-09-04T10:12:10Z

thx .. will try to review this today on my way to ezsummercamp.

lsmith77 · 2013-09-04T21:15:12Z

i pushed a few fixes in #323

also one note order for the selector name should always be last, default to 'a' .. and actually maybe the default should be something longer .. like default_selector_name

- Disabled QB join functionality temporarily - Swaped selectorName to last argument where applicable - findBy ordering now accepts array($field => 'asc|desc') as with doctrine ORM

dantleech · 2013-09-13T14:29:09Z

Right.. so I have added the changes from @lsmith77 's PR, I am throwing Exceptions when people try to do joins (joins do work but I am unsure about how they should work, so don't want to roll them out yet) and havesd swaped the selectorName argument around where applicable.

lsmith77 · 2013-09-13T17:00:15Z

there seems to be a PHP 5.3 issue in the test suite:
https://travis-ci.org/doctrine/phpcr-odm/jobs/11325232#L114

as for the doctrine dbal failures, i just pushed a fixed to master that will hopefully fix this so that you then just need to rebase on master.

btw once we have this PR merged, the next step is preparing for the PHPCR BC breaks:
https://github.com/phpcr/phpcr/pull/68/files

lsmith77 · 2013-09-13T17:02:02Z

bin/phpcrodm.php

@@ -48,6 +48,7 @@
    new \PHPCR\Util\Console\Command\WorkspaceQueryCommand(),
    new \PHPCR\Util\Console\Command\NodeTypeRegisterCommand(),
    new \Doctrine\ODM\PHPCR\Tools\Console\Command\RegisterSystemNodeTypesCommand(),
+    new \Doctrine\ODM\PHPCR\Tools\Console\Command\DumpQueryBuilderReferenceCommand(),


i guess you decided in the end to include it? fine for me

yeah.. I might modify it to make it dump the rst reference, or delete it yet. In any case let's not bootstrap it in the bundle.

Lukas Kahwe Smith notifications@github.com wrote:

@@ -48,6 +48,7 @@
new \PHPCR\Util\Console\Command\WorkspaceQueryCommand(),
new \PHPCR\Util\Console\Command\NodeTypeRegisterCommand(),
new
\Doctrine\ODM\PHPCR\Tools\Console\Command\RegisterSystemNodeTypesCommand(),

new
\Doctrine\ODM\PHPCR\Tools\Console\Command\DumpQueryBuilderReferenceCommand(),

i guess you decided in the end to include it? fine for me

Reply to this email directly or view it on GitHub:
https://github.com/doctrine/phpcr-odm/pull/318/files#r6352958

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

lsmith77 · 2013-09-13T17:15:42Z

went through the code and made a few comments. one more thing I just merged http://github.com/doctrine/phpcr-odm/pull/322 which aimed to get rid of all use of exceptions outside of the phpcr odm namespace

dantleech · 2013-09-13T19:32:23Z

cheers lukas.. I have added PHPCR exceptions for BadMethodCall and Runtime, added @IgnoreAnnotation and addressed the other points.

lsmith77 · 2013-09-13T20:33:45Z

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

@@ -0,0 +1,109 @@
+<?phph


lsmith77 · 2013-09-13T20:34:28Z

getting there .. a small typo and the php 5.3 issue left ..

- Added Runtime and BadMethodCall Exceptions - Added @IgnoreAnnotation - Removed extra lines

lsmith77 · 2013-09-13T21:12:28Z

there is now only a minor issue left with doctrine dbal

[WIP] Query Builder V2

lsmith77 · 2013-09-15T17:33:37Z

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

+    /**
+     * Descendant document constraint:
+     *
+     *   $qb->where()->descendantDocument('/ancestor/path', 'sel_1')


noticed that when you renamed the methods you missed the comments

yeah, I noticed that too :) Will be fixed in #326

Lukas Kahwe Smith notifications@github.com wrote:

*

\* @param string $path

\* @param string $selectorName

*

\* @factoryMethod

\* @return ConstraintSame

*/

public function same($path, $selectorName)

{

return $this->addChild(new ConstraintSame($this,
$selectorName, $path));

}

/**

\* Descendant document constraint:

*

\* $qb->where()->descendantDocument('/ancestor/path', 'sel_1')

noticed that when you renamed the methods you missed the comments

Reply to this email directly or view it on GitHub:
https://github.com/doctrine/phpcr-odm/pull/318/files#r6367502

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

dbu · 2013-09-16T09:12:24Z

lib/Doctrine/ODM/PHPCR/DocumentManager.php

        $qm = $this->session->getWorkspace()->getQueryManager();
+        $qomf = $qm->getQOMFactory();
+        $converter = new BuilderConverterPhpcr($this, $qomf);
+        $builder->setConverter($converter);


is the converter an optional dependency of the querybuilder? otherwise i would prefer constructor injection to make sure the builder always has a converter with it...

Constructor injection would make more sense I guess, at the slight cost of having to mock a Converter in the tests.

i think that is a cost that is not too high :-)

But then, I guess this class will be used quite alot in tests (mocking this class would be a complete pain). I was going to suggest also that we might want to set different converters at runtime, although, short of using this QB as an abstract interface for the ORM, not sure what the use case would be.

to use different converters, you could also just instantiate new query
builders.

dantleech reviewed Aug 22, 2013
View reviewed changes

lsmith77 reviewed Aug 22, 2013
View reviewed changes

dbu reviewed Aug 25, 2013
View reviewed changes

dbu mentioned this pull request Aug 26, 2013

[WIP] Ideas for query builder join #285

Closed

lsmith77 mentioned this pull request Sep 4, 2013

Qbv2 #323

Closed

dantleech closed this Sep 4, 2013

dantleech reopened this Sep 4, 2013

dantleech and others added 3 commits September 13, 2013 15:10

Numerous changes

f905012

- Disabled QB join functionality temporarily - Swaped selectorName to last argument where applicable - findBy ordering now accepts array($field => 'asc|desc') as with doctrine ORM

Added test fixes (from lsmith PR)

b0f7fd6

ensure there is a property

16c7646

lsmith77 reviewed Sep 13, 2013
View reviewed changes

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

@@ -0,0 +1,109 @@

<?phph

Copy link

Member

lsmith77 Sep 13, 2013

Choose a reason for hiding this comment

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

small typo

Code Review Fixes

3b8334b

- Added Runtime and BadMethodCall Exceptions - Added @IgnoreAnnotation - Removed extra lines

Fixed doctrine-dbal failure

2d7be10

lsmith77 added a commit that referenced this pull request Sep 13, 2013

Merge pull request #318 from dantleech/qbv2

f21d35e

[WIP] Query Builder V2

lsmith77 merged commit f21d35e into doctrine:master Sep 13, 2013

lsmith77 reviewed Sep 15, 2013
View reviewed changes

lsmith77 mentioned this pull request Sep 15, 2013

fix filters sonata-project/SonataDoctrinePhpcrAdminBundle#171

Closed

dbu reviewed Sep 16, 2013
View reviewed changes

dbu mentioned this pull request Sep 16, 2013

QueryBuilder update to ensure non null selector names #300

Closed

[WIP] Query Builder V2 #318

[WIP] Query Builder V2 #318

Conversation

dantleech commented Aug 13, 2013

Todo

Part 1

Part 2

lsmith77 commented Aug 13, 2013

dantleech commented Aug 14, 2013

lsmith77 commented Aug 14, 2013

dantleech commented Aug 14, 2013

dbu commented Aug 14, 2013

dantleech commented Aug 18, 2013

dantleech commented Aug 21, 2013

Choose a reason for hiding this comment

dantleech commented Aug 22, 2013

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

dantleech commented Aug 25, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu commented Aug 25, 2013

dantleech commented Aug 25, 2013

dantleech commented Aug 26, 2013

lsmith77 commented Aug 26, 2013

dantleech commented Aug 27, 2013

dantleech commented Aug 31, 2013

dantleech commented Sep 3, 2013

lsmith77 commented Sep 3, 2013

dantleech commented Sep 3, 2013

lsmith77 commented Sep 3, 2013

lsmith77 commented Sep 3, 2013

dantleech commented Sep 4, 2013

lsmith77 commented Sep 4, 2013

lsmith77 commented Sep 4, 2013

dantleech commented Sep 13, 2013

lsmith77 commented Sep 13, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsmith77 commented Sep 13, 2013

dantleech commented Sep 13, 2013

Choose a reason for hiding this comment

lsmith77 commented Sep 13, 2013

lsmith77 commented Sep 13, 2013

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