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.

Sign up for GitHub

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

Jump to bottom

feat: support binding to generated package #285

Closed
wants to merge 1 commit into from
Closed

feat: support binding to generated package #285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ When releasing a new version:

- The new `optional: generic` allows using a generic type to represent optionality. See the [documentation](genqlient.yaml) for details.
- For schemas with enum values that differ only in casing, it's now possible to disable smart-casing in genqlient.yaml; see the [documentation](genqlient.yaml) for `casing` for details.
- It is now possible to bind to the generated package both via `bindings` and `package_bindings` options.

### Bug fixes:
- The presence of negative pointer directives, i.e., `# @genqlient(pointer: false)` are now respected even in the when `optional: pointer` is set in the configuration file.
Expand Down
4 changes: 4 additions & 0 deletions docs/genqlient.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,10 @@ bindings:
# to the bindings map, above, for each exported type in the package. Multiple
# packages may be specified, and later ones take precedence over earlier ones.
# Explicit entries in bindings take precedence over all package bindings.
#
# Using "." is a shorthand for the package containing the generated file.
# Both allow the generated code to live in the same package as its types.
Comment on lines +227 to +229
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I would have some concerns about actually using the generated package here. I worry it would end up being circular in ways that are hard to understand -- in that now genqlient depends on genqlient-generated types. I don't think we need to ban it, but maybe we should say in the comment here that it's not a good idea? Or does it actually work fine?

Also, whatever we settle on for the syntax, can you add it to the docs for bindings above too?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now genqlient depends on genqlient-generated types

Yes, a warning might be appropriate, but it should work as long as the user avoids name conflicts.
I've added a "double generate" test to try to cover this case and made it more explicit in the comment.

can you add it to the docs for bindings above too

The shorthand is intentionally only supported in the "whole package" mode. Otherwise one would end up with ..ID or some other weird syntax to reference the types inside the package. Just didn't feel like it was worth it. WDYT?

# Note that generating code inside the bound package may cause naming conflicts.
package_bindings:
- package: github.com/you/yourpkg/models

Expand Down
21 changes: 21 additions & 0 deletions generate/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ type Config struct {
// The directory of the config-file (relative to which all the other paths
// are resolved). Set by ValidateAndFillDefaults.
baseDir string

// Fully qualified package being generated.
packagePath string
}

// A TypeBinding represents a Go type to which genqlient will bind a particular
Expand Down Expand Up @@ -153,6 +156,23 @@ func (c *Config) ValidateAndFillDefaults(baseDir string) error {
c.ExportOperations = pathJoin(baseDir, c.ExportOperations)
}

// Calculate the fully qualified package path
genPkgPath, err := filepath.Rel(c.baseDir, c.Generated)
if err != nil {
return fmt.Errorf("finding relative path to generated package: %w", err)
}
pkgs, err := packages.Load(&packages.Config{
Dir: c.baseDir,
Mode: packages.NeedName | packages.NeedModule,
}, "./"+filepath.Dir(genPkgPath))
if err != nil {
return fmt.Errorf("loading generated package: %w", err)
}
Comment on lines +168 to +170
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this is safe. Like would this break things if the directory where you're putting the output is empty (as it would be if your output is in a separate package and this is the first run of genqlient)? On the other hand swallowing the error seems dangerous, and it's not clear how easy it is to know if we actually need packagePath. Maybe instead we should just look at baseDir and then add the suffix ourselves? (Is that safe?)

(For that matter, does this require that baseDir is a valid Go package? Or at least one with at least one Go file with valid package clause? Maybe that's fine; if you have some kind of monorepo where all the Go is in a subdirectory you can just put genqlient.yaml in that directory.)

If we don't know the answer, it's probably not a huge deal; it doesn't really affect the API surface we are committing to so if we break someone's workflow we can fix it then.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would this break things if the directory where you're putting the output is empty

Yes, it would break, but that would be the same as trying to bind to an empty package, right?

does this require that baseDir is a valid Go package?

It shouldn't: it's just used as a base for the package search.

if len(pkgs) > 1 {
return fmt.Errorf("ambiguous generated package")
}
c.packagePath = pkgs[0].ID

if c.ContextType == "" {
c.ContextType = "context.Context"
}
Expand Down Expand Up @@ -203,6 +223,7 @@ func (c *Config) ValidateAndFillDefaults(baseDir string) error {

mode := packages.NeedDeps | packages.NeedTypes
pkgs, err := packages.Load(&packages.Config{
Dir: c.baseDir,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking out loud: this should be ok for third-party packages because it's just the directory where we run the build tool, so if binding.Package is absolute then it'll still return the right package (and, if relevant, will now respect any versions/replaces in the module, if they differ from those in the current directory, which is probably actually better than current behavior, in the rare case where it matters).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, exactly. This is just needed to support the "relative import" hack we use above, which isn't usual, but is supported.

Mode: mode,
}, binding.Package)
if err != nil {
Expand Down
96 changes: 69 additions & 27 deletions generate/generate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,24 +21,41 @@ const (
// buildGoFile returns an error if the given Go code is not valid.
//
// namePrefix is used for the temp-file, and is just for debugging.
func buildGoFile(namePrefix string, content []byte) error {
func buildGoFile(namePrefix string, content []byte, extraFiles ...string) error {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

// We need to put this within the current module, rather than in
// /tmp, so that it can access internal/testutil.
f, err := os.CreateTemp("./testdata/tmp", namePrefix+"_*.go")
d, err := os.MkdirTemp("./testdata/tmp", namePrefix+"_*")
if err != nil {
return err
}

f, err := os.Create(filepath.Join(d, "generated.go"))
if err != nil {
return err
}

defer func() {
f.Close()
os.Remove(f.Name())
os.RemoveAll(d)
}()

for _, extraFile := range extraFiles {
data, err := os.ReadFile(extraFile)
if err != nil {
return fmt.Errorf("reading file: %w", err)
}

if err := os.WriteFile(filepath.Join(d, filepath.Base(extraFile)), data, 0o644); err != nil {
return fmt.Errorf("writing file: %w", err)
}
}

_, err = f.Write(content)
if err != nil {
return err
}

cmd := exec.Command("go", "build", f.Name())
cmd := exec.Command("go", "build", d)
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
err = cmd.Run()
Expand Down Expand Up @@ -151,27 +168,28 @@ func TestGenerateWithConfig(t *testing.T) {
baseDir string // relative to dataDir
operations []string // overrides the default set below
config *Config // omits Schema and Operations, set below.
extraFiles []string // extra files to pass to buildGoFile
}{
{"DefaultConfig", "", nil, getDefaultConfig(t)},
{"DefaultConfig", "", nil, getDefaultConfig(t), nil},
{"Subpackage", "", nil, &Config{
Generated: "mypkg/myfile.go",
}},
}, nil},
{"SubpackageConfig", "mypkg", nil, &Config{
Generated: "myfile.go", // (relative to genqlient.yaml)
}},
}, nil},
{"PackageName", "", nil, &Config{
Generated: "myfile.go",
Package: "mypkg",
}},
}, nil},
{"ExportOperations", "", nil, &Config{
ExportOperations: "operations.json",
}},
}, nil},
{"CustomContext", "", nil, &Config{
ContextType: "github.com/Khan/genqlient/internal/testutil.MyContext",
}},
}, nil},
{"CustomContextWithAlias", "", nil, &Config{
ContextType: "github.com/Khan/genqlient/internal/testutil/junk---fun.name.MyContext",
}},
}, nil},
{"StructReferences", "", []string{"InputObject.graphql", "QueryWithStructs.graphql"}, &Config{
StructReferences: true,
Bindings: map[string]*TypeBinding{
Expand All @@ -181,7 +199,7 @@ func TestGenerateWithConfig(t *testing.T) {
Unmarshaler: "github.com/Khan/genqlient/internal/testutil.UnmarshalDate",
},
},
}},
}, nil},
{"StructReferencesAndOptionalPointer", "", []string{"InputObject.graphql", "QueryWithStructs.graphql"}, &Config{
StructReferences: true,
Optional: "pointer",
Expand All @@ -192,54 +210,80 @@ func TestGenerateWithConfig(t *testing.T) {
Unmarshaler: "github.com/Khan/genqlient/internal/testutil.UnmarshalDate",
},
},
}},
}, nil},
{"PackageBindings", "", nil, &Config{
PackageBindings: []*PackageBinding{
{Package: "github.com/Khan/genqlient/internal/testutil"},
},
}},
}, nil},
{"NoContext", "", nil, &Config{
ContextType: "-",
}},
}, nil},
{"ClientGetter", "", nil, &Config{
ClientGetter: "github.com/Khan/genqlient/internal/testutil.GetClientFromContext",
}},
}, nil},
{"ClientGetterCustomContext", "", nil, &Config{
ClientGetter: "github.com/Khan/genqlient/internal/testutil.GetClientFromMyContext",
ContextType: "github.com/Khan/genqlient/internal/testutil.MyContext",
}},
}, nil},
{"ClientGetterNoContext", "", nil, &Config{
ClientGetter: "github.com/Khan/genqlient/internal/testutil.GetClientFromNowhere",
ContextType: "-",
}},
}, nil},
{"Extensions", "", nil, &Config{
Extensions: true,
}},
}, nil},
{"OptionalValue", "", []string{"ListInput.graphql", "QueryWithSlices.graphql"}, &Config{
Optional: "value",
}},
}, nil},
{"OptionalPointer", "", []string{
"ListInput.graphql",
"QueryWithSlices.graphql",
"SimpleQueryWithPointerFalseOverride.graphql",
"SimpleQueryNoOverride.graphql",
}, &Config{
Optional: "pointer",
}},
}, nil},
{"OptionalGeneric", "", []string{"ListInput.graphql", "QueryWithSlices.graphql"}, &Config{
Optional: "generic",
OptionalGenericType: "github.com/Khan/genqlient/internal/testutil.Option",
}},
}, nil},
{"EnumRawCasingAll", "", []string{"QueryWithEnums.graphql"}, &Config{
Casing: Casing{
AllEnums: CasingRaw,
},
}},
}, nil},
{"EnumRawCasingSpecific", "", []string{"QueryWithEnums.graphql"}, &Config{
Casing: Casing{
Enums: map[string]CasingAlgorithm{"Role": CasingRaw},
},
}},
}, nil},
{"PackageBindingsLocal", "mypkg", nil, &Config{
Generated: "myfile.go",
PackageBindings: []*PackageBinding{
{Package: "github.com/Khan/genqlient/generate/testdata/queries/mypkg"},
},
}, []string{"testdata/queries/mypkg/types.go"}},
{"TypeBindingsLocal", "mypkg", nil, &Config{
Generated: "myfile.go",
Bindings: map[string]*TypeBinding{
"ID": {
Type: "github.com/Khan/genqlient/generate/testdata/queries/mypkg.ID",
},
},
}, []string{"testdata/queries/mypkg/types.go"}},
{"PackageBindingsLocalShorthand", "mypkg", nil, &Config{
Generated: "myfile.go",
PackageBindings: []*PackageBinding{
{Package: "."},
},
}, []string{"testdata/queries/mypkg/types.go"}},
{"PackageBindingsLocalWithAlreadyGeneratedFiles", "mypkg", nil, &Config{
Generated: "generated.go",
PackageBindings: []*PackageBinding{
{Package: "."},
},
}, []string{"testdata/queries/mypkg/types.go", "testdata/queries/mypkg/generated.go"}},
}

sourceFilename := "SimpleQuery.graphql"
Expand Down Expand Up @@ -277,8 +321,7 @@ func TestGenerateWithConfig(t *testing.T) {
t.Skip("skipping build due to -short")
}

err := buildGoFile(sourceFilename,
generated[config.Generated])
err := buildGoFile(sourceFilename, generated[config.Generated], test.extraFiles...)
if err != nil {
t.Error(err)
}
Expand Down Expand Up @@ -330,7 +373,6 @@ func TestGenerateErrors(t *testing.T) {
Schema: []string{filepath.Join(errorsDir, schemaFilename)},
Operations: []string{filepath.Join(errorsDir, sourceFilename)},
Package: "test",
Generated: os.DevNull,
ContextType: "context.Context",
Bindings: map[string]*TypeBinding{
"ValidScalar": {Type: "string"},
Expand Down
23 changes: 16 additions & 7 deletions generate/imports.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,15 +99,24 @@ func (g *generator) ref(fullyQualifiedName string) (qualifiedName string, err er

pkgPath := nameToImport[:i]
localName := nameToImport[i+1:]
alias, ok := g.imports[pkgPath]
if !ok {
if g.importsLocked {
return "", errorf(nil,
`genqlient internal error: imports locked but package "%v" has not been imported`, pkgPath)

var out strings.Builder
out.WriteString(prefix)

if pkgPath != g.Config.packagePath {
alias, ok := g.imports[pkgPath]
if !ok {
if g.importsLocked {
return "", errorf(nil,
`genqlient internal error: imports locked but package "%v" has not been imported`, pkgPath)
}
alias = g.addImportFor(pkgPath)
}
alias = g.addImportFor(pkgPath)
out.WriteString(alias + ".")
}
return prefix + alias + "." + localName, nil
out.WriteString(localName)

return out.String(), nil
}

// Returns the import-clause to use in the generated code.
Expand Down
67 changes: 67 additions & 0 deletions generate/testdata/queries/mypkg/generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions generate/testdata/queries/mypkg/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
package mypkg

type ID string
Loading