sonata-project / SonataDoctrineORMAdminBundle

Integrate Doctrine ORM into the SonataAdminBundle
https://docs.sonata-project.org/projects/SonataDoctrineORMAdminBundle
MIT License
445 stars 345 forks source link

Fetch join queries do not work on `3.31` #1360

Closed franmomu closed 3 years ago

franmomu commented 3 years ago

Environment

Sonata packages

show

``` $ composer show --latest 'sonata-project/*' sonata-project/admin-bundle 3.93.0 3.93.0 The missing Symfony Admin Generator sonata-project/block-bundle 4.5.2 4.5.2 Symfony SonataBlockBundle sonata-project/cache 2.1.1 2.1.1 Cache library sonata-project/doctrine-extensions 1.12.0 1.12.0 Doctrine2 behavioral extensions sonata-project/exporter 2.5.1 2.5.1 Lightweight Exporter library sonata-project/form-extensions 1.9.0 1.9.0 Symfony form extensions sonata-project/twig-extensions 1.5.1 1.5.1 Sonata twig extensions ```

Symfony packages

show

``` $ composer show --latest 'symfony/*' symfony/asset v5.2.4 v5.2.4 Manages URL generation and versioning of web assets such as CSS stylesheets, JavaScript files and image files symfony/browser-kit v5.2.4 v5.2.4 Simulates the behavior of a web browser, allowing you to make requests, click on links and submit forms programmatically symfony/cache v5.2.4 v5.2.4 Provides an extended PSR-6, PSR-16 (and tags) implementation symfony/cache-contracts v2.2.0 v2.2.0 Generic abstractions related to caching symfony/config v5.2.4 v5.2.4 Helps you find, load, combine, autofill and validate configuration values of any kind symfony/console v4.4.20 v5.2.5 Eases the creation of beautiful and testable command line interfaces symfony/debug v4.4.20 v4.4.20 Provides tools to ease debugging PHP code symfony/dependency-injection v5.2.5 v5.2.5 Allows you to standardize and centralize the way objects are constructed in your application symfony/deprecation-contracts v2.2.0 v2.2.0 A generic function and convention to trigger deprecation notices symfony/doctrine-bridge v4.4.20 v5.2.5 Provides integration for Doctrine with various Symfony components symfony/dom-crawler v5.2.4 v5.2.4 Eases DOM navigation for HTML and XML documents symfony/error-handler v4.4.20 v5.2.4 Provides tools to manage errors and ease debugging PHP code symfony/event-dispatcher v4.4.20 v5.2.4 Provides tools that allow your application components to communicate with each other by dispatching events and listening to them symfony/event-dispatcher-contracts v1.1.9 v2.2.0 Generic abstractions related to dispatching event symfony/expression-language v5.2.4 v5.2.4 Provides an engine that can compile and evaluate expressions symfony/filesystem v5.2.4 v5.2.4 Provides basic utilities for the filesystem symfony/finder v5.2.4 v5.2.4 Finds files and directories via an intuitive fluent interface symfony/form v4.4.20 v5.2.5 Allows to easily create, process and reuse HTML forms symfony/framework-bundle v4.4.20 v5.2.5 Provides a tight integration between Symfony components and the Symfony full-stack framework symfony/http-client v5.2.4 v5.2.4 Provides powerful methods to fetch HTTP resources synchronously or asynchronously symfony/http-client-contracts v2.3.1 v2.3.1 Generic abstractions related to HTTP clients symfony/http-foundation v5.2.4 v5.2.4 Defines an object-oriented layer for the HTTP specification symfony/http-kernel v4.4.20 v5.2.5 Provides a structured process for converting a Request into a Response symfony/intl v5.2.4 v5.2.4 Provides a PHP replacement layer for the C intl extension that includes additional data from the ICU library symfony/options-resolver v5.2.4 v5.2.4 Provides an improved replacement for the array_replace PHP function symfony/panther v1.0.1 v1.0.1 A browser testing and web scraping library for PHP and Symfony. symfony/phpunit-bridge v5.2.4 v5.2.4 Provides utilities for PHPUnit, especially user deprecation notices management symfony/polyfill-ctype v1.22.1 v1.22.1 Symfony polyfill for ctype functions symfony/polyfill-intl-grapheme v1.22.1 v1.22.1 Symfony polyfill for intl's grapheme_* functions symfony/polyfill-intl-icu v1.22.1 v1.22.1 Symfony polyfill for intl's ICU-related data and classes symfony/polyfill-intl-normalizer v1.22.1 v1.22.1 Symfony polyfill for intl's Normalizer class and related functions symfony/polyfill-mbstring v1.22.1 v1.22.1 Symfony polyfill for the Mbstring extension symfony/polyfill-php72 v1.22.1 v1.22.1 Symfony polyfill backporting some PHP 7.2+ features to lower PHP versions symfony/polyfill-php73 v1.22.1 v1.22.1 Symfony polyfill backporting some PHP 7.3+ features to lower PHP versions symfony/polyfill-php80 v1.22.1 v1.22.1 Symfony polyfill backporting some PHP 8.0+ features to lower PHP versions symfony/process v5.2.4 v5.2.4 Executes commands in sub-processes symfony/property-access v5.2.4 v5.2.4 Provides functions to read and write from/to an object or array using a simple string notation symfony/property-info v5.2.4 v5.2.4 Extracts information about PHP class' properties using metadata of popular sources symfony/routing v5.2.4 v5.2.4 Maps an HTTP request to a set of configuration variables symfony/security-acl v3.1.1 v3.1.1 Symfony Security Component - ACL (Access Control List) symfony/security-bundle v4.4.20 v5.2.5 Provides a tight integration of the Security component into the Symfony full-stack framework symfony/security-core v4.4.20 v5.2.5 Symfony Security Component - Core Library symfony/security-csrf v5.2.4 v5.2.4 Symfony Security Component - CSRF Library symfony/security-guard v4.4.20 v5.2.4 Symfony Security Component - Guard symfony/security-http v4.4.20 v5.2.5 Symfony Security Component - HTTP Integration symfony/service-contracts v2.2.0 v2.2.0 Generic abstractions related to writing services symfony/string v5.2.4 v5.2.4 Provides an object-oriented API to strings and deals with bytes, UTF-8 code points and grapheme clusters in a unified way symfony/templating v5.2.4 v5.2.4 Provides all the tools needed to build any kind of template system symfony/translation v4.4.20 v5.2.5 Provides tools to internationalize your application symfony/translation-contracts v2.3.0 v2.3.0 Generic abstractions related to translation symfony/twig-bridge v4.4.20 v5.2.5 Provides integration for Twig with various Symfony components symfony/twig-bundle v4.4.20 v5.2.4 Provides a tight integration of Twig into the Symfony full-stack framework symfony/validator v5.2.5 v5.2.5 Provides tools to validate values symfony/var-dumper v5.2.5 v5.2.5 Provides mechanisms for walking through any arbitrary PHP variable symfony/var-exporter v5.2.4 v5.2.4 Allows exporting any serializable PHP data structure to plain PHP code symfony/yaml v5.2.5 v5.2.5 Loads and dumps YAML files ```

PHP version

$ php -v
PHP 7.4.14 (cli) (built: Jan  8 2021 13:20:04) ( NTS )
Copyright (c) The PHP Group
Zend Engine v3.4.0, Copyright (c) Zend Technologies
    with Zend OPcache v7.4.14, Copyright (c), by Zend Technologies
    with Xdebug v2.9.2, Copyright (c) 2002-2020, by Derick Rethans

Subject

Version 3.31 (https://github.com/sonata-project/SonataDoctrineORMAdminBundle/pull/1319) introduced a BC break when using a fetch join query in AdminInterface::configureQuery, when the query is executed, the object is not fully hydrated, it only has one item in a one-to-many association.

Minimal repository with the bug

I created a test here https://github.com/sonata-project/SonataDoctrineORMAdminBundle/pull/1354 that it works on 3.30.

Steps to reproduce

It's already in the test, having an entity:

class Author
{
    /**
     * @ORM\OneToMany(targetEntity=Book::class, mappedBy="author")
     *
     * @var Collection<array-key, Book>
     */
    private $books;
}

in AuthorAdmin:

protected function configureQuery(ProxyQueryInterface $query): ProxyQueryInterface
{
    $alias = $query->getQueryBuilder()->getRootAliases()[0];

    $query
        ->addSelect('book')
        ->leftJoin(sprintf('%s.books', $alias), 'book');

    return $query;
}

Then adding 2 books to an author, then go to list page.

Expected results

The number of books should be 2.

image

Actual results

image

virtualize commented 3 years ago

I too can confirm this bug. Adding a leftJoin to a OTM or MTM relation in an admin's configureQuery does not show all the related entities of this join anymore. Only one related entity is show in admin list.

This is due the groupBy added in https://github.com/sonata-project/SonataDoctrineORMAdminBundle/pull/1319 https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/29aeea8ba292ac3f626c56e78484b2c10b04cbe2/src/Datagrid/ProxyQuery.php#L261

Downgrading to sonata-project/doctrine-orm-admin-bundle 3.30.0 solves it

VincentLanglet commented 3 years ago

I too can confirm this bug. Adding a leftJoin to a OTM or MTM relation in an admin's configureQuery does not show all the related entities of this join anymore. Only one related entity is show in admin list.

I'm not sure you're talking about the same things. The addGroupBy was added on purpose,

If you're using the author admin, with a leftJoin on article, author should be display only once ; this was fixed on purpose. If you want the author to be displayed multiple times (one by article), you should create an article admin.

@franmomu Does removing the groupBy fix the issue ? Should we let the developer adding the groupBy in his own configureQuery then ?

CC @greg0ire Maybe you can help

@franmomu can you also try

        foreach ($identifierFields as $identifierField) {
            $field = $rootAlias.'.'.$identifierField;

            if (!\in_array($field, $existingOrders, true)) {
                $queryBuilder->addOrderBy($field, $this->getSortOrder());
            }
        }

        return $queryBuilder->distinct()->getQuery();

instead of https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/29aeea8ba292ac3f626c56e78484b2c10b04cbe2/src/Datagrid/ProxyQuery.php#L256-L267

I tried on my project and I feel like it works for me

virtualize commented 3 years ago

@VincentLanglet thanks for your fast response. I have exactly the same use case (and problem) as franmomu already described here: https://github.com/sonata-project/SonataDoctrineORMAdminBundle/pull/1354#issuecomment-801912558

VincentLanglet commented 3 years ago

@VincentLanglet thanks for your fast response. I have exactly the same use case (and problem) as franmomu already described here: #1354 (comment)

Can you try my suggestion (Removing the groupBy and adding a distinct) ?

virtualize commented 3 years ago

Tried it. Yes, your suggestion does work!

franmomu commented 3 years ago

I'll see if I can try it better later, but I think it doesn't work with several fetch-joins.

franmomu commented 3 years ago

Apparently, it doesn't work, I've updated the test to add a second fetch-join https://github.com/sonata-project/SonataDoctrineORMAdminBundle/pull/1354

image

It also should show 5 elements, but there are only 3.

VincentLanglet commented 3 years ago

It also should show 5 elements, but there are only 3.

You only have three author, so it shouldn't show 5 elements. 3 is correct IMHO. I don't understand why the count made by the Paginator is saying 5

Can you post the queries ?

Previously to the PR #1319, you might have 5 elements indeed. But it leads to really weird behavior. With 80 results to displayed and 32 results per page, I had one day

The right result here should be IMHO

WonderfulAuthor / 1 / 0
Miguel / 1 / 0
Author with 2 books / 2 / 200

The 30 readers results help to understand the behavior. Because of the addSelect, it selects 32 results:

If you're modifying the select, I'm not surprised you're ending with a weird result. So I'm not sure it should be considered as a bug... This seems more to be an existing hacky way to have less queries, but nothing was documented as a real feature.

Previously, the query was something like

select * from ... where id in (select id from ... where your_conditions)

It had some issues, for example if you want to use a computed field to order the list

$query->addSelect('SUM(foo, bar) AS HIDDEN sum`) 
$query->orderBy('sum')

You had

select * from ... where id in (select id from ... where your_conditions order by sum)

So it threw an error. Now you have

select *, SUM(foo, bar) AS HIDDEN sum from ... where your_conditions order by sum

Which works.

Distinct is still maybe better because it fixes https://github.com/sonata-project/SonataDoctrineORMAdminBundle/issues/1361

VincentLanglet commented 3 years ago

Just another idea ; since we're using the Paginator for the count, we could maybe use the Paginator for the list results too https://www.doctrine-project.org/projects/doctrine-orm/en/2.8/tutorials/pagination.html

VincentLanglet commented 3 years ago

@franmomu @virtualize can you try https://github.com/sonata-project/SonataDoctrineORMAdminBundle/pull/1368 ?

I think it works.

franmomu commented 3 years ago

It also should show 5 elements, but there are only 3.

You only have three author, so it shouldn't show 5 elements. 3 is correct IMHO. I don't understand why the count made by the Paginator is saying 5

Can you post the queries ?

Previously to the PR #1319, you might have 5 elements indeed. But it leads to really weird behavior. With 80 results to displayed and 32 results per page, I had one day

  • 25 results on the page 1
  • 25 results on the page 2
  • 20 results on the page 3
  • 10 results never displayed, because they are on an invisible page 4.

The right result here should be IMHO

WonderfulAuthor / 1 / 0
Miguel / 1 / 0
Author with 2 books / 2 / 200

I have 5 authors... 3 from AuthorFixtures, one from EmbeddedMappingTest and other from ManyToOneMappingTest.

The 30 readers results help to understand the behavior. Because of the addSelect, it selects 32 results:

  • Wonderful author with no reader.
  • Miguel with no reader.
  • The first 30 reader of the author with two books.

If you're modifying the select, I'm not surprised you're ending with a weird result. So I'm not sure it should be considered as a bug... This seems more to be an existing hacky way to have less queries, but nothing was documented as a real feature.

What? I added this link in other comment: https://www.doctrine-project.org/projects/doctrine-orm/en/2.8/reference/dql-doctrine-query-language.html#joins, how so is it not documented?

VincentLanglet commented 3 years ago

I have 5 authors... 3 from AuthorFixtures, one from EmbeddedMappingTest and other from ManyToOneMappingTest.

Yes indeed, I discovered this by running the test. If you display 256 results, you see them.

What? I added this link in other comment: https://www.doctrine-project.org/projects/doctrine-orm/en/2.8/reference/dql-doctrine-query-language.html#joins, how so is it not documented?

I was thinking about the Sonata documentation. I wasn't sure the select part could be modified by the developer.

Anyway, can you make a try with my PR to use the Paginator ?

axzx commented 3 years ago

PR #1319 broke it all down :(

postgresql

SQLSTATE[42803]: Grouping error: 7 ERROR: column "i1_.id" must appear in the GROUP BY clause or be used in an aggregate function
VincentLanglet commented 3 years ago

@axzx Can you try https://github.com/sonata-project/SonataDoctrineORMAdminBundle/pull/1368 ?

virtualize commented 3 years ago

@VincentLanglet #1368 seems to work fine on our staging system. List pagination is correct, total number of results also. The joined entities are there and the number of queries is reduced as expected. We join two one-to-many relations like this:

public function configureQuery(ProxyQueryInterface $query): ProxyQueryInterface
{
    $query = parent::configureQuery($query);
    $alias = current($query->getRootAliases());

    return $query
        ->addSelect('matchables')
        ->leftJoin($alias.'.matchables', 'matchables')
        ->addSelect('purchases')
        ->leftJoin($alias.'.purchases', 'purchases')
    ;
}

So far thanks and thumbs up!

franmomu commented 3 years ago

I was thinking about the Sonata documentation. I wasn't sure the select part could be modified by the developer.

Covering QueryBuilder I think is out of the scope of Sonata Documentation, in your previous example you're modifying the select part:

$query->addSelect('SUM(foo, bar) AS HIDDEN sum`) 
$query->orderBy('sum')

Anyway, can you make a try with my PR to use the Paginator ?

Apparently it works, I'll try a little bit more later.

psyray commented 3 years ago

Hi,

@VincentLanglet @franmomu I have the same case OTM & MTM relations between 2 entities. I have modified the configureQuery method to reduce queries like @franmomu did.

    public function configureQuery(ProxyQueryInterface $query): ProxyQueryInterface
    {
        $query = parent::configureQuery($query);
        $alias = current($query->getRootAliases());

        // Reduce queries
        $query->addSelect('e');
        $query->addSelect('c');
        $query->addSelect('g');
        $query->leftJoin($alias.'.groups', 'g');
        $query->leftJoin($alias.'.evenements', 'e');
        $query->leftJoin($alias.'.codes', 'c');

        return $query;
    }

When updating to 3.31, I have a problem with sonata exporter on this query, error returned is

Iterate with fetch join in class App\Application\Sonata\UserBundle\Entity\User using association groups not allowed.

This problem was introduced by #1319 PR. If I remove the addSelects, export is working fine, but no queries reduction....

I tried many things by reading #1368, #1360, nothing worked. Only a downgrade to 3.30 is able to make this thing working fine.

So my questions is :

Thanks

VincentLanglet commented 3 years ago

Did you try the last version ?

According to this issue: https://github.com/doctrine/orm/issues/5868, adding distinct is supposed to be enough. What is the DQL of the query generated in the Datasource ? Best would be to add a functionnal test to this project.

@franmomu Do you have similar issue with the export ?

psyray commented 3 years ago

Yes same problem with the last version. I have already saw the doctrine issue (the first I saw, and I found by elimination that the problem is #1319). But I don't know how I could add the distinct part to configureQuery, do you know how could I do ? The query generated is this one

SELECT COUNT (*) AS
dctrn_count
FROM
(
   SELECT DISTINCT
      id_33
   FROM
   (
      SELECT
         f0_.username AS username_0,
         f0_.username_canonical AS username_canonical_1,
         f0_.email AS email_2,
         f0_.email_canonical AS email_canonical_3,
         f0_.enabled AS enabled_4,
         f0_.salt AS salt_5,
         f0_.password AS password_6,
         f0_.last_login AS last_login_7,
         f0_.confirmation_token AS confirmation_token_8,
         f0_.password_requested_at AS password_requested_at_9,
         f0_.roles AS roles_10,
         f0_.created_at AS created_at_11,
         f0_.updated_at AS updated_at_12,
         f0_.date_of_birth AS date_of_birth_13,
         f0_.firstname AS firstname_14,
         f0_.lastname AS lastname_15,
         f0_.website AS website_16,
         f0_.biography AS biography_17,
         f0_.gender AS gender_18,
         f0_.locale AS locale_19,
         f0_.timezone AS timezone_20,
         f0_.phone AS phone_21,
         f0_.facebook_uid AS facebook_uid_22,
         f0_.facebook_name AS facebook_name_23,
         f0_.facebook_data AS facebook_data_24,
         f0_.twitter_uid AS twitter_uid_25,
         f0_.twitter_name AS twitter_name_26,
         f0_.twitter_data AS twitter_data_27,
         f0_.gplus_uid AS gplus_uid_28,
         f0_.gplus_name AS gplus_name_29,
         f0_.gplus_data AS gplus_data_30,
         f0_.token AS token_31,
         f0_.two_step_code AS two_step_code_32,
         f0_.id AS id_33,
         f1_.name AS name_34,
         f1_.roles AS roles_35,
         f1_.id AS id_36,
         e2_.id AS id_37,
         e2_.nom AS nom_38,
         e2_.libelle AS libelle_39,
         e2_.nom_court AS nom_court_40,
         e2_.date_debut AS date_debut_41,
         e2_.date_fin AS date_fin_42,
         e2_.email AS email_43,
         e2_.langue AS langue_44,
         e2_.logo_url AS logo_url_45,
         e2_.resume_url AS resume_url_46,
         e2_.event_url AS event_url_47,
         e2_.json AS json_48,
         e2_.deleted_at AS deleted_at_49,
         c3_.id AS id_50,
         c3_.code AS code_51,
         c3_.reference AS reference_52,
         c3_.libelle AS libelle_53,
         c3_.json AS json_54,
         c3_.code_type AS code_type_55,
         c3_.deleted_at AS deleted_at_56
      FROM
         fos_user_user f0_
      LEFT
      JOIN fos_user_user_group f4_ ON f0_.id = f4_.user_id
      LEFT
      JOIN fos_user_group f1_ ON f1_.id = f4_.group_id
      LEFT
      JOIN user_evenement u5_ ON f0_.id = u5_.user
      LEFT
      JOIN evenement e2_ ON e2_.id = u5_.evenement
      AND
      (
         e2_.deleted_at IS NULL
      )
      LEFT
      JOIN user_code u6_ ON f0_.id = u6_.user
      LEFT
      JOIN code c3_ ON c3_.id = u6_.code
      AND
      (
         c3_.deleted_at IS NULL
      )
      ORDER BY
         f0_.lastname ASC,
      f0_.id ASC,
      e2_.nom_court ASC
   )

      dctrn_result
)

   dctrn_table [] []
VincentLanglet commented 3 years ago

But I don't know how I could add the distinct part to configureQuery, do you know how could I do ?

You shouldn't need to add it to the configureQuery. It's only needed to iterate.

But it's supposed to be added in the DataSource

            $query->getQueryBuilder()->distinct();
            $query->setFirstResult(null);
            $query->setMaxResults(null);

            $doctrineQuery = $query->getDoctrineQuery();

The query generated is this one

This is the Query generated in order to display the count on the page. I would need the query used when exporting.

You could have it by adding dd($doctrineQuery->getDQL()) in your vendor. Right before

return new DoctrineORMQuerySourceIterator($doctrineQuery, $fields);

in DataSource::createIterator.

psyray commented 3 years ago

Thanks for the dd tip, here it comes

SELECT DISTINCT
   o,
   e,
   c,
   g
FROM
   App\Application\Sonata\UserBundle\Entity\User o
LEFT
JOIN o.groups g
LEFT
JOIN o.evenements e
LEFT
JOIN o.codes c
ORDER BY
   o.lastname ASC,
o.id ASC
psyray commented 3 years ago

I think I know where the problem is, in the Datasource there is a if which test the object instance. It is set to ProxyQueryInterface, but while i'm debugging, the $query object is a ProxyQuery instance, not ProxyQueryInterface image

EDIT : Forget about it, it's not the problem. Anyway the querybuilder is correctly passing through

            // Distinct is needed to iterate, even if group by is used
            // @see https://github.com/doctrine/orm/issues/5868
            $query->getQueryBuilder()->distinct();
            $query->setFirstResult(null);
            $query->setMaxResults(null);

            $doctrineQuery = $query->getDoctrineQuery();

and it's failing with error Iterate...

VincentLanglet commented 3 years ago

I reproduced the issue.

We might have to do something like

        $query->getQueryBuilder()->distinct()->select(current($query->getQueryBuilder()->getRootAliases()));
        $query->setFirstResult(null);
        $query->setMaxResults(null);

        $doctrineQuery = $query->getDoctrineQuery();
psyray commented 3 years ago

I confirm that it works with your modification :+1:

VincentLanglet commented 3 years ago

I confirm that it works with your modification 👍

Can you open a PR ?

psyray commented 3 years ago

Do you think this modification will not have any side effects ?

VincentLanglet commented 3 years ago

Do you think this modification will not have any side effects ?

It's pretty similar to previous code: https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/3.x/src/Exporter/DataSource.php#L42

The best would be to add a functionnal test in order to avoid another regression.

psyray commented 3 years ago

I can do the PR, but I can't do the test.

franmomu commented 3 years ago

Hmmm I haven't used the export action with these queries, but those changes looks fine.