api-platform / core

The server component of API Platform: hypermedia and GraphQL APIs in minutes
https://api-platform.com
MIT License
2.45k stars 882 forks source link

Setting `KeepLegacyInflector` to false, will remove all filters from Get/GetCollection operations. #6023

Open frankdekker opened 1 year ago

frankdekker commented 1 year ago

API Platform version(s) affected: v3.2.3

Description Below a (slimmed down) example of our entity and ApiPlatform resource and filter definition:

<?php
declare(strict_types=1);

namespace DR\Review\Entity\Review;

use ApiPlatform\Doctrine\Orm\Filter\SearchFilter;
use ApiPlatform\Metadata\ApiFilter;
use ApiPlatform\Metadata\ApiResource;
use ApiPlatform\Metadata\GetCollection;
use Doctrine\ORM\Mapping as ORM;

#[ApiResource(operations: [new GetCollection()])]
#[ApiFilter(SearchFilter::class,properties: ['id' => 'exact'])]
class CodeReviewActivity
{
    #[ORM\Id]
    #[ORM\GeneratedValue]
    #[ORM\Column]
    private ?int $id = null;
}

When I toggle keep_legacy_inflector setting to false the search filter will disappear from the documentation and api.

How to reproduce
Create the entity above and set keep_legacy_inflector to false. Keep an eye on the namespace. The namespace must with start 2 capital letters.

Possible Solution
I've tracked down the problem to: vendor/api-platform/core/src/Metadata/Util/Inflector.php in the tableize method. See phpunit test below for the difference in output. It seems that the (new UnicodeString($word))->snake()->toString() does not return the exact same output as the original tableize method. Handling multiple sequential uppercase letters differently.

PHPUnit test that reproduces the problem:

<?php
declare(strict_types=1);

use ApiPlatform\Metadata\Util\Inflector;
use PHPUnit\Framework\TestCase;

class InflectorTest extends TestCase
{
    public function testTableize(): void
    {
        $word = "DRReviewEntityReviewCodeReviewActivityApiPlatformDoctrineOrmFilterSearchFilter";

        Inflector::keepLegacyInflector(false);
        $outputA = Inflector::tableize($word);

        Inflector::keepLegacyInflector(true);
        $outputB = Inflector::tableize($word);

        static::assertSame($outputA, $outputB);
        // assert fails:
        // $outputA: dr_review_entity_review_code_review_activity_api_platform_doctrine_orm_filter_search_filter
        // $outputB: d_r_review_entity_review_code_review_activity_api_platform_doctrine_orm_filter_search_filter
    }
}

Additional Context

mrossard commented 12 months ago

Haven't investigated further yet, but this option makes some OrderFilters disappear in my current project too.

mrossard commented 12 months ago

Actually same exact problem here, the ApiResources having this problem have double uppercase letters in their names...

celinederoland commented 11 months ago

Exactly the same for me (filters are ignored when keepLegacyInflector is set to false and the name contains 2 consecutive uppercases or more).

I'm not convinced by the conclusion that Inflector::tableize should do the same thing if keep_legacy_inflector is set to true or false. there is an if/else The solution depends on what the keepLegacyInflector is supposed to do. Maybe it's normal that the legacy inflector doesn't do the same than the new one. The problem is that the new one is not used at every step when we set keepLegacyInflector to false.

There is 3 Inflectors Doctrine\Inflector\Inflector ApiPlatform\Util\Inflector (redirect all method calls to Doctrine\Inflector\Inflector) ApiPlatform\Metadata\Util\Inflector (has it's own implementation, redirect method calls to Doctrine\Inflector\Inflector only if the keep_legacy_inflector option is set to true)

During serviceLocator initialization, we use ApiPlatform\Util\Inflector ; so the legacy one is used, and our filters are stored with the legacy naming convention image

At the moment we create the context to inject it into the Operation, we use ApiPlatform\Metadata\Util\Inflector to retrieve the instance of our filters, and we obtain a name following the new convention. image

And later we always 'continue' because their is no registered filter with the given name image

Other possible solution : add a use statement to replace ApiPlatform\Util\Inflector by ApiPlatform\Metadata\Util\Inflector image

Other possible solution : change the code of ApiPlatform\Util\Inflector to do the same than ApiPlatform\Metadata\Util\Inflector ; but it will still be strange to have 2 inflectors that do the same thing, I wonder what is the reason why those 2 Inflector classes coexist.

@soyuka what do you think ? Is it good ideas ? If you think I'm on the right way I'd be happy to make a PR.

soyuka commented 11 months ago

Hi, ApiPlatform\Util\Inflector is deprecated. We should definitely always go to the new inflector or the one you choose please open a PR.

jotwea commented 7 months ago

Same issue here. Any news?

jonnyeom commented 4 months ago

Im actually seeing a very similar issue.

jonnyeom commented 4 months ago

Im actually seeing a very similar issue.

  • Set keep_legacy_inflector to false
  • The API resources are not multiple capital letters in a row however
  • Custom filters are not getting triggered. Built in ones are.

I was wrong, there was a Multiple Capital letters in a row in the namespace

celinederoland commented 2 months ago

@jonnyeom the fix of december 2023 was actually to handle this multiple capital letters case. But unfortunatly, it seems that there is an other issue with that in production environment only with cache generation.

How to reproduce (tested with apiPlatform 3.2 and 3.3): Set APP_ENV=prod Create a resource with custom filters in the DTO namespace (or any namespace with consecutive capital letters) Remove your cache, and execute php bin/console -v (or any command but debug:router) The cache is re-generated by this command, Then somewhere in var/prod/cache/pools/system there is a file describing the User resource, transforming DTO to d_t_o (INCORRECT ==> see the left part of the illustration bellow) Remove your cache, and execute php bin/console debug:router The cache is re-generated by this command, Then somewhere in var/prod/cache/pools/system there is a file describing the User resource, transforming DTO to d_t_o (CORRECT ==> see the right part of the illustration bellow)

Of course, once the cache is incorrectly generated, in production environment it stays in the same state forever.

Maybe I'll try to create a new MR for this case, but in the meantime I guess avoiding consecutive uppercases is the workaround. Renaming my DTO namespace to DataTransferObject fixes the problem but not the cause of the problem obviously.

image

jonnyeom commented 2 months ago

For now, we could change our namespace.

But this is a really weird issue with non-consistent behavior between environments. I think it should be looked into

jonnyeom commented 2 months ago

@celinederoland I was able to reproduce it in my APP_ENV=test environment by building cache twice. (i.e. bin/console cache:warm --env=test x2)

The first time, the filters are cached into the container and the Inflector class uses the correct ApiPlatform\Metadata\Util\Inflector::$keepLegacyInflector = false from configuration (set to keep_legacy_inflector=false).

But when I run cache:warm a second time, the same static instance of the Inflector class has ApiPlatform\Metadata\Util\Inflector::$keepLegacyInflector = true

I didnt get to the nitty-gritty of what is underscoring it exactly, but..

Current Solution

With Api-Platform 3.4.x #6447 introduces Inflector as a service and correctly sets ::$keepLegacyInflector each time. In fact \ApiPlatform\Metadata\Util\AttributeFilterExtractorTrait::generateFilterId() doesnt even use that service anymore.

tldr

Just upgrading to 3.4.x should fix this specific issue. The environment inconsistencies of the Inflector class should be fixed with the new change anyway.