Skip to content

Conversation

@kylekatarnls
Copy link
Contributor

Fix #174

@rebing rebing merged commit b13d490 into rebing:master Jan 22, 2019
@kylekatarnls kylekatarnls deleted the fix-duplicate-table-prefix branch January 22, 2019 07:49
@pushchris
Copy link

pushchris commented Jan 22, 2019

This introduced a bug on all HasMany, MorphMany and HasOne type relations because they were already using the exploded foreign key and now the length is one less.

// If 'HasMany', then add it in the 'with'
elseif(is_a($relation, HasMany::class) || is_a($relation, MorphMany::class) || is_a($relation, HasOne::class))
{
$foreignKey = explode('.', $foreignKey)[2];
if( ! array_key_exists($foreignKey, $field))
{
$field[$foreignKey] = self::FOREIGN_KEY;
}
}

It looks like this can be resolved by taking the end of this variable as well instead of the second index. Replacing:

$foreignKey = explode('.', $foreignKey)[2]

With something like the following:

$foreignKey = explode('.', $foreignKey);
$foreignKey = end($foreignKey);

@kylekatarnls
Copy link
Contributor Author

I did not add [2].

$segments = explode('.', $foreignKey);
$foreignKey = end($segments);

is what I added and what getForeignKey() contains (Laravel source code). So it would be logical to use this pattern everywhere indeed.

@pushchris
Copy link

@kylekatarnls, I should've commented a little better, the code I linked to is later on in the file. It's almost identical to what you changed, but was only being used for certain types of relations. Now that your change is in there, it makes the later code be missing an index and cause those relations to throw an error.

@kylekatarnls
Copy link
Contributor Author

If you can add a unit test for your error, I will try to fix this test keeping duplicate-table fix.

@pushchris
Copy link

pushchris commented Jan 22, 2019

@kylekatarnls I don't fully understand it all, but the error in 1.7.3 was linked to line 200. Essentially, prior to the changes you provided, for HasMany, MorphMany and HasOne relation types, the foreign key (without the parent) was getting put into the $field array.

$field[$foreignKey] = self::FOREIGN_KEY;

So in this situation it doesn't want the full table name, it just wants the key itself so it can be converted later on.

Where things stand right now, your adjustment to the computation of the foreign key shrunk its length by one segment so trying to reach in and get the value at index 2 no longer works. I believe a simple solution is just to get the end (the last segment) instead of trying to reach for a fixed index.

Hopefully this makes a little more sense, again, just reporting an error that came up in our testing and what appears to be the issue. Still more investigation to be done.

I've included a code chunk from SelectFields that I adjusted on our codebase and seems to resolve the issue.

// Get the next parent type, so that 'with' queries could be made
// Both keys for the relation are required (e.g 'id' <-> 'user_id')
$relation = call_user_func([app($parentType->config['model']), $key]);
// Add the foreign key here, if it's a 'belongsTo'/'belongsToMany' relation
if(method_exists($relation, 'getForeignKey'))
{
    $foreignKey = $relation->getForeignKey();
}
else if(method_exists($relation, 'getQualifiedForeignPivotKeyName'))
{
    $foreignKey = $relation->getQualifiedForeignPivotKeyName();
}
else
{
    $foreignKey = $relation->getQualifiedForeignKeyName();
}

$segments = explode('.', $foreignKey);
$foreignKey = end($segments);
$foreignKey = $parentTable ? ($parentTable . '.' . $foreignKey) : $foreignKey;

if(is_a($relation, MorphTo::class))
{
    $foreignKeyType = $relation->getMorphType();
    $foreignKeyType = $parentTable ? ($parentTable . '.' . $foreignKeyType) : $foreignKeyType;

    if( ! in_array($foreignKey, $select))
    {
        $select[] = $foreignKey;
    }

    if( ! in_array($foreignKeyType, $select))
    {
        $select[] = $foreignKeyType;
    }
}
elseif(is_a($relation, BelongsTo::class))
{
    if( ! in_array($foreignKey, $select))
    {
        $select[] = $foreignKey;
    }
}
// If 'HasMany', then add it in the 'with'
elseif(is_a($relation, HasMany::class) || is_a($relation, MorphMany::class) || is_a($relation, HasOne::class))
{

    //
    // Instead of getting a fixed index, get end
    //
    $foreignKey = explode('.', $foreignKey);
    $foreignKey = end($foreignKey);


    if( ! array_key_exists($foreignKey, $field))
    {
        $field[$foreignKey] = self::FOREIGN_KEY;
    }
}

// New parent type, which is the relation
$newParentType = $parentType->getField($key)->config['type'];

self::addAlwaysFields($fieldObject, $field, $parentTable, true);

$with[$key] = self::getSelectableFieldsAndRelations($field, $newParentType, $customQuery, false);

@kylekatarnls
Copy link
Contributor Author

I will test it with my case but I think it's right and you can submit it as a pull-request.

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