-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
DDC-671: Table aliases wrongly computed with YAML + mappedsuperclasses and joins #5181
Comments
|
Comment created by shurakai: A testcase is now available in my repo: http://github.com/Shurakai/doctrine2/tree/[DDC-671](http://www.doctrine-project.org/jira/browse/DDC-671) |
Comment created by @jwage: According to the docs: http://www.doctrine-project.org/projects/orm/2.0/docs/reference/inheritance-mapping/en#inheritance-mapping "A mapped superclass cannot be an entity, it is not queryable and persistent relationships defined by a mapped superclass must be unidirectional. For further support of inheritance, the single or joined table inheritance features have to be used." |
Comment created by @beberlei: I agree with Jon, this is not an issue. I dont understand at all what this has to do with XML, YAML or Annotations? |
Comment created by shurakai: Jonathan: Thanks for your comment, but that does not seem to fit the problem. As you can see within my testcase, the mapped superclass defines a relationship (manyToOne) which is unidirectional. Benjamin: The problem is, that this specific feature works with annotations, but not with YAML. |
Comment created by @jwage: When I look at your test I see you have association on the mapper super class side as well as on Directory side. That looks bidirectional, no? |
Comment created by @beberlei: @jwage he is specifiying a directory or page on the OneToMany side, so this looks ok. @christian: Why do you specified a table attribute on the mapped superclass? That doesnt make sense. The mapping looks correct, can you please show the error message? ADditionally can you link the correct line in SQL Walker again? The lines moved, so i cant verify what 718 is anymore, it changed. |
Comment created by shurakai: Benjamin: I removed the table attribute on the mapped superclass. I don't know why it was in there. Some more information: The parentDirectory is currently in the mappedSuperclass. However, if you try to join it this way, the source entity used in SqlWalker::walkJoinVariableDeclarationwill look like this:
Which is actually the mappingSuperclass. That said, Doctrine will try to get an alias for AbstractContentItem (l. 730 SqlWalker), which does not exist at that point. Thus, it will create a new one (in my case: a2). You can see the result in the query below. Note that a2 is only used, but never declared. {quote}SELECT c0_.id AS id0, c0_.extension AS extension1, c0_.parent_directory_id AS parent_directory_id2 FROM core_content_pages c0_ INNER JOIN core_content_directories c1_ ON a2_.parent_directory_id = c1_.id WHERE (c1_.url = ?) AND (c0_.extension = ?){quote} It might be that l. 730 currently passes a wrong argument (mappedSuperclass instead of entity-in-use) or that there is some error in the yaml driver? As I'm unfortunately not too familiar with this process, I need your oppinions to fix this issue. It would be interesting to know where the
|
Comment created by @beberlei: This is actually wrong for Annotations and XML Driver also, the problem is shown by this assertions and test-failures in combination with your test:
Failure.
|
Comment created by @beberlei: This works with annotations because the annotation reader also reads properties from parent classes. For YAML and XML this did not work and required a little change in the ClassMetadataFactory method that copied over inherited metadata. |
Issue was closed with resolution "Fixed" |
1. When inheriting a to-one association from a mapped superclass, update the `sourceEntity` class name to the current class only when the association is actually _declared_ in the mapped superclass. 2. Reject association types that are not allowed on mapped superclasses only when they are actually _declared_ in a mapped superclass, not when inherited from parent classes. Currently, when a many-to-one association is inherited from a `MappedSuperclass`, mapping information will be updated so that the association has the current (inheriting) class as the source entity. https://github.com/doctrine/orm/blob/2138cc93834cfae9cd3f86c991fa051a3129b693/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php#L384-L393 This was added in 7dc8ef1 for [DDC-671](doctrine#5181). The reason for this is that a mapped superclass is not an entity itself and has no table. So, in the database, associations can only be from the inheriting entities' tables towards the referred-to target. This is also the reason for the limitation that only to-one associations may be added in mapped superclasses, since for those the database foreign key can be placed on the table(s) of the inheriting entities (and there may be more than one child class). Neither the decision to update the `sourceEntity` nor the validation check should be based on `$parent->isMappedSuperclass`. This only works in the simple case where the class hierarchy is `Mapped Superclass → Entity`. The check is wrong when we have an inheritance hierarchy set up and the class hierarchy is `Base Entity → Mapped Superclass → Child Entity`. Bug 1: The association should keep the root entity as the source. After all, in a JTI, the root table will contain the foreign key, and we need to base joins on that table when traversing `FROM LeafClass l JOIN l.target`. Bug 2: Do not reject the to-many association declared in the base class. It is ok to have the reverse (owning) side point back to the base entity, as it would be if there were no mapped superclasses at all. The mapped superclass does not declare, add or otherwise interfere with the to-many association at all. Base the decision to change the `sourceEntity` on `$mapping['inherited']` being set. This field points to the topmost _parent entity_ class in the ancestry tree where the relationship mapping appears for the first time. When it is not set, the current class is the first _entity_ class in the hierarchy with that association. Since we are inheriting the relation, it must have been added in a mapped superclass above, but was not yet present in the nearest parent entity class. In that case, it may only be a to-one association and the source entity needs to be updated. (See doctrine#10396 for a clarification of the semantics of `inherited`.) Here is a simplified example of the class hierarchy. See the two tests added for more details – one is for checking the correct usage of a to-one association against/with the base class in JTI. The other is to test that a to-many association on the base class is not rejected. I am sure that there are other tests that (still) cover the update of `sourceEntity` is happening. ```php /** * @entity */ class AssociationTarget { /** * @column(type="integer") * @id * @GeneratedValue */ public $id; } /** * @entity * @InheritanceType("JOINED") * @DiscriminatorColumn(name="discriminator", type="string") * @DiscriminatorMap({"1" = "BaseClass", "2" = "LeafClass"}) */ class BaseClass { /** * @column(type="integer") * @id * @GeneratedValue */ public $id; /** * @manytoone(targetEntity="AssociationTarget") */ public $target; } /** * @MappedSuperclass */ class MediumSuperclass extends BaseClass { } /** * @entity */ class LeafClass extends MediumSuperclass { } ``` When querying `FROM LeafClass l`, it should be possible to `JOIN l.target`. This currently leads to an SQL error because the SQL join will be made via `LeafClass.target_id` instead of `BaseClass.target_id`. `LeafClass` is considered the `sourceEntity` for the association – which is wrong–, and so the foreign key field is expected to be in the `LeafClass` table (using JTI here). Fixes doctrine#5998, fixes doctrine#7825. I have removed the abstract entity class, since it is not relevant for the issue and took the discussion off course. Also, the discriminator map now contains all classes. Added the second variant of the bug, namely that a to-many association would wrongly be rejected in the same situation.
…le of an inheritance hierarchy This fixes two closely related bugs. 1. When inheriting a to-one association from a mapped superclass, update the `sourceEntity` class name to the current class only when the association is actually _declared_ in the mapped superclass. 2. Reject association types that are not allowed on mapped superclasses only when they are actually _declared_ in a mapped superclass, not when inherited from parent classes. Currently, when a many-to-one association is inherited from a `MappedSuperclass`, mapping information will be updated so that the association has the current (inheriting) class as the source entity. https://github.com/doctrine/orm/blob/2138cc93834cfae9cd3f86c991fa051a3129b693/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php#L384-L393 This was added in 7dc8ef1 for [DDC-671](doctrine#5181). The reason for this is that a mapped superclass is not an entity itself and has no table. So, in the database, associations can only be from the inheriting entities' tables towards the referred-to target. This is also the reason for the limitation that only to-one associations may be added in mapped superclasses, since for those the database foreign key can be placed on the table(s) of the inheriting entities (and there may be more than one child class). Neither the decision to update the `sourceEntity` nor the validation check should be based on `$parent->isMappedSuperclass`. This only works in the simple case where the class hierarchy is `Mapped Superclass → Entity`. The check is wrong when we have an inheritance hierarchy set up and the class hierarchy is `Base Entity → Mapped Superclass → Child Entity`. Bug 1: The association should keep the root entity as the source. After all, in a JTI, the root table will contain the foreign key, and we need to base joins on that table when traversing `FROM LeafClass l JOIN l.target`. Bug 2: Do not reject the to-many association declared in the base class. It is ok to have the reverse (owning) side point back to the base entity, as it would be if there were no mapped superclasses at all. The mapped superclass does not declare, add or otherwise interfere with the to-many association at all. Base the decision to change the `sourceEntity` on `$mapping['inherited']` being set. This field points to the topmost _parent entity_ class in the ancestry tree where the relationship mapping appears for the first time. When it is not set, the current class is the first _entity_ class in the hierarchy with that association. Since we are inheriting the relation, it must have been added in a mapped superclass above, but was not yet present in the nearest parent entity class. In that case, it may only be a to-one association and the source entity needs to be updated. (See doctrine#10396 for a clarification of the semantics of `inherited`.) Here is a simplified example of the class hierarchy. See the two tests added for more details – one is for checking the correct usage of a to-one association against/with the base class in JTI. The other is to test that a to-many association on the base class is not rejected. I am sure that there are other tests that (still) cover the update of `sourceEntity` is happening. ```php /** * @entity */ class AssociationTarget { /** * @column(type="integer") * @id * @GeneratedValue */ public $id; } /** * @entity * @InheritanceType("JOINED") * @DiscriminatorColumn(name="discriminator", type="string") * @DiscriminatorMap({"1" = "BaseClass", "2" = "LeafClass"}) */ class BaseClass { /** * @column(type="integer") * @id * @GeneratedValue */ public $id; /** * @manytoone(targetEntity="AssociationTarget") */ public $target; } /** * @MappedSuperclass */ class MediumSuperclass extends BaseClass { } /** * @entity */ class LeafClass extends MediumSuperclass { } ``` When querying `FROM LeafClass l`, it should be possible to `JOIN l.target`. This currently leads to an SQL error because the SQL join will be made via `LeafClass.target_id` instead of `BaseClass.target_id`. `LeafClass` is considered the `sourceEntity` for the association – which is wrong–, and so the foreign key field is expected to be in the `LeafClass` table (using JTI here). Fixes doctrine#5998, fixes doctrine#7825. I have removed the abstract entity class, since it is not relevant for the issue and took the discussion off course. Also, the discriminator map now contains all classes. Added the second variant of the bug, namely that a to-many association would wrongly be rejected in the same situation.
…le of an inheritance hierarchy This fixes two closely related bugs. 1. When inheriting a to-one association from a mapped superclass, update the `sourceEntity` class name to the current class only when the association is actually _declared_ in the mapped superclass. 2. Reject association types that are not allowed on mapped superclasses only when they are actually _declared_ in a mapped superclass, not when inherited from parent classes. Currently, when a many-to-one association is inherited from a `MappedSuperclass`, mapping information will be updated so that the association has the current (inheriting) class as the source entity. https://github.com/doctrine/orm/blob/2138cc93834cfae9cd3f86c991fa051a3129b693/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php#L384-L393 This was added in 7dc8ef1 for [DDC-671](doctrine#5181). The reason for this is that a mapped superclass is not an entity itself and has no table. So, in the database, associations can only be from the inheriting entities' tables towards the referred-to target. This is also the reason for the limitation that only to-one associations may be added in mapped superclasses, since for those the database foreign key can be placed on the table(s) of the inheriting entities (and there may be more than one child class). Neither the decision to update the `sourceEntity` nor the validation check should be based on `$parent->isMappedSuperclass`. This only works in the simple case where the class hierarchy is `Mapped Superclass → Entity`. The check is wrong when we have an inheritance hierarchy set up and the class hierarchy is `Base Entity → Mapped Superclass → Child Entity`. Bug 1: The association should keep the root entity as the source. After all, in a JTI, the root table will contain the foreign key, and we need to base joins on that table when traversing `FROM LeafClass l JOIN l.target`. Bug 2: Do not reject the to-many association declared in the base class. It is ok to have the reverse (owning) side point back to the base entity, as it would be if there were no mapped superclasses at all. The mapped superclass does not declare, add or otherwise interfere with the to-many association at all. Base the decision to change the `sourceEntity` on `$mapping['inherited']` being set. This field points to the topmost _parent entity_ class in the ancestry tree where the relationship mapping appears for the first time. When it is not set, the current class is the first _entity_ class in the hierarchy with that association. Since we are inheriting the relation, it must have been added in a mapped superclass above, but was not yet present in the nearest parent entity class. In that case, it may only be a to-one association and the source entity needs to be updated. (See doctrine#10396 for a clarification of the semantics of `inherited`.) Here is a simplified example of the class hierarchy. See the two tests added for more details – one is for checking the correct usage of a to-one association against/with the base class in JTI. The other is to test that a to-many association on the base class is not rejected. I am sure that there are other tests that (still) cover the update of `sourceEntity` is happening. ```php /** * @entity */ class AssociationTarget { /** * @column(type="integer") * @id * @GeneratedValue */ public $id; } /** * @entity * @InheritanceType("JOINED") * @DiscriminatorColumn(name="discriminator", type="string") * @DiscriminatorMap({"1" = "BaseClass", "2" = "LeafClass"}) */ class BaseClass { /** * @column(type="integer") * @id * @GeneratedValue */ public $id; /** * @manytoone(targetEntity="AssociationTarget") */ public $target; } /** * @MappedSuperclass */ class MediumSuperclass extends BaseClass { } /** * @entity */ class LeafClass extends MediumSuperclass { } ``` When querying `FROM LeafClass l`, it should be possible to `JOIN l.target`. This currently leads to an SQL error because the SQL join will be made via `LeafClass.target_id` instead of `BaseClass.target_id`. `LeafClass` is considered the `sourceEntity` for the association – which is wrong–, and so the foreign key field is expected to be in the `LeafClass` table (using JTI here). Fixes doctrine#5998, fixes doctrine#7825. I have removed the abstract entity class, since it is not relevant for the issue and took the discussion off course. Also, the discriminator map now contains all classes. Added the second variant of the bug, namely that a to-many association would wrongly be rejected in the same situation.
Jira issue originally created by user shurakai:
Affects only YAML. (Maybe also XML? But works with Annotations!)
If there's a relation specified in a mappedSuperclass (manyToOne, towards entity B) and two Entities A and B using this mappedSuperclass, an error occurs within generating the SQL-Query when using a query a la SELECT * FROM A JOIN B ON A.relation = B.id:
This is due to no table name being specified for the mappedSuperclass. (But this tableName is accessed in SqlWalker.php ll. 718. Be aware that there's the mappedSuperclass in use, NOT the class B!)
I'm not quite sure how this should be handled. At least, not specifying the relation in the mappedSuperclass but moving it to each of the yml-files of A and B solves this problem. (But unneccessarily complicates things?)
I will push a test case to my repo.
The text was updated successfully, but these errors were encountered: