Doctrine 2 - ORM
  1. Doctrine 2 - ORM
  2. DDC-2226

addFilterConstraint is called with wrong ClassMetadata, if Entity uses inheritance

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Invalid
    • Affects Version/s: 2.3.2
    • Fix Version/s: None
    • Component/s: ORM
    • Security Level: All
    • Labels:

      Description

      When using the filter system to modify the generated SQL the addFilterConstraint method always receives the metadata of the parent object. Both for Joined and Single Table Inheritance, which is not helpful if you want to do something based on the entity, but for example the parent entity does not fulfill the prequisites.

        Activity

        Hide
        Marco Pivetta added a comment -

        Can you add some example code?

        Show
        Marco Pivetta added a comment - Can you add some example code?
        Hide
        Philipp Dobrigkeit added a comment -
        ParentClass.php
        <?php
        namespace Entities;
        
        /**
         * @Entity @Table(name="parent_class")
         * @InheritanceType("SINGLE_TABLE")
         * @DiscriminatorColumn(name="discr", type="string")
         * @DiscriminatorMap({"parentclass" = "ParentClass", "childclass" = "ChildClass"})
         **/
        class ParentClass {
        
            /**
             * @Id @GeneratedValue @Column(type="integer")
             * @var int
             **/
            protected $id;
        
        }
        
        ChildClass.php
        <?php
        namespace Entities;
        
        /**
         * @Entity
         **/
        class ChildClass extends ParentClass {
        
            /**
             * @Column(type="string")
             * @var string
             **/
            protected $foo;
        
        }
        
        filter.php
        <?php
        use Doctrine\ORM\Mapping\ClassMetaData,
            Doctrine\ORM\Query\Filter\SQLFilter;
        
        class TestFilter extends SQLFilter
        {
            public function addFilterConstraint(ClassMetadata $targetEntity, $targetTableAlias)
            {
                print_r($targetEntity);
                exit;
        
                return "";
            }
        }
        
        index.php
        <?php
        
        require_once "vendor/autoload.php";
        
        // Create a simple "default" Doctrine ORM configuration for XML Mapping
        $isDevMode = true;
        
        $config = Setup::createAnnotationMetadataConfiguration(array(__DIR__."/Entities"), $isDevMode);
        //$config = Setup::createYAMLMetadataConfiguration(array(__DIR__."/config/yaml"), $isDevMode);
        
        // database configuration parameters
        $conn = array(
            'driver' => 'pdo_sqlite',
            'path' => __DIR__ . '/db.sqlite',
        );
        
        // obtaining the entity manager
        $entityManager = \Doctrine\ORM\EntityManager::create($conn, $config);
        
        require_once "Entities/ParentClass.php";
        require_once "Entities/ChildClass.php";
        require_once "filter.php";
        
        $config->addFilter("test", "TestFilter");
        
        $filter = $entityManager->getFilters()->enable("test");
        
        $entityManager->find('Entities\ChildClass', 1);
        

        When doing this the filter is called, but with the ParentClass metadata. Looking into the source-code, both the SingleTablePersister:158 as well as the JoinedSubclassPersister:386 explicity change the metadata for the filter to the RootEntitiy, but there is no indication why this is done?! If there is a reason for doing so, it should also pass the child metadata, so the filter can actually work based on the queried entity, not the root entity only.

        Show
        Philipp Dobrigkeit added a comment - ParentClass.php <?php namespace Entities; /** * @Entity @Table(name= "parent_class" ) * @InheritanceType( "SINGLE_TABLE" ) * @DiscriminatorColumn(name= "discr" , type= "string" ) * @DiscriminatorMap({ "parentclass" = "ParentClass" , "childclass" = "ChildClass" }) **/ class ParentClass { /** * @Id @GeneratedValue @Column(type= "integer" ) * @ var int **/ protected $id; } ChildClass.php <?php namespace Entities; /** * @Entity **/ class ChildClass extends ParentClass { /** * @Column(type= "string" ) * @ var string **/ protected $foo; } filter.php <?php use Doctrine\ORM\Mapping\ClassMetaData, Doctrine\ORM\Query\Filter\SQLFilter; class TestFilter extends SQLFilter { public function addFilterConstraint(ClassMetadata $targetEntity, $targetTableAlias) { print_r($targetEntity); exit; return ""; } } index.php <?php require_once "vendor/autoload.php" ; // Create a simple " default " Doctrine ORM configuration for XML Mapping $isDevMode = true ; $config = Setup::createAnnotationMetadataConfiguration(array(__DIR__. "/Entities" ), $isDevMode); //$config = Setup::createYAMLMetadataConfiguration(array(__DIR__. "/config/yaml" ), $isDevMode); // database configuration parameters $conn = array( 'driver' => 'pdo_sqlite', 'path' => __DIR__ . '/db.sqlite', ); // obtaining the entity manager $entityManager = \Doctrine\ORM\EntityManager::create($conn, $config); require_once "Entities/ParentClass.php" ; require_once "Entities/ChildClass.php" ; require_once "filter.php" ; $config->addFilter( "test" , "TestFilter" ); $filter = $entityManager->getFilters()->enable( "test" ); $entityManager->find('Entities\ChildClass', 1); When doing this the filter is called, but with the ParentClass metadata. Looking into the source-code, both the SingleTablePersister:158 as well as the JoinedSubclassPersister:386 explicity change the metadata for the filter to the RootEntitiy, but there is no indication why this is done?! If there is a reason for doing so, it should also pass the child metadata, so the filter can actually work based on the queried entity, not the root entity only.
        Hide
        Benjamin Eberlei added a comment -

        This is not a bug, its by design, see the documentation of filters.

        We cannot pass the child class, since there are common cases where using fields of the children will break the complete query.

        Show
        Benjamin Eberlei added a comment - This is not a bug, its by design, see the documentation of filters. We cannot pass the child class, since there are common cases where using fields of the children will break the complete query.
        Hide
        Philipp Dobrigkeit added a comment -

        I just read the documentation again.

        Throughout this document the example MyLocaleFilter class will be used to illustrate how the filter feature works. A filter class must extend the base Doctrine\ORM\Query\Filter\SQLFilter class and implement the addFilterConstraint method. The method receives the ClassMetadata of the filtered entity and the table alias of the SQL table of the entity.

        it states that the method receives the ClassMetadata of the filtered entity, which the above shows is not true. My Filter is supposed to add security constraints and it is not possible to have explicit differences between for example the different "Roles": "Admin" and "User", because I would only get the ClassMetadata of "Role".

        Would you be ok if there was an optional third parameter to the addFilterConstraint that passes the actual metadata and then I can check if that parameter is present?

        Show
        Philipp Dobrigkeit added a comment - I just read the documentation again. Throughout this document the example MyLocaleFilter class will be used to illustrate how the filter feature works. A filter class must extend the base Doctrine\ORM\Query\Filter\SQLFilter class and implement the addFilterConstraint method. The method receives the ClassMetadata of the filtered entity and the table alias of the SQL table of the entity. it states that the method receives the ClassMetadata of the filtered entity, which the above shows is not true. My Filter is supposed to add security constraints and it is not possible to have explicit differences between for example the different "Roles": "Admin" and "User", because I would only get the ClassMetadata of "Role". Would you be ok if there was an optional third parameter to the addFilterConstraint that passes the actual metadata and then I can check if that parameter is present?
        Hide
        Philipp Dobrigkeit added a comment -

        See my comment

        Show
        Philipp Dobrigkeit added a comment - See my comment
        Hide
        Benjamin Eberlei added a comment -

        No that is not possible.

        As for the documentation, i fixed it in the following commit:

        https://github.com/doctrine/orm-documentation/commit/47043a54a535941f5cdcdacb4a9dd2bb5de887c1

        I was pretty sure Alexander added that paragraph before, sorry to point you to the docs without rechecking first.

        Show
        Benjamin Eberlei added a comment - No that is not possible. As for the documentation, i fixed it in the following commit: https://github.com/doctrine/orm-documentation/commit/47043a54a535941f5cdcdacb4a9dd2bb5de887c1 I was pretty sure Alexander added that paragraph before, sorry to point you to the docs without rechecking first.
        Hide
        Stefan Kleff added a comment -

        Can you please be more specific or provide an example? Saying "there are some edge cases" does not clarify things. There seems to be a valid reason why the root is used instead of the entity, but it would be very helpful to know this for writing custom filters.

        Show
        Stefan Kleff added a comment - Can you please be more specific or provide an example? Saying "there are some edge cases" does not clarify things. There seems to be a valid reason why the root is used instead of the entity, but it would be very helpful to know this for writing custom filters.
        Hide
        Stefan Kleff added a comment - - edited

        And just to emphasise the importance: Afaik all filters (which I know) rely on the entity and not the root entity, like f.ex. the commonly used DoctrineExtensions: https://github.com/l3pp4rd/DoctrineExtensions/blob/master/lib/Gedmo/SoftDeleteable/Filter/SoftDeleteableFilter.php

        Show
        Stefan Kleff added a comment - - edited And just to emphasise the importance: Afaik all filters (which I know) rely on the entity and not the root entity, like f.ex. the commonly used DoctrineExtensions: https://github.com/l3pp4rd/DoctrineExtensions/blob/master/lib/Gedmo/SoftDeleteable/Filter/SoftDeleteableFilter.php
        Hide
        Alexander added a comment -

        The problem is that with for example Joined Table Inheritance the tables of the child classes are outer/left joined. Adding a filter for those tables will result in filtering away the data of the joined table, not of the whole entity. Based on this we decided to implement only filtering on the root entity of the inheritance tree.

        Does this answer your question?

        Show
        Alexander added a comment - The problem is that with for example Joined Table Inheritance the tables of the child classes are outer/left joined. Adding a filter for those tables will result in filtering away the data of the joined table, not of the whole entity. Based on this we decided to implement only filtering on the root entity of the inheritance tree. Does this answer your question?
        Hide
        Philipp Dobrigkeit added a comment -

        At least to some bit. But aren't the filters completely in user-land code and I would be responsible for making sure what I am doing? I. e. checking if I have a child entity at hand? You guys cannot be responsible for every mistake your users make and limiting the functionality in one of the (already) few extension points makes things harder for the developer.

        I understand that there might be cases when people implement a bad filter and things break down, but that can happen.

        For us this thing is related to the security of our complete application and it makes a huge difference if a user must be allowed to view every entity of a inheritance hierarchy or I can define on a much more granular level. And for using a filter to limit the result returned from the DB is the logical place to do so.

        I also would be happy to hear some kind of suggestion what I could do instead, not only a "will not fix, not my problem" kind of response

        Are there any filters implemented in the main ORM project?

        Show
        Philipp Dobrigkeit added a comment - At least to some bit. But aren't the filters completely in user-land code and I would be responsible for making sure what I am doing? I. e. checking if I have a child entity at hand? You guys cannot be responsible for every mistake your users make and limiting the functionality in one of the (already) few extension points makes things harder for the developer. I understand that there might be cases when people implement a bad filter and things break down, but that can happen. For us this thing is related to the security of our complete application and it makes a huge difference if a user must be allowed to view every entity of a inheritance hierarchy or I can define on a much more granular level. And for using a filter to limit the result returned from the DB is the logical place to do so. I also would be happy to hear some kind of suggestion what I could do instead, not only a "will not fix, not my problem" kind of response Are there any filters implemented in the main ORM project?
        Hide
        Benjamin Eberlei added a comment -

        No, that is not our opinion. The ORM is complicated and we don't make it even more complicated by allowing users to practically do everything they want.

        Filters and inheritance are common enough problems that we think this would occur relatively often and by the way this bug occurs it wouldn't even be apparent to probably most people. Thats a very dangerous setup we avoid by restricing the usage of this feature.

        Inheritance is overused in OOP applications and composition is much better in most cases anyways. With a model that is not using inheritance, you would not have this problem. With inheritance, you can still do filtering as long as the filtered properties are on the root of the inheritance hierachy.

        There are no filters in the main ORM.

        Show
        Benjamin Eberlei added a comment - No, that is not our opinion. The ORM is complicated and we don't make it even more complicated by allowing users to practically do everything they want. Filters and inheritance are common enough problems that we think this would occur relatively often and by the way this bug occurs it wouldn't even be apparent to probably most people. Thats a very dangerous setup we avoid by restricing the usage of this feature. Inheritance is overused in OOP applications and composition is much better in most cases anyways. With a model that is not using inheritance, you would not have this problem. With inheritance, you can still do filtering as long as the filtered properties are on the root of the inheritance hierachy. There are no filters in the main ORM.

          People

          • Assignee:
            Benjamin Eberlei
            Reporter:
            Philipp Dobrigkeit
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: