Skip to content

Failing test: Criteria matching on fields with custom column types #11897

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

Open
wants to merge 1 commit into
base: 2.20.x
Choose a base branch
from

Conversation

mpdude
Copy link
Contributor

@mpdude mpdude commented Mar 30, 2025

This adds failing tests that show that Criteria matching with at least in and eq expressions does not work for the case of SQL-based lookups (uninitialized PersistentCollections) when the targeted field uses custom types.

In that case, we need to convert the value used within the expression to what it looks like in the database.

The challenging part is that I do not know how to combine the handling of custom types with the fact that the in and notIn expressions need to deal with query parameters that are arrays.

https://www.doctrine-project.org/projects/doctrine-dbal/en/4.2/reference/data-retrieval-and-manipulation.html#list-of-parameters-conversion seems to be part of the puzzle, but I guess we somehow have to apply the type conversion (derive the value that would be found at the DBMS level) before constructing such a list?

Maybe @morozov or @derrabus could advise, you guys are much more knowledgeable with the DBAL type system.

@mpdude mpdude changed the title Add failing tests: Criteria matching on fields with custom column types Failing test: Criteria matching on fields with custom column types Mar 30, 2025
@mpdude mpdude force-pushed the criteria-matching-custom-type branch from 7b74b56 to 8cfa174 Compare March 30, 2025 20:59
@mpdude
Copy link
Contributor Author

mpdude commented Mar 31, 2025

Not sure wheter #7102 is about the same issue

public function provideMatchingExpressions(): Generator
{
yield [Criteria::expr()->eq('field', 'match this')];
yield [Criteria::expr()->in('field', ['match this'])];
Copy link
Member

@morozov morozov Mar 31, 2025

Choose a reason for hiding this comment

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

Not sure if it's related to custom types at all, but this test fails because the following code doesn't properly handle the IN operator:

if ($value === null && ($operator === Comparison::EQ || $operator === Comparison::NEQ)) {
$whereClauses[] = sprintf('te.%s %s NULL', $field, $operator === Comparison::EQ ? 'IS' : 'IS NOT');
} else {
$whereClauses[] = sprintf('te.%s %s ?', $field, $operator);
$params[] = $value;
$paramTypes[] = PersisterHelper::getTypeOfField($name, $targetClass, $this->em)[0];
}

It should:

  1. Add parentheses to the SQL.
  2. Repeat the placeholder, $params[] and $paramTypes[] for each element of the value.

Copy link
Contributor

Choose a reason for hiding this comment

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

#11031 seems to be related to this bug.

public function provideMatchingExpressions(): Generator
{
yield [Criteria::expr()->eq('field', 'match this')];
yield [Criteria::expr()->in('field', ['match this'])];
Copy link
Member

@morozov morozov Mar 31, 2025

Choose a reason for hiding this comment

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

And this one fails because by the time when the following line is executed

$stmt = $this->conn->executeQuery($query, $params, $types);

$types contains [ArrayParameterType::STRING, 'rot13'], so at this point, the information about the custom type is lost.

Right now, the DBAL only supports a fixed set of array types (see ArrayParameterType).

For this to work, we can try introducing the ArrayType class and have it eventually replace the ArrayParameterType enum. This class should accepts the element type in its constructor. For example, new ArrayType(ParameterType::INTEGER) would represent what's currently represented as ArrayParameterType::INTEGER, and new ArrayType('rot13') would represent an array of custom type.

Not sure what effort it would take and how doable this is within the current type system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, something like this was my assumption... that we currently cannot combine the array type hint with custom types. If I get you right, your suggestion is to have the ArrayType as a wrapper (not necessarily decorator) around other types.

Copy link
Member

@morozov morozov Mar 31, 2025

Choose a reason for hiding this comment

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

What's the difference between a wrapper and a decorator? I believe I want to see "typed array" as one of the standard types. It will (I guess) wrap (or be parameterized with) its value type. This way, we can build queries with the array values of any standard or custom type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The distinction between wrapper and decorator was not sharp enough. What I meant was that ArrayType could be wrapped around any other base type, but (to my understanding) need not implement the same interface as the base types themselves.

A textbook decorator – to my understanding – implements at least the same interface as the objects it wraps, it's transparent from the interface point of view.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, in this case I mean the decorator. It should wrap a type and be a type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let’s discuss this over at doctrine/dbal#6883

Sign up for free to 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.

3 participants