feat: add withComment method to add comments

[simivar]

Q	A
Type	feature
Fixed issues	#4168

Summary

This PR provides new withComment method that adds a comment at the top of query. It works for queries and statements.

Support for different database engines is shown in the table below. As you can see, both /* */ (slash-star) and -- (double-hyphen) comment styles are supported across all engines. We can use either, but since double-hyphen comments terminate at the end of a line, I personally find the slash-star style more flexible and a better fit.

Engine	/* */ support	-- support
SQL Server	✅ ^1	✅ ^8
MySQL	✅ ^2	✅ ^9
SQLite	✅ ^3	✅ ^10
PostgreSQL	✅ ^4	✅ ^11
MariaDB	✅ ^5	✅ ^12
Oracle	✅ ^6	✅ ^13
IBM DB2	✅ ^7	✅ ^14

[derrabus]

What would happen if I canfigured my query builder like this?

$qb->withComment('*/ DROP TABLE users; /*');

[morozov]

What would happen if I canfigured my query builder like this?

Cannot this technique be applied to any other component of the query?

I don't really understand why this needs to be part of the DBAL. It doesn't provide any functionality per se.

I understand that some engines may extract query metadata from those commends and provide metrics on top of it, but this functionality could be built as a middleware tailored to the specific use case. The middleware could collect the labels that would need to be applied to the query end encode them into the statement SQL. This way, you can annotate all queries, without having to modify each of them individually.

[derrabus]

What would happen if I canfigured my query builder like this?

Cannot this technique be applied to any other component of the query?

Yeah, maybe. But I think, a user might assume that adding a comment to a query is safe. I probably would. I believe we should at least talk about this topic.

[morozov]

I probably would. I believe we should at least talk about this topic.

We can, but so far it's a well-known (to the maintainers) design issue (see https://github.com/doctrine/dbal/pull/794#discussion_r23899537 for example).

But I think, a user might assume that adding a comment to a query is safe.

I don't think we should be adding this API. It solves a very specific problem in a very specific way, while this problem could be solved with the existing API in a much more flexible way.

[derrabus]

@simivar Can you elaborate why we need this feature?

[simivar]

@morozov @derrabus while this Pull Request references an issue about collecting metrics, which is indeed a great fit for middleware, our use case is slightly different. In one of our legacy monoliths we rely on query comments to instruct the SQL proxy server where to execute a query. As we’re migrating to Doctrine DBAL, we need to preserve this behavior so that developers can continue to add these comments directly in the code when building queries.

In theory, this could be implemented via middleware by overloading bindValue or bindParam and then injecting the comment into the SQL string at execution time. That said, it's seems like misuse of a middleware. Or maybe it's intended use or you see another solution to this problem?

As per the comment being not safe we can always use the -- style which would make it safe in every case.

[morozov]

In theory, this could be implemented via middleware by overloading bindValue or bindParam and then injecting the comment into the SQL string at execution time. That said, it's seems like misuse of a middleware.

I made my original recommendation to use a middleware under the assumption that the comments will reflect some context (e.g. controller, action, source IP, etc.). If a comment needs to be added independently to each query then it's less suitable. However, if the comment will be derived from parameters (that's my understanding), then I'd argue that a middleware is the right approach.

Alternatively, you can just extend the query builder (class AcmeQueryBuilder extends QueryBuilder and use it instead. I have a successful experience of extending it for building unions and subqueries w/o any downsides.

As per the comment being not safe we can always use the -- style which would make it safe in every case.

It would still require some parsing logic because the comment can contain a new line.