Safer template usage with more aggressive errors on likely problems #799
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bgentry
approved these changes
Mar 8, 2025
Comment on lines
124
to
126
| case !containerOK && !sqlContainsTemplate: | ||
| // Neither context container or template in SQL; short circuit fast because there's no work to do. | ||
| return sql, args, nil |
There was a problem hiding this comment.
Switch statements are executed top-to-bottom, so maybe you want this fast path short circuit to go first?
Follows up the addition of `sqlctemplate` in #794. I noticed while adding functionality in with templates that it was quite easy to (1) add a `/* TEMPLATE` tag to `river_job.sql`, (2) put in context parameters to a driver, but then (3) forget to run `make generate`. The context parameters are injected, but `sqlctemplate` no ops with a fast short circuit because there's no `/* TEMPLATE` tag present in the generated Go code that the driver is executing. This leads to confusion. Here, add a few more error conditions: * If a context container is present without any `/* TEMPLATE` tags, error. * If any `/* TEMPLATE` tags are present without a context container, error. This makes dumb bugs easier to catch because we get an explicit error instead of them failing silently. Tests are updated to check for them.
brandur
force-pushed
the
brandur-safer-template-usage
branch
from
565d684 to
d261a51
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follows up the addition of
sqlctemplatein #794. I noticed whileadding functionality in with templates that it was quite easy to (1) add
a
/* TEMPLATEtag toriver_job.sql, (2) put in context parameters toa driver, but then (3) forget to run
make generate. The contextparameters are injected, but
sqlctemplateno ops with a fast shortcircuit because there's no
/* TEMPLATEtag present in the generatedGo code that the driver is executing. This leads to confusion.
Here, add a few more error conditions:
If a context container is present without any
/* TEMPLATEtags,error.
If any
/* TEMPLATEtags are present without a context container,error.
This makes dumb bugs easier to catch because we get an explicit error
instead of them failing silently. Tests are updated to check for them.