Skip to content

2.1.x#280

Merged
beberlei merged 4 commits intodoctrine:2.1.xfrom
datasyntax:DC-1648
Mar 14, 2012
Merged

2.1.x#280
beberlei merged 4 commits intodoctrine:2.1.xfrom
datasyntax:DC-1648

Conversation

@r0ssIV
Copy link
Contributor

@r0ssIV r0ssIV commented Feb 13, 2012

Make Reverse Engineering to support Primary Keys as Foreign Keys.

The origin of the problem:

While parsing database Doctrine2 creates 2 separate arrays - "field mappings" and "association mappings".
If the field is within table foreign keys, it goes to "association mappings", otherwise it goes to "field mappings".

With primary keys as foreign keys Doctrine2 behaves incorrectly - it adds it both to "field mappings" and "association mappings", creating a nasty MappingException::duplicateFieldMapping

Reworked the code so it adds it only to the "association mappings", creating a OneToOne mapping type.

I wonder why so few attention is payed to reverse engineering in the project.

When you introspect database it only creates unidirectional mappings, so you have to manually create an
inverse in other entities.

In Doctrine 1.x it was much-much better.
Reverse engineering is not evil. Is a thing really very important in many situations when you a have a huge existing database structure.

@beberlei
Copy link
Member

Why are you creating OneToOne Mappings? ManyToOne is the better choice i think.

I agree the reveres engineering could be better.

Copy link
Member

Choose a reason for hiding this comment

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

Please remove this and use ManyToOne instead of OneToOne.

Copy link
Member

Choose a reason for hiding this comment

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

Why do you still need this? I think it can be removed, because of DatabaseDriver.php#L306

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello, sorry I am in a heavy development process, so cannot react very quickly.

In your current code (which did not support pk as fk in reverse -engineering) you checked if ONE-TO-ONE assoc consisted just of 1 column, and if yes, you set the property "unique" (which is quite correct) - see above (lines 886-888).

Now in my code, I added one more additional check - if ONE-TO-ONE assoc consists of 1 column AND it is also a primary key, we do not set the unique property, because it is not needed here (PK is always unique). I did not want to have both Id and Unique present for one field.

Copy link
Member

Choose a reason for hiding this comment

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

But it would still be required to ensure data-consistency. If you have a combined primary key and one field is additionally OneToOne, then there should be a unique key on it by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, bcz of DatabaseDriver.php#L307

One-To-One mapping is created only if all columns that make a combined primary key, are used in association.
In your case it would create Many-To-One, and consistency would be ok. :)

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, now i get it. Yes then the line is necessary obviously :-)

@r0ssIV
Copy link
Contributor Author

r0ssIV commented Feb 13, 2012

Why are you creating OneToOne Mappings? ManyToOne is the better choice i think.

The OneToOne mapping is used because this is "a primary key that is in the same time a foreign key".
Primary keys cannot have duplicates, that's why it is not "Many" side.

You can of course use ManyToOne with "unique" parameter, if you want, but it is a little weird-looking.

Also in your code ManyToOne is a wrapper over OneToOne

/**
 * Adds a many-to-one mapping.
 *
 * @param array $mapping The mapping.
 */
public function mapManyToOne(array $mapping)
{
    $mapping['type'] = self::MANY_TO_ONE;
    // A many-to-one mapping is essentially a one-one backreference
    $mapping = $this->_validateAndCompleteOneToOneMapping($mapping);
    $this->_storeAssociationMapping($mapping);
}

So maybe to get rid of OneToOne mappings at all and use ManyToOne with unique parameter on all Joined Columns?

@beberlei
Copy link
Member

Ah we are getting somewhere, ManyToOne or OneToOne depends on the following cases:

  1. OneToOne when foreign key as primary key only spans one column (User => Adress with user_id as PK for example)
  2. ManyToOne when foreign key as pk spans multiple columns.

You could optimize the code in this direction.

Choose a reason for hiding this comment

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

Missing a space after if

@r0ssIV
Copy link
Contributor Author

r0ssIV commented Feb 13, 2012

This makes a cocktail in my head :)

One/Multiple columns in a foreign key is determined by JoinedColumns directive:

  Multiple:
  @ORM\JoinColumns({
    @ORM\JoinColumn(name="FirstPKField", referencedColumnName="remoteRefOfFirstPKField")
    @ORM\JoinColumn(name="SecondPKField", referencedColumnName="secondRefOfSecondPKField")
  })

 One:
 @ORM\JoinColumns({
   @ORM\JoinColumn(name="SinglePKField", referencedColumnName="remoteRefOfSinglePKField")
  })

OneToOne and ManyToOne is the association mapping.

OneToOne means that there is only one record on our side (like only one address is available per user).
ManyToOne means many records on our side (like many addresses per user)

Both associations can be made with 1 joined column, or with N joined columns - i mean number of joined columns do not have any influence on whether it is OneToOne or ManyToOne mapping.

@beberlei
Copy link
Member

Ah yes, but you can catch this case when the foreign keys point to the same table. So the condition for onetoone is:

Primary Key only has foreign keys pointing to a single table.

For ManyToOne its:

  • Primary Key has at least one non-foreign key column
  • Primary Key has only foreign keys, but they point to at least 2 different tables.

@r0ssIV
Copy link
Contributor Author

r0ssIV commented Feb 13, 2012

Primary Key has at least one non-foreign key column

Update: In MySQL this is doable
See discussion below.

Primary Key has only foreign keys, but they point to at least 2 different tables.

In that case we would have 2 separate OneToOne associations.

create table table3
(
ssn type,
firstName type,
lastName type,
col1 type,
primary key (ssn, firstName, lastName),
foreign key (ssn, firstName, lastName) references table1 (ssn, firstName, lastName), <-- first 1-to-1
foreign key (ssn, firstName, lastName) references table2 (ssn, firstName, lastName) <-- second 1-to-1
)

@beberlei
Copy link
Member

Huh? What about CREATE TABLE ProductATtributes (product_id INT, attribute_name VARCHAR, attribute_value VARCHAR, PRIMARY KEY(product_id, attribute_name)) that is 1 fk and 1 "normal" column on the table.

@r0ssIV
Copy link
Contributor Author

r0ssIV commented Feb 13, 2012

In your CREATE TABLE statement you did not mention any foreign keys, only primary key.
Please be more specific, so it's not guessing.

@r0ssIV
Copy link
Contributor Author

r0ssIV commented Feb 13, 2012

Let's look at such example:

PRIMARY KEY(product_id, attribute_name)
FOREIGN KEY(product_id, attribute_name) references TableMoreDesc(product_id, attribute_name)
FOREIGN KEY(product_id) references Products(product_id)

My code:

        //Here we need to check if $cols are the same as $primaryKeyColums
        if (!array_diff($cols,$primaryKeyColumns)) {
            $metadata->mapOneToOne($associationMapping);
        }
        else {
            $metadata->mapManyToOne($associationMapping);
        }
  1. For the first FOREIGN KEY !array_diff($cols,$primaryKeyColumns) would match and create ONE-TO-ONE
  2. For the second FOREIGN KEY it would create MANY-TO-ONE, because arrays don't match

Is this what you wanted to know?

@r0ssIV
Copy link
Contributor Author

r0ssIV commented Feb 15, 2012

Any updates on this?

1 similar comment
@r0ssIV
Copy link
Contributor Author

r0ssIV commented Feb 16, 2012

Any updates on this?

@beberlei
Copy link
Member

beberlei commented Mar 3, 2012

Sorry, i was busy the last weeks so i couldn't catch up.

'product_id' is the foreign key in my example. Ok good that your code supports this. Let me review then i'll merge.

Copy link
Member

Choose a reason for hiding this comment

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

please squash the lines and put } else { here.

beberlei added a commit that referenced this pull request Mar 14, 2012
@beberlei beberlei merged commit cab10eb into doctrine:2.1.x Mar 14, 2012
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.

4 participants