Skip to content

Commit

Permalink
Add an option to handle enums with ugly casing (#270)
Browse files Browse the repository at this point in the history
There are a bunch of places in genqlient where we just kind of hope you
don't have ridiculous casing conflicts in your schema. Apparently with
enum values there are actual schemas that have this problem! Now we have
an option to disable. I ended up putting it all under `casing` instead
of in `bindings` so we can clearly document the list of algorithms we
support, and so we can have an `all_enums` value if you're working with
a schema with a lot of this; in the future we may want to add similar
behavior for types/fields, add more possible algorithms (e.g. to make
things unexported), etc.

I added tests for the new feature, although the way the tests are set up
it wasn't convenient to do so for a schema where this is actually
required. I also added a check for case conflicts that points you to
this option. (I don't want to do it automatically for reasons described
in the issue; mainly it just seemed a bit too magical.)

Fixes #265.
  • Loading branch information
benjaminjkraft committed May 7, 2023
1 parent 9b2f16c commit ff790e1
Show file tree
Hide file tree
Showing 16 changed files with 375 additions and 17 deletions.
1 change: 1 addition & 0 deletions docs/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ When releasing a new version:
### New features:

- 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.

### Bug fixes:

Expand Down
21 changes: 21 additions & 0 deletions docs/genqlient.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -227,3 +227,24 @@ bindings:
# Explicit entries in bindings take precedence over all package bindings.
package_bindings:
- package: github.com/you/yourpkg/models

# Configuration for genqlient's smart-casing.
#
# By default genqlient tries to convert GraphQL type names to Go style
# automatically. Sometimes it doesn't do a great job; this suite of options
# lets you configure its algorithm as makes sense for your schema.
#
# Options below support the following values:
# - default: use genqlient's default algorithm, which tries to convert GraphQL
# names to exported Go names. This is usually best for GraphQL schemas using
# idiomatic GraphQL types.
# - raw: map the GraphQL type exactly; don't try to convert it to Go style.
# This is usually best for schemas with casing conflicts, e.g. enums with
# values which differ only in casing.
casing:
# Use the given casing-style (see above) for all GraphQL enum values.
all_enums: raw
# Use the given casing-style (see above) for the enum values in the given
# GraphQL types (takes precedence over all_enum_values).
enums:
MyEnum: raw
58 changes: 58 additions & 0 deletions generate/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ type Config struct {
ClientGetter string `yaml:"client_getter"`
Bindings map[string]*TypeBinding `yaml:"bindings"`
PackageBindings []*PackageBinding `yaml:"package_bindings"`
Casing Casing `yaml:"casing"`
Optional string `yaml:"optional"`
OptionalGenericType string `yaml:"optional_generic_type"`
StructReferences bool `yaml:"use_struct_references"`
Expand Down Expand Up @@ -68,6 +69,59 @@ type PackageBinding struct {
Package string `yaml:"package"`
}

// CasingAlgorithm represents a way that genqlient can handle casing, and is
// documented further in the [genqlient.yaml docs].
//
// [genqlient.yaml docs]: https://github.com/Khan/genqlient/blob/main/docs/genqlient.yaml
type CasingAlgorithm string

const (
CasingDefault CasingAlgorithm = "default"
CasingRaw CasingAlgorithm = "raw"
)

func (algo CasingAlgorithm) validate() error {
switch algo {
case CasingDefault, CasingRaw:
return nil
default:
return errorf(nil, "unknown casing algorithm: %s", algo)
}
}

// Casing wraps the casing-related options, and is documented further in
// the [genqlient.yaml docs].
//
// [genqlient.yaml docs]: https://github.com/Khan/genqlient/blob/main/docs/genqlient.yaml
type Casing struct {
AllEnums CasingAlgorithm `yaml:"all_enums"`
Enums map[string]CasingAlgorithm `yaml:"enums"`
}

func (casing *Casing) validate() error {
if casing.AllEnums != "" {
if err := casing.AllEnums.validate(); err != nil {
return err
}
}
for _, algo := range casing.Enums {
if err := algo.validate(); err != nil {
return err
}
}
return nil
}

func (casing *Casing) forEnum(graphQLTypeName string) CasingAlgorithm {
if specificConfig, ok := casing.Enums[graphQLTypeName]; ok {
return specificConfig
}
if casing.AllEnums != "" {
return casing.AllEnums
}
return CasingDefault
}

// pathJoin is like filepath.Join but 1) it only takes two argsuments,
// and b) if the second argument is an absolute path the first argument
// is ignored (similar to how python's os.path.join() works).
Expand Down Expand Up @@ -172,6 +226,10 @@ func (c *Config) ValidateAndFillDefaults(baseDir string) error {
}
}

if err := c.Casing.validate(); err != nil {
return err
}

return nil
}

Expand Down
47 changes: 35 additions & 12 deletions generate/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,19 @@ package generate

import (
"os"
"path/filepath"
"testing"

"github.com/Khan/genqlient/internal/testutil"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

const (
findConfigDir = "testdata/find-config"
invalidConfigDir = "testdata/invalid-config"
)

func TestFindCfg(t *testing.T) {
cwd, err := os.Getwd()
require.NoError(t, err)
Expand All @@ -18,15 +25,15 @@ func TestFindCfg(t *testing.T) {
expectedErr error
}{
"yaml in parent directory": {
startDir: cwd + "/testdata/find-config/parent/child",
expectedCfg: cwd + "/testdata/find-config/parent/genqlient.yaml",
startDir: filepath.Join(cwd, findConfigDir, "parent", "child"),
expectedCfg: filepath.Join(cwd, findConfigDir, "parent", "genqlient.yaml"),
},
"yaml in current directory": {
startDir: cwd + "/testdata/find-config/current",
expectedCfg: cwd + "/testdata/find-config/current/genqlient.yaml",
startDir: filepath.Join(cwd, findConfigDir, "current"),
expectedCfg: filepath.Join(cwd, findConfigDir, "current", "genqlient.yaml"),
},
"no yaml": {
startDir: cwd + "/testdata/find-config/none/child",
startDir: filepath.Join(cwd, findConfigDir, "none", "child"),
expectedErr: os.ErrNotExist,
},
}
Expand Down Expand Up @@ -56,23 +63,23 @@ func TestFindCfgInDir(t *testing.T) {
found bool
}{
"yaml": {
startDir: cwd + "/testdata/find-config/filenames/yaml",
startDir: filepath.Join(cwd, findConfigDir, "filenames", "yaml"),
found: true,
},
"yml": {
startDir: cwd + "/testdata/find-config/filenames/yml",
startDir: filepath.Join(cwd, findConfigDir, "filenames", "yml"),
found: true,
},
".yaml": {
startDir: cwd + "/testdata/find-config/filenames/dotyaml",
startDir: filepath.Join(cwd, findConfigDir, "filenames", "dotyaml"),
found: true,
},
".yml": {
startDir: cwd + "/testdata/find-config/filenames/dotyml",
startDir: filepath.Join(cwd, findConfigDir, "filenames", "dotyml"),
found: true,
},
"none": {
startDir: cwd + "/testdata/find-config/filenames/none",
startDir: filepath.Join(cwd, findConfigDir, "filenames", "none"),
found: false,
},
}
Expand All @@ -94,15 +101,31 @@ func TestAbsoluteAndRelativePathsInConfigFiles(t *testing.T) {
require.NoError(t, err)

config, err := ReadAndValidateConfig(
cwd + "/testdata/find-config/current/genqlient.yaml")
filepath.Join(cwd, findConfigDir, "current", "genqlient.yaml"))
require.NoError(t, err)

require.Equal(t, 1, len(config.Schema))
require.Equal(
t,
cwd+"/testdata/find-config/current/schema.graphql",
filepath.Join(cwd, findConfigDir, "current", "schema.graphql"),
config.Schema[0],
)
require.Equal(t, 1, len(config.Operations))
require.Equal(t, "/tmp/genqlient.graphql", config.Operations[0])
}

func TestInvalidConfigs(t *testing.T) {
files, err := os.ReadDir(invalidConfigDir)
if err != nil {
t.Fatal(err)
}

for _, file := range files {
t.Run(file.Name(), func(t *testing.T) {
filename := filepath.Join(invalidConfigDir, file.Name())
_, err := ReadAndValidateConfig(filename)
require.Error(t, err)
testutil.Cupaloy.SnapshotT(t, err.Error())
})
}
}
17 changes: 16 additions & 1 deletion generate/convert.go
Original file line number Diff line number Diff line change
Expand Up @@ -516,8 +516,23 @@ func (g *generator) convertDefinition(
Description: def.Description,
Values: make([]goEnumValue, len(def.EnumValues)),
}
goNames := make(map[string]*goEnumValue, len(def.EnumValues))
for i, val := range def.EnumValues {
goType.Values[i] = goEnumValue{Name: val.Name, Description: val.Description}
goName := g.Config.Casing.enumValueName(name, def, val)
if conflict := goNames[goName]; conflict != nil {
return nil, errorf(val.Position,
"enum values %s and %s have conflicting Go name %s; "+
"add 'all_enums: raw' or 'enums: %v: raw' "+
"to 'casing' in genqlient.yaml to fix",
val.Name, conflict.GraphQLName, goName, def.Name)
}

goType.Values[i] = goEnumValue{
GoName: goName,
GraphQLName: val.Name,
Description: val.Description,
}
goNames[goName] = &goType.Values[i]
}
return g.addType(goType, goType.GoName, pos)

Expand Down
12 changes: 12 additions & 0 deletions generate/generate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,18 @@ func TestGenerateWithConfig(t *testing.T) {
Optional: "generic",
OptionalGenericType: "github.com/Khan/genqlient/internal/testutil.Option",
}},
{"EnumRawCasingAll", "", []string{"QueryWithEnums.graphql"}, &Config{
Generated: "generated.go",
Casing: Casing{
AllEnums: CasingRaw,
},
}},
{"EnumRawCasingSpecific", "", []string{"QueryWithEnums.graphql"}, &Config{
Generated: "generated.go",
Casing: Casing{
Enums: map[string]CasingAlgorithm{"Role": CasingRaw},
},
}},
}

sourceFilename := "SimpleQuery.graphql"
Expand Down
13 changes: 13 additions & 0 deletions generate/names.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ package generate
// response object (inline in convertOperation).

import (
"fmt"
"strings"

"github.com/vektah/gqlparser/v2/ast"
Expand Down Expand Up @@ -178,3 +179,15 @@ func makeLongTypeName(prefix *prefixList, typeName string) string {
typeName = upperFirst(typeName)
return joinPrefixList(&prefixList{typeName, prefix})
}

func (casing *Casing) enumValueName(goTypeName string, enum *ast.Definition, val *ast.EnumValueDefinition) string {
switch algo := casing.forEnum(enum.Name); algo {
case CasingDefault:
return goTypeName + goConstName(val.Name)
case CasingRaw:
return goTypeName + "_" + val.Name
default:
// Should already be caught by validation.
panic(fmt.Sprintf("unknown casing algorithm %s", algo))
}
}
1 change: 1 addition & 0 deletions generate/testdata/errors/ConflictingEnumValues.graphql
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
query ConflictingEnumValues { f }
9 changes: 9 additions & 0 deletions generate/testdata/errors/ConflictingEnumValues.schema.graphql
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
enum AnnoyingEnum {
first_value
second_value
FIRST_VALUE
}

type Query {
f: AnnoyingEnum
}
3 changes: 3 additions & 0 deletions generate/testdata/invalid-config/InvalidCasing.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
casing:
enums:
MyType: bogus
2 changes: 1 addition & 1 deletion generate/testdata/queries/schema.graphql
Original file line number Diff line number Diff line change
Expand Up @@ -204,4 +204,4 @@ input IntComparisonExp {
_lte: Int
_neq: Int
_nin: [Int!]
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
testdata/errors/ConflictingEnumValues.schema.graphql:4: enum values FIRST_VALUE and first_value have conflicting Go name AnnoyingEnumFirstValue; add 'all_enums: raw' or 'enums: AnnoyingEnum: raw' to 'casing' in genqlient.yaml to fix
Loading

0 comments on commit ff790e1

Please sign in to comment.