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

Fixes JOIN duplication issue when default join type equals given one (issue #373) #641

Merged
merged 1 commit into from Mar 31, 2013

Conversation

Projects
None yet
6 participants
@arvenil
Copy link
Contributor

commented Mar 30, 2013

This fixes issue #373.

Instead of checking object identity I switched to check if Joins equals.

I've provided fix for Criteria and ModelCriteria. For ModelCriteria I also added whole test set for checking if joins doesn't duplicate (similar to tests for Criteria).

if (!in_array($join, $this->joins)) { // compare equality, NOT identity
$isAlreadyAdded = false;
foreach ($this->joins as $alreadyAddedJoin) {
if ($join->equals($alreadyAddedJoin)) {

This comment has been minimized.

Copy link
@staabm

staabm Mar 31, 2013

Member

You should swap the arguments to prevent null warnings/errors. I am not sure how this method should handle null but definitly without an warning/error emitted ;)

This comment has been minimized.

Copy link
@arvenil

arvenil Mar 31, 2013

Author Contributor

I don't understand about which exactly part you are talking? Is it $join? $join can't be null because method definition prevents passing null (it is public function addJoinObject(Join $join) and not public function addJoinObject(Join $join = null))

This comment has been minimized.

Copy link
@staabm

staabm Mar 31, 2013

Member

But one could call it using null as parameter.. A method can be called with an arbirtary value no matter how the default is defined

This comment has been minimized.

Copy link
@arvenil

arvenil Mar 31, 2013

Author Contributor

You can't pass null to method which has definition addJoinObject(Join $join).
You will get fatal error Catchable fatal error: Argument 1 passed to addJoinObject() must be an instance of Join, null given.
So it doesn't make sense to check for null again inside of method - such code will never execute.

If in method definition you strictly declare what type parameter should have then the only way to allow pass null is to declare it like this addJoinObject(Join $join = null).

http://php.net/manual/en/language.oop5.typehinting.php

PHP 5 introduces type hinting. Functions are now able to force parameters to be objects (by specifying the name of the class in the function prototype), interfaces, arrays (since PHP 5.1) or callable (since PHP 5.4). However, if NULL is used as the default parameter value, it will be allowed as an argument for any later call.

This comment has been minimized.

Copy link
@staabm

staabm Mar 31, 2013

Member

OMG mixed it up with java, you right..

if (!in_array($join, $this->joins)) { // compare equality, NOT identity
$isAlreadyAdded = false;
foreach ($this->joins as $alreadyAddedJoin) {
if ($join->equals($alreadyAddedJoin)) {

This comment has been minimized.

Copy link
@staabm

staabm Mar 31, 2013

Member

Same here

@staabm

This comment has been minimized.

Copy link
Member

commented Mar 31, 2013

LGTM

willdurand added a commit that referenced this pull request Mar 31, 2013

Merge pull request #641 from arvenil/fix-issue-373-join-duplication
Fixes JOIN duplication issue when default join type equals given one (issue #373)

@willdurand willdurand merged commit 808bc84 into propelorm:master Mar 31, 2013

1 check passed

default The Travis build passed
Details
@willdurand

This comment has been minimized.

Copy link
Member

commented Mar 31, 2013

Thanks!

@matthewpeach

This comment has been minimized.

Copy link

commented Feb 18, 2014

@arvenil @willdurand I've just upgraded to Propel 1.7.0, and this has broken our multiple column joins. Here's an example:

$q = ExampleModelQuery::create();

$q->addJoin(array(ExampleModelPeer::COLUMN_A, ExampleModelPeer::COLUMN_B), array(RelatedModelPeer::COLUMN_A, RelatedModelPeer::COLUMN_B), Criteria::INNER_JOIN);

var_dump(count($q->getJoins()));

> int(1)

$q->addJoin(array(RelatedModelPeer:COLUMN_C, RelatedModelPeer::COLUMN_D), array(AnotherRelatedModelPeer::COLUMN_A, AnotherRelatedModelPeer::COLUMN_B), Criteria::INNER_JOIN);

var_dump(count($q->getJoins()));

> int(1)

So, we're adding two quite different joins, but only one is added to the criteria. This is a huge problem for obvious reasons.

This is happening because we compare the joins using Join->equals(), here:

public function equals($join)
{
    return $join !== null
            && $join instanceof Join
            && $this->getJoinType() == $join->getJoinType()
            && $this->getConditions() == $join->getConditions();
}

if we step in to getConditions() we have this:

public function getConditions()
{
    $conditions = array();
    for ($i = 0; $i < $this->count; $i++) {

        $conditions[] = array(
            'left'     => $this->getLeftColumn($i),
            'operator' => $this->getOperator($i),
            'right'    => $this->getRightColumn($i)
        );
    }

    return $conditions;
}

When a join has been added with multiple conditions as described, $this->count is zero and this returns an empty array. Therefore, the comparison returns true, and we decide not to add the subsequent join.

With a regular one-to-one join, this works as expected because the join is added with addExplicitCondition which sets up the left/right columns and increments the condition count.

I'll purposefully leave this comment open-ended as I'm not 100% sure on the correct solution here. I.e. Does the root problem lie with the adding of the join, in the Join equals function in the multiple-column join creation, or somewhere else?

@matthewpeach

This comment has been minimized.

Copy link

commented Feb 18, 2014

Also, it doesn't look like this change has made it in to Propel 2. Is this because the problem was addressed in another way there, or has it just not been done?

@jaugustin

This comment has been minimized.

Copy link
Member

commented Feb 18, 2014

@matthewpeach all patches should be ported to propel2 if the fixed issue is also present in propel2 maybe we didn't port it yet.

I think you should open a new issue with your problem and if possible provide a little test case that fail with this code.

@stevleibelt

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2014

@matthewpeach

After debugging more than a day, i'm on your side. The check of equality is not working. I vote for adding a check of "getClauses()".

For my case, we are working with (deprecated) "addMultipleJoin" calls and the second one differs in the "clauses" but not in "join_type" nor "conditions" (empty arrays).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.