Conversation
Fixes sqlc-dev#4624. A global override that sets `engine` was applied to every generated package, not just the one being generated. `opts.Parse` prepends the global overrides to the per-package list, and the first match wins in goInnerType, so with postgresql, mysql and sqlite declared in one config all three packages picked up whichever rule was listed first and, for struct tags, the last one. Reproduced with the config from the report on bdbe55d: ``` before after postgresql Value string backend:"sqlite" Value string backend:"postgresql" Code json.RawMessage Code json.RawMessage mysql Value string backend:"sqlite" Value sql.NullString backend:"mysql" Code json.RawMessage Code []byte sqlite Value string backend:"sqlite" Value []byte backend:"sqlite" Code json.RawMessage Code string ``` parseGlobalOpts now drops overrides whose `engine` is set to something other than the engine being generated. Overrides with no `engine` keep applying to every engine, which is what single-engine configs use. The config already requires `engine` on global overrides whenever more than one engine is in use (internal/config), so the selector is always available at the point this filter runs. Adds internal/endtoend/testdata/overrides_global_engine, which generates three packages from one config and pins each type and struct tag. Reverting options.go makes it fail with the sqlite types in all three packages. go test ./internal/codegen/golang/... ./internal/config/... passes. gofmt and go vet are clean.
This file contains hidden or 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
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.
Filter global type overrides by engine before generating
Fixes #4624.
A global override that sets
enginewas applied to every generated package, not just the one being generated.opts.Parseprepends the global overrides to the per-package list, andgoInnerTypereturns on the first match. So withpostgresql,mysqlandsqlitedeclared in one config, all three packages picked up whicheverdb_type: textrule was listed first, and all three took the last matchinggo_struct_tag.Reproduced
Using the config from the report against
bdbe55db:Value string/Code json.RawMessage, tagbackend:"sqlite"Value string/Code json.RawMessage, tagbackend:"postgresql"Value string/Code json.RawMessage, tagbackend:"sqlite"Value sql.NullString/Code []byte, tagbackend:"mysql"Value string/Code json.RawMessage, tagbackend:"sqlite"Value []byte/Code string, tagbackend:"sqlite"sqlc generateexits 0 in both cases, so the wrong types are silent.The fix
parseGlobalOptsdrops overrides whoseengineis set to something other than the engine being generated. Overrides with noenginestill apply everywhere, which is what single-engine configs rely on.req.Settings.Engineis already whatgoInnerTypeswitches on to pick the engine's own type mapping, so the selector is available at this point without any plumbing. The config layer already requiresengineon global overrides whenever more than one engine is in use, so this filter never has to guess.Tests
Adds
internal/endtoend/testdata/overrides_global_engine, which generates three packages from a single config and pins every type and struct tag. Revertingoptions.gomakes it fail with the sqlite types in all three packages:go test ./internal/endtoend -run 'TestReplay/base'passes (251s), as do./internal/codegen/golang/...and./internal/config/....gofmtandgo vetare clean.