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

Proving bug with Deep nested many_many relations #3839

Closed

Conversation

dhensby
Copy link
Contributor

@dhensby dhensby commented Jan 30, 2015

When adding a filter to a many_many with a shared inheritance, the FROM table is removed and added as a LEFT JOIN which causes a syntax error.

This means $dataList->filter('ManyManyRel.ID', array(1,2)) doesn't work.

@dhensby
Copy link
Contributor Author

dhensby commented Jan 30, 2015

Ok, So I've fixed an issue where the FROM clause was removed from the DataQuery.

There remains a problem with the actual filter due to the collision of the table names and thus the WHERE clause in the query is ambiguous and doesn't filter on the link table (where I feel it should) and instead filters on the joined table (which is a collision with the FROM table).

@dhensby dhensby force-pushed the pulls/many-many-apply-relation-bug branch from 36c1fb7 to 7a3cb15 Compare January 30, 2015 14:49
@dhensby
Copy link
Contributor Author

dhensby commented Feb 9, 2015

@tractorcow I'd like you're thoughts on whether your ORM work will have fixed this problem?

@tractorcow
Copy link
Contributor

I doubt it, I don't think I've tested this specific use case.

@tractorcow
Copy link
Contributor

1) DataObjectTest::testBelongsTo
Couldn't run query: 
INSERT INTO "DataObjectTest_Company" ("Created") VALUES (NOW()) 
Table 'ss_tmpdb2069596.DataObjectTest_Company' doesn't exist

You need to ensure you add any class with TestOnly interface to extraDataObjects variable.

@dhensby dhensby force-pushed the pulls/many-many-apply-relation-bug branch 2 times, most recently from 340c1d4 to 5f58c8a Compare February 12, 2015 13:04
@dhensby
Copy link
Contributor Author

dhensby commented Feb 12, 2015

thanks @tractorcow added that and now looks like the real reason for failing is happening

@tractorcow
Copy link
Contributor

Did you want to fix these tests? :D I think this should be merged soon.

@tractorcow tractorcow added this to the 3.1.14 milestone May 21, 2015
@dhensby dhensby force-pushed the pulls/many-many-apply-relation-bug branch from 5f58c8a to 8e669f7 Compare June 14, 2015 22:57
@dhensby dhensby force-pushed the pulls/many-many-apply-relation-bug branch from 8e669f7 to e26a323 Compare June 24, 2015 09:43
@dhensby
Copy link
Contributor Author

dhensby commented Jun 24, 2015

Ok, the tests aren't broken, it's the core itself.

For some reason the query builder is joining on the wrong column for the link table.

@sminnee
Copy link
Member

sminnee commented Apr 19, 2016

@dhensby any enthusiasm for remedying whatever is broken in these?

@dhensby
Copy link
Contributor Author

dhensby commented Apr 23, 2016

Yep, is a pretty serious bug, it's all about time for free at the moment... :(

@tractorcow
Copy link
Contributor

This API has changed a lot in later branches... you probably don't want to leave this too long before the branches diverge. :P

I would close and re-open on 3.3 if I were you.

@kinglozzer
Copy link
Member

Just ran into this one, nasty 😢 . Don’t suppose you’ve had a chance to look at this yet @dhensby?

if (!$this->query->isJoinedTo($componentBaseClass)) {
$this->query->addLeftJoin($componentBaseClass,
"\"$relationTable\".\"$componentField\" = \"$componentBaseClass\".\"ID\"");
}
if(ClassInfo::hasTable($componentClass)) {
Copy link
Member

Choose a reason for hiding this comment

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

My example also needed !$this->query->isJoinedTo($componentClass) added to this condition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm.. maybe you'd need to do that if the field that needed to be filtered on was not on the componentBaseClass?

@dhensby dhensby force-pushed the pulls/many-many-apply-relation-bug branch 3 times, most recently from 83e15c8 to 82ba66f Compare May 10, 2016 13:35
@dhensby
Copy link
Contributor Author

dhensby commented May 10, 2016

I've gone and refreshed my memory with this issue. There are two problems here which I've now resolved, though perhaps not completely and in a very hacky and non-semver way.

  1. Empty FROM clause when joining to the same table as the base table.

I think this has been resolved, though @kinglozzer doesn't seem to agree. Because DataQuery stores the joins and FROM clauses in the same keyed array, a join would just override the from and it'd be empty. This is fixed by checking that the table isn't already "joined" before adding the join.

  1. Self referencing relations fail

If you have a relationship between the same ancestor classes (eg: SomePage many-many SomeOtherPage) it will select base columns from SiteTree, try to join SiteTree again (but doesn't because of fix above) and then does a WHERE on SiteTree.

The generated SQL for SomePage::get()->filter('RelatedPages', array(1,2,3)); is something like:

SELECT "SiteTree".*
FROM "SiteTree"
INNER JOIN "SomePage_RelatedPages" ON "SomePage_RelatedPages"."SomePageID" = "SiteTree"."ID"
WHERE "SiteTree"."ID" IN (1,2,3) AND "SiteTree"."ClassName" = 'SomePage';

This fails to return results as the join is not done properly and should be something closer to:

SELECT "SomePage".*
FROM "SiteTree" AS "SomePage"
INNER JOIN "SomePage_RelatedPages" ON "SomePage_RelatedPages"."SomePageID" = "SomePage"."ID"
INNER JOIN "SiteTree" AS "RelatedPage" ON "SomePage_RelatedPages"."RelatedPageID" = "RelatedPage"."ID"
WHERE "RelatedPage"."ID" IN (1,2,3) AND "SomePage"."ClassName" = 'SomePage'

This is a difficult problem to solve as the SearchFilter needs knowledge that the relation has a common inheritance and the where clause will have been aliased out.

@dhensby dhensby force-pushed the pulls/many-many-apply-relation-bug branch 4 times, most recently from a2dcfe5 to 2fd5cad Compare May 10, 2016 15:55
dhensby and others added 5 commits May 11, 2016 20:34
When adding a filter to a many_many with a shared inheritance, the FROM table is removed and added as a LEFT JOIN which causes a syntax error.

This means `$dataList->filter('ManyManyRel.ID', array(1,2))` doesn't work.
@dhensby dhensby force-pushed the pulls/many-many-apply-relation-bug branch from 99f550f to 2b15eb6 Compare May 11, 2016 19:35
@dhensby dhensby closed this May 12, 2016
@dhensby dhensby deleted the pulls/many-many-apply-relation-bug branch May 12, 2016 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants