Make sqlcommenter more configurable #15169
Conversation
github-actions
bot
added
the
test native
| }); | ||
| } | ||
|
|
||
| private static void addSqlCommenterCustomizers(ClassLoader extensionClassLoader) { |
There was a problem hiding this comment.
Based on addHttpServerResponseCustomizers that is right before this method. It supports multiple customizers but really probably works best when there is only once customizers, since since the customizers would most likely end up overwriting what the previous customizer has set.
| * @param propagator a function that receives the database connection and whether the query is | ||
| * executed only once or could be reused. Connection may be a jdbc Connection, R2DBC | ||
| * Connection, or any other connection type used by the data access framework performing the | ||
| * operation. If the second argument to the function is true, the query is executed only once |
There was a problem hiding this comment.
is the second argument called safe above? If so, it sounds like it should be called once to match this description
There was a problem hiding this comment.
is the second argument called safe above? If so, it sounds like it should be called once to match this description
yes it is. Open to suggestions for a better name. What I wanted to express is that when you use Connection.prepareStatement then adding dynamic components to the query text isn't always safe as it wouldn't work correctly when the prepared statement is reused (tracing context points to where the query is prepared not where it is executed). In contrast when you use Statement.execute then you won't have that issue. Based on that boolean instrumentations could choose to use different strategies.
There was a problem hiding this comment.
safe is a good name - but I wasn't sure if it aligns with the javadoc
if true .. only once
it sounds like safe would allow you to do things more than once
There was a problem hiding this comment.
Renamed safe to executed. I had the value flipped for most of the callers, should be corrected now.
| * @param connection connection object, e.g. JDBC connection or R2DBC connection, that is used to | ||
| * execute the query | ||
| * @param sql original query | ||
| * @param executed whether the query is immediately executed after being processed, e.g. {@link |
There was a problem hiding this comment.
maybe it should be an enum since a boolean is so hard to get right
There was a problem hiding this comment.
perhaps we should leave it for later when it is clear whether having this information is useful at all
There was a problem hiding this comment.
your call - I just find the boolean a bit hard to follow
2 participants
Add an option to specify a custom propagator, allow prepending the comment instead of appending, add an experimental api that allows extensions to chose the behavior based on the connection.