[3.2] Using reserved names as fields causes a crash when searching

[pkly]

Describe the bug Somewhat related to #3073 An entity containing a reserved keyword name for doctrine will crash the search when configured in configureCrud -> setSearchFields

To Reproduce Create 2 entities with an associations Foo, containing a field called $order (valid for field name, but needs JoinColumn(name="order_id") which is an association to another entity called Order (let's say it just contains the id), needs a special table name like @ORM\Table(name="sale_order") to function properly (in my case it's just sale__order, so it's not a SQL keyword.)

Create a CrudController for Foo and add

public function configureCrud(
    Crud $crud
}: Crud {
    return $crud
        ->setSearchFields(['order.id']);
}

Then try to search anything, you'll get

[Syntax Error] line 0, col 844: Error: Expected StateFieldPathExpression | string | InputParameter | FunctionsReturningStrings | AggregateExpression, got 'order'

This is not really because it's a forbidden thing, but because the QueryBuilder passed to EasyCorp\Bundle\EasyAdminBundle\Factory\PaginatorFactory::create already contains a ready DQL that contains the left join for order as an alias. This issue can be resolved if the following check is made when creating aliases by the QueryBuilder used in this function

        $name = 'Doctrine\ORM\Query\Lexer::T_' . strtoupper($string);
        if (defined($name)) {
            $type = constant($name);

            if ($type > 100) {
                return self::SAFE_PREFIX.$string; // fix keyword aliases
            }
        }

        return $string;

where self::SAFE_PREFIX is just a prefix added to left joins when using entities with funky field names and can be anything (ea_reserved_prefix_ or similar suggested), alternatively the prefix could be added to all joins, eliminating the need for the lookup in the first place. This is an issue which can be resolved directly in EA, as I've resolved it in another bundle using this trick, but it doesn't apply to table names, only to join aliases which can be anything. I'm not quite sure how the result fetching works yet, so I can't say if it'll be needed to resolve the aliases back to something sensible if needed or if it's done automatically via Doctrine.

(OPTIONAL) Additional context Symfony 4.4.20, EA 3.2.8

[bastoune]

I have the exact same bug with "group". trying to find a workaround before it's fixed.

[pkly]

You'd likely have to override a few classes, it's better if it's simply fixed upstream...

[javiereguiluz]

Thanks for reporting this. I want to fix it. Is there any method in Doctrine to escape the needed things? If there's not ... I don't fully understand how to solve this manually: which exact prefix should we add to order, group, etc.? Thanks!

[pkly]

@javiereguiluz there's no public method to check per-se but you can implement the solution I posted in the report.

https://github.com/doctrine/orm/blob/bcb4889a2c8eb57080f10494554dad5a0ffb1434/lib/Doctrine/ORM/Query/Lexer.php#L182

This place exactly checks for keywords, and the method is protected. As such it is not really possible, unless you'd extend lexer and add an additional method which would then return a boolean if a token in a list that's not allowed was found, but the simpler solution is to simply add the code I wrote. I've used it with great success in a fix for a datatable bundle we're using in production and it does not seem to cause any real issues. An example implementation could be added

// assuming $alias is the current alias as a string

$name = 'Doctrine\ORM\Query\Lexer::T_' . strtoupper($alias );
if (defined($name)) {
    $type = constant($name);

    if ($type > 100) {
        $alias = self:::EA_SAFE_JOIN_PREFIX.$alias ; // fix keyword aliases
    }
}

$qb->leftJoin($entity, $alias);

I'm unsure where the following code would be added as I've only started to encounter these issues when converting our bigger codebase to EA from a custom admin panel, so you'll have to excuse me there.

[javiereguiluz]

Thanks for the details. It sill looks a bit confusing to me 😅 so I looked for in other projects (e.g. https://github.com/TYPO3/TYPO3.CMS/blob/master/typo3/sysext/core/Classes/Database/Query/QueryBuilder.php). In #4334 I'm proposing a solution, but I don't know if that will fix this issue.

[pkly]

Well I guess escaping it is another choice, I kinda forgot about it since all I see is ORM and I kinda got used to it's limitations. If you'd like I can let you know if your PR fixes the issue on monday morning, if tests pass and everything seems to be working there's probably no harm in merging it if you're planning a new release sometime soon, regardless I'll try to get back to you with this.

[pkly]

@javiereguiluz turns out there was some harm and it's good you didn't. Seems like the search function is completely borked, regardless of if we use something that's even supposed to be reserved.

[Syntax Error] line 0, col 197: Error: Expected StateFieldPathExpression | string | InputParameter | FunctionsReturningStrings | AggregateExpression, got '`'

with a stack trace starting from

in vendor/easycorp/easyadmin-bundle/src/Orm/EntityPaginator.php -> getIterator (line 67)

Seems like doctrine doesn't like escaping? I mean, I didn't even know if it was supported in there or not, assuming that you can just change the join alias. On that note why not simply generate custom aliases?

[javiereguiluz]

Thanks for checking this! Sadly I don't have enough Doctrine knowledge to fix this, so we'll need help from the community to create a Pull Request to fix this. Thanks!

[pkly]

This is fixable if you simply assume that each join must have a prefix. So

$qb->leftJoin('entity.order', 'order')

would change into

$qb->leftJoin('entity.order', 'ea_order')

then the subsequent search on that field instead of being order.text LIKE :whatever would be ea_order.text LIKE :whatever This however could be a breaking change for people that modify these queries, though the fix is rather simple and shouldn't take too much time. If you'll tell me where to look (for joins mostly and where they are used later) I could probably look into creating a PR for this

[gaelb80]

I think that's the best approach too

[javiereguiluz]

Fixed (hopefully) by #4344.

[pkly]

@javiereguiluz Unfortunately this does not actually change the builder for the index view, which is what I was having trouble with. It seems that all that was fixed was something related to filters? Sorry for not checking earlier, I have been kind of busy.

As a hint to where to look for it, the following file is the entry point before doctrine dies: https://github.com/EasyCorp/EasyAdminBundle/blob/132c18847a22a558a2be35f505069303f47a89ea/src/Controller/AbstractCrudController.php#L128 The $queryBuilder here contains an invalid (reserved keyword) left join when a field you display is one of many of doctrine's reserved words.

This is somewhat unrelated, but are filters that link to other entities ever supposed to work? (something like TextFilter::new('orderItem.order.id'))

[gaelb80]

@pkly You're right, the addSearchClause method is not fixed and could easily be fixed, but after looking for it, auto join will be broken if a joined property configured for text search is used in filter as it will be join twice with the same alias.

@javiereguiluz I think we need a global rework for relation management in query builder and filters.

Before working on a PR I think we should discuss on the way to do it.

On my side you could find the main ideas on how to do it :

• Manage join building and consistency in EntityRepository service like you do it for addSearchClause method but with some improvements
• Change filter form field name logic : instead of using property name directly, use a formatter to skip reserved character in sfForm (relation1.relation2.field should be converted to relation1relation2field)
• detach filter type guesser from entityDto as we need to retrieve full metadata to dive into relation

With this 2 little improvements you should greatly improve the filters management

You should create a dedicated Issue is you're interested on working on it.

[pkly]

@javiereguiluz could this be re-opened?

[michaelKaefer]

An easy workaround for apps is to use a property name which is not a reserved keyword. In the original example of this issue you would replace the following:

Replace this:

class Foo {
    /**
     * @ORM\ManyToOne(targetEntity=Order::class, inversedBy="foos")
     * @ORM\JoinColumn(name="order_id")
     */
    private $order;
}

DQL Result with unallowed alias (using order) and unallowed WHERE clause (using order.id):

SELECT entity FROM App\Entity\Foo entity LEFT JOIN entity.order order WHERE order.id = :query_for_numbers

And use this instead:

class Foo {
    /**
     * @ORM\ManyToOne(targetEntity=Order::class, inversedBy="foos")
     */
    private $order_;
}

class FooCrudController extends AbstractCrudController
{
    public function configureCrud(Crud $crud): Crud
    {
        return $crud->setSearchFields(['order_.id']);
    }

    public function configureFields(string $pageName): iterable
    {
        return [
            AssociationField::new('order_'),
        ];
    }
}

SQL Result:

SELECT DISTINCT f0_.id AS id_0 
FROM foo f0_ 
LEFT JOIN sale_order s1_ ON f0_.order__id = s1_.id 
WHERE s1_.id = ? LIMIT 20

Note: We don't need the JoinColumn as far as I can see. Just rename your property and it works. The reason is that EA uses the property name when creating the alias.

---

I will also submit a PR to fix it in EA.