UNION queries ignore parameters set on QueryBuilder objects, causing SQL errors

[punjabik]

Bug Report

Q	A
Version	4.1.1

Summary

Parameters in UNION queries using QueryBuilder objects are ignored, causing SQL syntax errors due to unprocessed parameter placeholders.

Current behaviour

When using QueryBuilder objects with the union() and addUnion() methods, parameters set using setParameter() are ignored in the final query execution. This results in SQL syntax errors as parameter placeholders (e.g., ":age") are sent to the database unprocessed.

How to reproduce

1. Create a main QueryBuilder object
2. Create additional QueryBuilder objects for UNION parts
3. Use setParameter() to set parameters on the UNION part QueryBuilder objects
4. Use union() and addUnion() methods to combine the queries, passing the QueryBuilder objects
5. Execute the query

Code snippet to reproduce:

$connection = DriverManager::getConnection(['url' => 'mysql://user:pass@localhost/mydb']);

$qb1 = $connection->createQueryBuilder();
$qb1->select('id', 'name')
   ->from('users')
   ->where('age > :age')
   ->setParameter('age', 30);

$qb2 = $connection->createQueryBuilder();
$qb2->select('id', 'name')
   ->from('admins')
   ->where('level > :level')
   ->setParameter('level', 5);

$unionQb = $connection->createQueryBuilder();
$unionQb->union($qb1)
        ->addUnion($qb2);

$result = $unionQb->executeQuery(); // Throws a SQL syntax error

This code results in a SQL syntax error because the ":age" and ":level" placeholders are sent to the database unprocessed.

The issue appears to be in the DefaultUnionSQLBuilder class:

public function buildSQL(UnionQuery $query): string
{
    $parts = [];
    foreach ($query->getUnionParts() as $union) {
        // ...
        $parts[] = $this->platform->getUnionSelectPartSQL((string) $union->query);
    }
    // ...
}

The $union->query is cast to a string, which only retrieves the SQL string without parameters. There's no mechanism to collect or apply parameters from the UNION parts.

Expected behaviour

Parameters set on all QueryBuilder objects (main and UNION parts) using setParameter() should be collected and applied to the final query execution, resulting in a valid SQL query with all parameters properly substituted.

[derrabus]

This is expected behavior. You have to set the parameters on the outermost query builder.

cc @sbuerk

[punjabik]

This is expected behavior. You have to set the parameters on the outermost query builder.

cc @sbuerk

Thank you for clarifying that this is the expected behavior. I appreciate the quick response. However, I'd like to discuss some concerns about this design decision and its implications for code quality and maintainability.

While I understand that setting parameters on the outermost query builder might simplify the internal implementation, it introduces several practical issues:

1. Encapsulation: This approach breaks the encapsulation of individual query parts. Each sub-query is no longer self-contained, making it harder to manage complex queries.

2. Reusability: If a sub-query needs to be used both as a standalone query and as part of a UNION, we'd need to handle its parameters differently in each context, reducing code reusability.

3. Separation of Concerns: The outer scope is forced to know about and manage the internal parameters of sub-queries, violating the principle of separation of concerns.

4. Error Proneness: In complex queries with multiple UNIONs, setting all parameters on the outermost QueryBuilder could lead to mistakes, especially if parameter names clash across sub-queries.

5. Consistency: The behavior of setParameter() differs for UNION queries compared to regular queries, which can be unintuitive and potentially lead to subtle bugs.

Consider this example:

$qb1 = $connection->createQueryBuilder()
    ->select('id', 'name')
    ->from('users')
    ->where('age > :age');

$qb2 = $connection->createQueryBuilder()
    ->select('id', 'name')
    ->from('admins')
    ->where('level > :level');

// We can't reuse $qb1 and $qb2 as-is in a UNION query
$unionQb = $connection->createQueryBuilder()
    ->union($qb1)
    ->addUnion($qb2)
    ->setParameter('age', 30)
    ->setParameter('level', 5);

In this scenario, $qb1 and $qb2 lose their self-contained nature when used in a UNION, requiring knowledge of their internal parameters to be managed externally.

Given these concerns, I wonder if it would be possible to consider an alternative design that allows for better encapsulation and separation of concerns, while still meeting the performance and implementation needs of the library.

Thank you for your time and for considering these points.

[derrabus]

Thank you for your thorough answer. We've considered these points. However, they haven't been design goals for us.

The goal of the query builder is to help composing a single query programmatically, nothing more, nothing less. There are no self-contained subqueries. There never were and we did not intend to introduce that concept for unions.

Moreover, merging parameters of multiple query builders is not a trivial task. Think about ordered parameters or naming collisions of named parameters. This feature would introduce a lot if unnecessary complexity with really not much gain.

[punjabik]

Thank you for your detailed explanation of the QueryBuilder's design philosophy. I understand the focus on simplicity and the challenges involved in parameter handling for UNION queries.

After considering your points, I'd like to highlight a specific aspect of the current implementation that could potentially lead to confusion:

1. The union() and addUnion() methods accept QueryBuilder objects as arguments.
2. These QueryBuilder objects have a setParameter() method, suggesting that parameters can be set on them.
3. However, when used in a UNION context, any setParameter() calls on these QueryBuilder objects are silently ignored.

This creates a situation where the method signature suggests functionality (by accepting QueryBuilder objects with their full interface) that isn't actually supported in this context. It's counterintuitive that you can pass a QueryBuilder to union(), but the parameters set on that QueryBuilder are not used.

To address this inconsistency, I'd like to propose two alternative solutions:

1. Accept only SQL strings in union() and addUnion(), not QueryBuilder objects. This would make it clear that subqueries in UNIONs are treated as raw SQL. For example:

public function union(string $part): self
{
    // ... implementation
}

public function addUnion(string $part, UnionType $type = UnionType::DISTINCT): self
{
    // ... implementation
}

This change would make it explicit that only SQL strings are accepted, eliminating any confusion about parameter handling on QueryBuilder objects.

2. If maintaining the current method signatures is preferred, enhance the documentation to clearly explain the behavior. Add a notice to the DocBlocks of both union() and addUnion() methods:

/**
 * Specifies union parts to be used to build a UNION query.
 * Replaces any previously specified parts.
 *
 * <code>
 *     $qb = $conn->createQueryBuilder()
 *         ->union('SELECT 1 AS field1', 'SELECT 2 AS field1');
 * </code>
 *
 * @param string|QueryBuilder $part The union part to add
 *
 * @return $this
 *
 * @notice When a QueryBuilder is provided as $part, only its SQL string is used.
 *         Any parameters set on this QueryBuilder are ignored. Use setParameter()
 *         on the main QueryBuilder for parameters in UNION queries.
 */
public function union(string|QueryBuilder $part): self
{
    // ... (existing implementation)
}

Either of these solutions would:

1. Address the inconsistency between accepting QueryBuilder objects and ignoring their parameters.
2. Make the current behavior explicit to users of the library.
3. Help prevent confusion about parameter handling in UNION queries.
4. Guide developers towards the correct way of setting parameters for UNION queries.

What are your thoughts on these proposals?

[derrabus]

We don't use @notice in our code. Apart from that, your suggestion 2 sounds acceptable. Please send a PR. And please also update the documentation accordingly. I realize that this part is missing there as well.

Looking at the doc block, I also realize that the code examples still show variadic calls of the union() and addUnion() methods, but that has been removed during the development of the feature. Can you also fix that?

[sbuerk]

I'm not a fan of changing the signature to string:

• a) that means you have to use getSQL() or (string) cast when passing a QueryBuilder
• b) it's known that when using nested QueryBuilder that the most outer instance needs to retrieve the plaeholder and parameters - just that the new api allows passing the object without having to manually turn it into a string
• c) it's not more or less a confusion like using QueryBuilder instances to provide subselects, for example in where() or select fields etc.

Adding something to the method docblocks should be done, as I expect that it should be expectable that devlopers reads method docblocks when using a method. Don't get it why I missed that when providing the union builder API. So thanks for the report on that.

Will see if I can find some time to add these things if noone is quicker, but the next days are quite full packed due to upcoming release (major) of another project.