Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Specify unique node types via mapping to allow proper outer joins #667

Merged
merged 4 commits into from
Nov 6, 2015

Conversation

sarcher
Copy link
Contributor

@sarcher sarcher commented Nov 1, 2015

Because of the existence of the class and classparents query restrictions on loosely-typed nodes, it is not possible to perform real outer joins. This adds a new document-level mapping property called uniqueNodeType that identifies documents which can safely have those restrictions omitted, allowing for proper joins. (See discussion in #658)

The full scope of this work spans a few different repositories:

Also note that this corrects a problem with Functional\QueryBuilderTest that was introduced in phpcr/phpcr-utils#157 and is currently failing in master. I could have done this in a separate PR but I needed to modify those lines for this one anyway.

@sarcher
Copy link
Contributor Author

sarcher commented Nov 2, 2015

This also requires a change to jackalope-doctrine-dbal, which is why the Travis build is failing currently. In Client.php the following:

$result[] = array('dcr:name' => 'jcr:path' , 'dcr:value' => $row[$columnPrefix . 'path'], 'dcr:selectorName' => $selectorName);
$result[] = array('dcr:name' => 'jcr:score', 'dcr:value' => 0                           , 'dcr:selectorName' => $selectorName);
if (0 === count($qom->getColumns())) {
    $selectorPrefix = null !== $selector->getSelectorName() ? $selectorName . '.' : '';
    $result[] = array('dcr:name' => $selectorPrefix . 'jcr:primaryType', 'dcr:value' => $primaryType, 'dcr:selectorName' => $selectorName);
}

Must become:

$result[] = array('dcr:name' => 'jcr:path' , 'dcr:value' => $row[$columnPrefix . 'path'], 'dcr:selectorName' => $selectorName);
$result[] = array('dcr:name' => 'jcr:score', 'dcr:value' => 0                           , 'dcr:selectorName' => $selectorName);

$selectorPrefix = null !== $selector->getSelectorName() ? $selectorName . '.' : '';
$result[] = array('dcr:name' => $selectorPrefix . 'jcr:primaryType', 'dcr:value' => $primaryType, 'dcr:selectorName' => $selectorName);

The reason is that to support outer joins, Jackrabbit must have the source documents specified, e.g. SELECT a.*, b.* FROM .... Simply doing SELECT * FROM... will fail if b is null (perhaps this is at the transport level and not on the Jackrabbit level, but I wasn't able to isolate it; the failure seems to be in how results are delivered/converted).

To work around that, I simply made selecting each source document explicit if specific columns are not requested. So for example, if you have two document sources, it will submit a query like SELECT a.*, b.* rather than SELECT *. The implication is that the code above will never trigger, since $qom->getColumns() will never equal 0, and the code for hydrating rows relies on either an explicit column selector or for the jcr:primaryType to be present in the row result.

I've tested that particular change with my own code using both Jackrabbit and MySQL and it seems to be fine, and all tests pass. But if anybody has better guidance on how to solve that problem, I'm all ears.

Also, the test related to phpcr/phpcr-utils#157 is failing under PHP 5.3, which appears to be picking up different behavior than 5.4+. I am not sure on that one, but I don't think it is related to this PR.

@sarcher
Copy link
Contributor Author

sarcher commented Nov 2, 2015

This is now complete pending review, with all the pieces in place other than the Jackalope change mentioned above. I edited the PR description a bit so that all of the associated PRs and other info are in there.

use Symfony\Component\Console\Output\OutputInterface;

/**
* Show information about mapped documents
Copy link
Member

Choose a reason for hiding this comment

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

phpdoc needs to be adjusted

@dbu
Copy link
Member

dbu commented Nov 3, 2015

thanks, this is really well done! i will look into the jackalope issue you found. i found some minor code style issues and the like, maybe you could clean those up?

@dbu
Copy link
Member

dbu commented Nov 4, 2015

oh well. there are indeed bugs with outer joins. started to work on them.

@dbu
Copy link
Member

dbu commented Nov 4, 2015

see jackalope/jackalope-jackrabbit#127 and jackalope/jackalope-doctrine-dbal#308

is this PR strictly depending on the fix? it makes of course not much sense to only work when the outer join happens to find something in all cases, but i am not sure how long it will take us to fix jackalope-doctrine-dbal. maybe we should skip the test with jackalope-doctrine-dbal for now and merge the feature so it can make it in the next release which is imminent. as soon as dbal is fixed, we tag a new version and from there on people can use this. we can put a warning in the doc saying what the minimal required dbal version is.

@dbu
Copy link
Member

dbu commented Nov 4, 2015

so i introduced a test for querying with * that failed, and then fix it https://github.com/phpcr/phpcr-api-tests/pull/174/files and jackalope/jackalope-doctrine-dbal#308

can you please try to use that version of jackalope in composer.json (dev-fix-outer-joins as 1.2.5) to see if that solves the problem? if it does, we will merge the jackalope fixes and tag a dot release.

@lsmith77 lsmith77 modified the milestone: 1.3 Nov 4, 2015
@dbu
Copy link
Member

dbu commented Nov 5, 2015

also, this needs to be rebased on master, we seem to have touched something that you also change in this PR

@sarcher
Copy link
Contributor Author

sarcher commented Nov 6, 2015

I've applied the requested style fixes and rebased to the current master (the conflict was from the test I'd fixed in the PR description).

The test suite passes with the current version of Jackalope DBAL and I can't seem to duplicate any of the old problems in Jackrabbit either, so it seems to be okay for now. I will keep working with it but so far the current state here seems to be good.

@sarcher
Copy link
Contributor Author

sarcher commented Nov 6, 2015

We are seeing a related error in Travis with the DBAL now that I wasn't getting locally; that might be one for you, but if not, let me know.

dbu added a commit that referenced this pull request Nov 6, 2015
Specify unique node types via mapping to allow proper outer joins
@dbu dbu merged commit ec40d24 into doctrine:master Nov 6, 2015
@lsmith77 lsmith77 removed the review label Nov 6, 2015
@dbu
Copy link
Member

dbu commented Nov 6, 2015

the same failure happens with master, so its not related to what you do here.

thanks a lot for this contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants