Skip to content
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

feat: allow setting a query comment through a context value #1122

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kylemcc
Copy link
Contributor

@kylemcc kylemcc commented Feb 10, 2025

This is an enhancement to recently merged comment functionality. This implementation preserves the current functionality, but allows a comment to be set at the time a query is executed.

This can be used like so:

qctx := bun.ContextWithComment(ctx, "traceid:"+traceID)
err := db.NewSelect().
    Model(&things).
    Where("type = ?", t).
    Scan(qctx)

Which, like the Comment() API will produce:

/* traceid:f2e006f2-8494-4ce6-947e-2c77f4c33dc8 */ SELECT * FROM thing WHERE type = 'whatever'

I should note that in this implementation, if a comment is set through a context, it takes precedence over one set on a query using the Comment() API. If the opposite precedence makes more sense, I'm happy to change this.

I know there was some discussion about supporting multiple comments - I'm happy to add some support for that if it's desirable. I think there's good reason to support both mechanisms and have them play nicely together, but maybe that should be discussed separately. For example, the idea of adding a .Comment("descriptive query name") to individual queries, but then being able to combine that with a context that provides a trace ID so that all queries made as part of a given request can be identified. In this case, I would propose that any comment set on the context appear first in the query followed by any set using the Comment() method.

@j2gg0s j2gg0s self-requested a review February 11, 2025 02:50
@j2gg0s
Copy link
Collaborator

j2gg0s commented Feb 12, 2025

Passing comments through context is a good idea, thank you for your contribution.

  1. If both the Context and Comment methods specify a comment, wouldn’t it make more sense to merge them?
  2. We can implement the logic in the beforeQuery hook method, right? That way, the intrusion into the code will be minimal.

@kylemcc
Copy link
Contributor Author

kylemcc commented Feb 12, 2025

1. If both the Context and Comment methods specify a comment, wouldn’t it make more sense to merge them?

I wouldn't be opposed to merging them - I think there is definitely value there. If I remember correctly, one of the other discussions raised some questions about comment order if multiple comments are specified. So, I think we just need to decide what the proper behavior should be.

2. We can implement the logic in the [beforeQuery](https://github.com/uptrace/bun/blob/master/hook.go#L55) hook method, right? That way, the intrusion into the code will be minimal.

Yes, I think that would be a reasonable way to refactor and DRY up this addition and the existing implementation. Happy to take a pass at that if you'd like.

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.

2 participants