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	✅ 2
MySQL	✅ 3	✅ 4
SQLite	✅ 5	✅ 6
PostgreSQL	✅ 7	✅ 8
MariaDB	✅ 9	✅ 10
Oracle	✅ 11	✅ 12
IBM DB2	✅ 13	✅ 14

Footnotes

1. https://learn.microsoft.com/en-us/sql/t-sql/language-elements/slash-star-comment-transact-sql?view=sql-server-ver17#syntax ↩

2. https://learn.microsoft.com/en-us/sql/t-sql/language-elements/comment-transact-sql?view=sql-server-ver17 ↩

3. https://dev.mysql.com/doc/refman/5.7/en/comments.html#:~:text=From%20a%20/*%20sequence ↩

4. https://dev.mysql.com/doc/refman/5.7/en/comments.html#:~:text=From%20a%20--%20sequence ↩

5. https://www.sqlite.org/lang_comment.html#:~:text=C-style%20comments%20begin%20with%20"/*" ↩

6. https://www.sqlite.org/lang_comment.html#:~:text=SQL%20comments%20begin%20with%20two%20consecutive%20%22-%22 ↩

7. https://www.postgresql.org/docs/7.0/syntax519.htm#:~:text=We%20also%20support%20C-style%20block%20comments,%20e.g.: ↩

8. https://www.postgresql.org/docs/7.0/syntax519.htm#:~:text=A%20comment%20is%20an%20arbitrary%20sequence%20of%20characters%20beginning%20with%20double%20dashes ↩

9. https://mariadb.com/docs/server/reference/sql-statements/comment-syntax#:~:text=C%20style%20comments%20from ↩

10. https://mariadb.com/docs/server/reference/sql-statements/comment-syntax#:~:text=From%20a%20'--'%20to%20the%20end%20of%20a%20line ↩

11. https://docs.oracle.com/cd/E24693_01/server.11203/e17118/sql_elements006.htm#:~:text=Begin%20the%20comment%20with%20a,a%20slash%20(*/). ↩

12. https://docs.oracle.com/cd/E24693_01/server.11203/e17118/sql_elements006.htm#:~:text=Begin%20the%20comment%20with%20-- ↩

13. https://www.ibm.com/docs/en/db2-for-zos/12.0.0?topic=statements-sql-comments#:~:text=Bracketed%20comments%20are%20introduced ↩

14. https://www.ibm.com/docs/en/db2-for-zos/12.0.0?topic=statements-sql-comments#:~:text=Simple%20comments%20are%20introduced%20with%20two%20consecutive%20hyphens ↩

[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 #794 (comment) 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.

[simivar]

@derrabus @morozov I've added simple string replacement for the comment tokens (/* and */) which should resolve the issue with comment injection. If you do not feel comfotable with this change, do not want to maintain this or there is any other reason you do not feel like this should make it to the library please let me know. It is mentioned from time to time in different issues and would be helpful for the project I am working on, but I fully understand and we can always close this to not keep stale MRs in the repository.

[greg0ire]

Just noticed I had pending review comments that were never sent. Here they are.

[simivar]

Codestyle issues are now fixed, I've missed one place.

$ vendor/bin/phpcs 
WARNING: The  standard uses 1 deprecated sniff
-------------------------------------------------------------------------------------------------------------------------------------------------
-  Squiz.WhiteSpace.LanguageConstructSpacing
   This sniff has been deprecated since v3.3.0 and will be removed in v4.0.0. Use the Generic.WhiteSpace.LanguageConstructSpacing sniff instead.

Deprecated sniffs are still run, but will stop working at some point in the future.

............................................................ 60 / 72 (83%)
............                                                 72 / 72 (100%)


Time: 290ms; Memory: 52.19MB

[morozov]

To make sure this is not lost in the conversation:

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.

[simivar]

I get the concern about adding API surface for something that might look problem-specific. The reason I still think addComment() belongs in DBAL is that this has been requested multiple times before and the feature itself is generic: "prefix SQL with a safe block comment"

Also, this is not hypothetical. ProxySQL explicitly supports "query annotations" inside a leading comment to tune or route individual queries (timeout, hostgroup, cache TTL, etc.)

Middleware helps when the comment is derived from global context (request id/trace), but it does not cover intentionally per-query annotations. Subclassing QueryBuilder works, but it forces each app/integration to add custom plumbing for the same tiny, broadly useful primitive

The latest changes keep it opt-in and minimal, and they cover one more small SQL primitive

That said, if you still feel this should not land in DBAL, I understand and I am fine with that decision

[derrabus]

Injecting a comment into a specific query for debugging/tracing purposes sounds like a valid use-case to me. I would be fine with adding this feature to the query builder.

[derrabus]

Why should we normalize whitespaces here? This would essentially forbid multi-line comments, wouldn't it?

[simivar]

I added this as supporting context: Elastic Filebeat, Logstash, Splunk, etc. typically require extra configuration (multiline rules/codecs) to ingest multiline log entries correctly. If you think this is out of scope or too much detail, I can remove it

Example reference: https://www.elastic.co/docs/reference/logstash/plugins/plugins-codecs-multiline

[derrabus]

Well, the query builder is an abstraction for SQL. Comments are a part of the language, so adding an abstraction for that is fine.

However, I don't see why an SQL abstraction should know about how to preprocess strings for any of those tools you've mentioned.

Please remove the whitespace normalization and add multiline comments to the tests.

[simivar]

@derrabus done 👍 please let me know if it looks good