Skip to content

feat(doctrine): stateOptions can handleLinks for query optimization - #5732

Merged
soyuka merged 1 commit into
api-platform:mainfrom
soyuka:feat/handle-links-hook
Sep 8, 2023
Merged

feat(doctrine): stateOptions can handleLinks for query optimization#5732
soyuka merged 1 commit into
api-platform:mainfrom
soyuka:feat/handle-links-hook

Conversation

@soyuka

@soyukasoyuka commented Aug 8, 2023

Copy link
Copy Markdown
Member

First, this patch allows stateOptions: entityClass to work with Doctrine ODM (I'll move this to another patch though).

Then, it provides a way to hook into our providers to change how links are provided to Doctrine. This really help with improving performances, it is quite complicated let me try to explain.

The Problem

Say you have /company/les-tilleuls/employees/soyuka. API Platform tries to handle your links, and the algorithm tries to cover all the cases so it'd probably do something like this:

SELECT*FROM Employee e
INNER JOIN Company c ONe.company_id=c.idWHEREc.id='les-tilleuls'ande.id='soyuka'

First it's not that simple as we work with doctrine, relations between multiple tables and different nature (toMany, toOne) are sometimes really tricky and API Platform does things like this:

SELECT*FROM Employee e
WHEREe.idIN (
SELECTc.employee_idFROM Company c
WHEREc.id='les-tilleuls'
)

Depending on the nature of the relation it can be over-complicated and probably also bad in term of query performances.

A solution to this is to say that, depending on business rules, we decide to use:

SELECT * FROM Employee e
WHERE e.company = 'les-tilleuls' and e.id = 'soyuka'

DX

Today you'd have to write a custom provider. Thing is, people love our filters and our pagination handling. Despite trying my best to work on that extensibility, for now, the best is to "copy paste API Platform code" (and keep our copyright thanks <3).

URI Variables came with this huge refactoring and re-visiting data retrieval on subresources. This lead to a quite natural extension point where all our logic resides:

$this->handleLinks($aggregationBuilder, $uriVariables, $context, $resourceClass, $operation);

$this->handleLinks($queryBuilder, $uriVariables, $queryNameGenerator, $context, $entityClass, $operation);

While discussing with @divine, we thought it'd be a good idea to be able to change that part in the User-land.

Current implementation

Maybe this needs re-visiting, maybe that we need a new interface, but it'd need an ORM-specific signature... We already happen to have ApiPlatfirm\State\Option for this?

useApiPlatform\Doctrine\Orm\State;
useDoctrine\ORM\QueryBuilder;
useApiPlatform\Doctrine\Orm\Util\QueryNameGenerator;
useApiPlatform\Metadata\Operation;
#[ApiResource(uriTemplate: '/company/{company}/employees/{employee}' stateOptions: newOptions(handleLinks: [Employee::class, 'handleLinks']))]
#[ORM\Entity]
finalclass Employee {
publicstring$id;
publicstring$employee;
staticfunctionhandleLinks(QueryBuilder$queryBuilder, array$identifiers, QueryNameGenerator$queryNameGenerator, array$context, string$entityClass, Operation$operation) {
$alias = $queryBuilder->getRootAliases()[0];
$queryBuilder->andWhere("$alias.id = :id");
$queryBuilder->setParameter('id', $identifiers['employee']);
}
}

You really have all you need in that signature, but we could also move some of them in the $context (entityClass and operation).

Let me know your thoughts.

Comment threadsrc/Doctrine/Odm/State/Options.php Outdated
Comment threadsrc/Doctrine/Orm/State/Options.php Outdated
Comment threadtests/Doctrine/Odm/State/CollectionProviderTest.php
Comment threadfeatures/doctrine/separated_resource.feature
@soyuka
soyukaforce-pushed the feat/handle-links-hook branch from 57b5273 to e13f29dCompareAugust 9, 2023 08:02
@mrossard

Copy link
Copy Markdown
Contributor

I love the idea!

Comment threadsrc/Doctrine/Orm/State/CollectionProvider.php Outdated
@divine

Copy link
Copy Markdown
Contributor

Hello,

I truly appreciate your work, @soyuka. Thank you very much.

Everything looks nearly perfect, and I've removed 2 files that I've copied from the APP (CollectionProvider and ItemProvider).

I've tested it locally now, but I'm facing a problem while trying to inject the Security or Repository classes since the handleLinks calls are static...

Is there a way to inject it or make it that work?

Thanks!

@soyuka

Copy link
Copy Markdown
MemberAuthor

I need to add a way to call services for that to work!

@usu

usu commented Aug 14, 2023

Copy link
Copy Markdown
Contributor

Tested with my problem described in #5673 and worked well for me. Thanks a lot!

Minor thing: In your example above in the PR description, I think you'd still need to include the uriVariables property, right? So the example should read:

#[ApiResource(
uriTemplate: '/company/{company}/employees/{employee}',
uriVariables: ['company', 'employee'],
stateOptions: newOptions(handleLinks: [Employee::class, 'handleLinks'])
)]

Not really relevant for this PR itself, but probably relevant for the documentation of this new feature.

@usuusu mentioned this pull request Aug 14, 2023
9 tasks
@dunglas

Copy link
Copy Markdown
Member

Your proposal looks great!

Regarding services, we should do as for similar options, controllers etc and call the service with that name of it exists.

@divine

Copy link
Copy Markdown
Contributor

Hello,

Sorry to bump. Can this feature be included in the 3.2 release cycle? Otherwise, I'm worried this will be forgotten.

Thank you so much @soyuka!

@soyuka

Copy link
Copy Markdown
MemberAuthor

I'm curently adding a service locator to improve this a bit

Comment threadsrc/Doctrine/Common/PropertyHelperTrait.php Outdated
Comment threadsrc/Doctrine/Common/State/LinksHandlerTrait.php Outdated
{
$resourceClass = $operation->getClass();
$documentClass = $operation->getClass();
if (($options = $operation->getStateOptions()) && $options instanceof Options && $options->getDocumentClass()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Redundant with DoctrineMongoDbOdmResourceCollectionMetadataFactory, ODM/ItemProvider, ODM/LinksHandlerTrait, GraphQl/FieldsBuilder, Hydra/CollectionFiltersNormalizer, etc.

Can't it be in a more generic trait (e.g. Doctrine/Common/OptionsTrait) or something similar? (same with ORM)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is a quite hard problem I'm not a huge fan of the current dependency between CollectionFiltersNormalizer and state options as when we'll need to subtree split we potentially won't have access to them. When working on this we probably need to uplift this information into some $context['filter_class'] as this option allows filters do be described on an entity instead of on a resource... This adds complexity whereas to where to put the Trait, and also would need specific implementations (ODM vs ORM). I'd like to keep this as-is for now.

Magic is hard to implement correctly :|.

Comment threadsrc/Doctrine/Odm/State/LinksHandlerTrait.php Outdated
Comment threadsrc/Doctrine/Odm/State/Options.php
Comment threadsrc/Doctrine/Orm/State/LinksHandlerInterface.php Outdated
Comment threadsrc/Doctrine/Orm/State/Options.php
@soyuka
soyukaforce-pushed the feat/handle-links-hook branch 3 times, most recently from a7dd210 to 927ab07CompareSeptember 8, 2023 08:59
Comment threadsrc/Doctrine/Orm/State/LinksHandlerTrait.php Outdated
@soyuka
soyukaforce-pushed the feat/handle-links-hook branch from 927ab07 to c33fa22CompareSeptember 8, 2023 13:03
@soyuka
soyukaforce-pushed the feat/handle-links-hook branch from c33fa22 to 111a066CompareSeptember 8, 2023 13:30
@soyuka
soyuka merged commit c7dcd36 into api-platform:mainSep 8, 2023
@soyuka
soyuka deleted the feat/handle-links-hook branch September 8, 2023 13:38
Sign up for freeto 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.

6 participants

@soyuka@mrossard@divine@usu@dunglas@vincentchalamon