Skip to content
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

Allow Partial for Non-Object Hydration #10920

Closed
wants to merge 3 commits into from

Conversation

raziel057
Copy link
Contributor

@raziel057 raziel057 commented Aug 24, 2023

As mentioned here we can understand partial object hydration is a bad idea that can lead to many issues. So it seems a fine to not support it anymore.

But it's also mentionned that the partial object problem does not apply to methods or queries where you do not retrieve the query result as objects. Examples are: Query#getArrayResult(), Query#getScalarResult(), Query#getSingleScalarResult(), etc.

While trying to update code in a project using partial and array result hydration, for performance reasons, my code is way less clean using Dto (that are always flat objects) or using the mix object and scalar data.

Ex. using partial to get an entity with metadata of file attached (but without the blob content for performance and to avoid unnecessary memory load).

$qb->select('d, p, partial f.{id, mimetype, name, size}')
    ->from(Document::class, 'd')
    ->join('d.file', 'f')
    ->leftJoin('d.publications', 'p')
    ->getQuery()->getArrayResult();

Result:
262074956-2436bfa3-025a-4009-8ee9-eda3ff3b8e81

Using a Dto, it's not possible to create nested objects like that:

$qb->select('new DocumentDto(d, p, new FileMetadataDto(f.id, f.mimetype, f.name, f.size))')
    ->from(Document::class, 'd')
    ->join('d.file', 'f')
    ->leftJoin('d.publications', 'p')
    ->getQuery()->getArrayResult();

I receive an error [Syntax Error] line 0, col 53: Error: Unexpected 'new' as nested objects are not supported.

Using a flat Dto:

$qb->select('new DocumentDto(d, p, f.id, f.mimetype, f.name, f.size)')
    ->from(Document::class, 'd')
    ->join('d.file', 'f')
    ->leftJoin('d.publications', 'p');

I'm forced to type publications as int rather than Collection or array else I receive an error.

use Doctrine\Common\Collections\Collection;

class DocumentDto
{
    public function __construct(
        public int $id,
        public int $publications,
        public int $fileId,
        public string $fileMimetype,
        public string $fileName,
        public int $fileSize,
    ) {
    }
}

Result:
image

But in that case the object is duplicated because I should have a collection of publication. I would expect to receive a collection or array of publications in the first object rather than duplicated objects.

Using a mix of object and scalar in query:

$qb->select('d, p, f.id as fileId, f.mimetype as fileMimeType, f.name as fileName, f.size as fileSize')
    ->from(Document::class, 'd')
    ->join('d.file', 'f')
    ->leftJoin('d.publications', 'p');

I need to name the scalar fields explicitly to avoid ambiguity when fetching data. I also have a multi level root result which seems not as clean as with partial query.

Result:
image

Here is just one of multiple sample I have in mind. Could you please reconsider the support of partial for non problematic use cases? Thanks

Related commit: #8471

@raziel057
Copy link
Contributor Author

@beberlei Sorry to disturb you but is it possible to reconsider this deprecation as partial can add value without drawbacks when used for array hydration? I'm afraid that a lot of people will have to loose time to adapt their code for a bad reason.

@derrabus
Copy link
Member

Using a Dto, it's not possible to create nested objects like that:

That's something we could fix, imho. I don't see why we should not allow to nest DTO constructors. Would this already solve the issue for you?

@raziel057
Copy link
Contributor Author

It would be nice to allow to nest Dto constructors and fix the other issue I described concerning Collections, but I still see value in support of partial with array hydration. No need to write boilerplate code for no added value. It can be boring to write Dto(s) to replicate entities just for one query that will be used to display data that you fetch immediately.

@derrabus
Copy link
Member

It can be boring to write Dto(s) to replicate entities just for one query that will be used to display data that you fetch immediately.

Right, but in such a case, selecting data into a flat array should be fine as well.

@raziel057
Copy link
Contributor Author

Well, it's possible to build a flat array but its time consuming as you need to add properties manually taking care of possible conflict in property name and adding alias for such properties. Moreover it duplicates the rows as soon as you select a property joined to a toMany entity.

Reusing the same sample, I can query like that:

$qb->select('d.id, d.title, d.public, d.onlyAccredited, d.createdAt, d.updatedAt, p.id as publications, 
    f.id as fileId, f.mimetype as fileMimeType, f.name as fileName, f.size as fileSize')
    ->from(Document::class, 'd')
    ->join('d.file', 'f')
    ->leftJoin('d.publications', 'p');

image

It's way more elegant with the partial solution. Once again I am not saying that it is necessary to allow the hydration of partial objects (which is problematic and can lead to bad situation) but to keep the support for what does not pose any problem.

@raziel057
Copy link
Contributor Author

raziel057 commented Sep 18, 2023

Would it be possible to provide feedback here? I don't know why it's tagged as "New Feature" but I just propose here to keep partial hydration in for non controversial use cases (for scalar and array result hydration), as it avoid boilerplate code while providing a useful formatting of nested data.

https://www.doctrine-project.org/projects/doctrine-orm/en/2.16/reference/partial-objects.html

The partial object problem in general does not apply to methods or queries where you do not retrieve the query result as objects. Examples are: Query#getArrayResult(), Query#getScalarResult(), Query#getSingleScalarResult(), etc.

@derrabus
Copy link
Member

Would it be possible to provide feedback here?

For this PR specifically: I don't think that the parser result should depend on the hydration mode. That would be the consequence of merging your change up to 3.0.x where partial hydration has been removed already. Also, it's kinda odd to keep a feature called "partial hydration" for those cases where we actually don't hydrate partial objects.

DTOs should be the proper replacement for partial hydration. You have described scenarios where they did not work for you, e.g. nested DTOs. Let's look into making those happen. We could also look into introducing a syntax for selecting fields into an array to create a lightweight nested structure.

If it helps you, I can close the PR as won't fix, but I believe this discussion to be important and I think we can evolve this PR into something we want to keep for 3.0, if you're willing to work with us.

@raziel057
Copy link
Contributor Author

Thanks for your reply. From what I can see the Partial hydration have not yet been removed from Parser in 3.0.x, that's why I was hopping it could be discussed before final decision:
https://github.com/doctrine/orm/blob/3.0.x/lib/Doctrine/ORM/Query/Parser.php#L1677C75-L1677C75
https://github.com/doctrine/orm/blob/3.0.x/lib/Doctrine/ORM/Query/AST/PartialObjectExpression.php

Concerning the naming, I understand you point, even if the partial can be seen as the selection from the data source, not the result. We select partial data from entities.

DTOs, even with nested management, can help for sure but for some use cases were you need dynamism it's clearly not an ideal solution. It's not always easy to explain simple uses cases but I could see different projects / applications implemented by different companies, where dropping of partial will be the main problematic path for migration.

We could also look into introducing a syntax for selecting fields into an array to create a lightweight nested structure.

It would be nice to give a try even to avoid too much issues for adoption of Doctrine ORM 3.x.

@beberlei
Copy link
Member

@derrabus there is a PR somewhere (open or closed status unknown) that adds support for nesting DTOs. It is rather complex, so I wasn't sure about it yet, but its definately possible.

@kevinpapst
Copy link

kevinpapst commented Oct 31, 2023

Not sure if this is the right place to discuss / ask. But I was just recently hit by the deprecation about PARTIAL objects (and I am on the latest Symfony 6.3.x release cycle), so maybe many people did not even realize that there is a (years old) discussion about removing support for PARTIAL.

I raised a discussion here: #11034

But this is critical and questions in other issues have been ignored, so I am cross-posting here as well.

My entire application uses trees of Entity objects, fetched via PARTIAL (example links in the discussion) for performance issues. I don't think, that this logic can be replaced with these new DTO objects: that would literally mean rewriting my entire application. I don't see how using DTO can solve this issue, as the current code relies on the original entities.

Considering that many devs runs into performance issues when using multiple JOINS across larger entity relations, I would bet that PARTIAL is widely used. At this point, I would love to see a forward compatible way to replace PARTIAL queries and/or kindly ask to reconsider the entire deprecation (until this topic is really clarified).

EDIT: There are many open & closed issues about this topic. For everyone else wondering where to get more infos, this issue looks like a good option: #10314 and the PR #8391

@beberlei
Copy link
Member

beberlei commented Nov 1, 2023

Closing for now, as partial expression has been removed in 3.0, this is only going to work through other ways.

See #8471 for current state of ideas and replacements.

@beberlei beberlei closed this Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants