enum values binding

[nikitacrit]

I want to improve enum model mapping. I miss the ability to flexible map enum values to go const enums of eny types. My suggestion is to add ability to map each grahpql enum value to specific go const via @goModel directive or models config.
Now you are able to use @goModel directive with ENUM_VALUE. First map enum with specific type then map each enum value with required values. Same result also can be achieved by models config. This implementation contains example server.
How to use it:
Complete schema with @goModel binding:

directive @goModel(
    model: String
    models: [String!]
) on OBJECT | INPUT_OBJECT | SCALAR | ENUM | ENUM_VALUE | INTERFACE | UNION

type Query {
    int(typedN: IntTyped, untypedN: IntUntyped, typed: IntTyped!, untyped: IntUntyped!): [IntTyped!]
    string(typedN: StringTyped, untypedN: StringUntyped, typed: StringTyped!, untyped: StringUntyped!): [StringTyped!]
    bool(typedN: BoolTyped, untypedN: BoolUntyped, typed: BoolTyped!, untyped: BoolUntyped!): [BoolTyped!]
}

enum IntTyped @goModel(model: "github.com/99designs/gqlgen/_examples/enum/model.IntTyped") {
    ONE @goModel(model: "github.com/99designs/gqlgen/_examples/enum/model.IntTypedOne")
    TWO @goModel(model: "github.com/99designs/gqlgen/_examples/enum/model.IntTypedTwo")
}

enum IntUntyped @goModel(model: "github.com/99designs/gqlgen/graphql.Int") {
    ONE @goModel(model: "github.com/99designs/gqlgen/_examples/enum/model.IntUntypedOne")
    TWO @goModel(model: "github.com/99designs/gqlgen/_examples/enum/model.IntUntypedTwo")
}

enum StringTyped @goModel(model: "github.com/99designs/gqlgen/_examples/enum/model.StringTyped")  {
    ONE @goModel(model: "github.com/99designs/gqlgen/_examples/enum/model.StringTypedOne")
    TWO @goModel(model: "github.com/99designs/gqlgen/_examples/enum/model.StringTypedTwo")
}

enum StringUntyped @goModel(model: "github.com/99designs/gqlgen/graphql.String") {
    ONE @goModel(model: "github.com/99designs/gqlgen/_examples/enum/model.StringUntypedOne")
    TWO @goModel(model: "github.com/99designs/gqlgen/_examples/enum/model.StringUntypedTwo")
}

enum BoolTyped {
    TRUE
    FALSE
}

enum BoolUntyped {
    TRUE
    FALSE
}

Or just use models config:

models:
  BoolTyped:
    model: github.com/99designs/gqlgen/_examples/enum/model.BoolTyped
    enum_values:
      TRUE: github.com/99designs/gqlgen/_examples/enum/model.BoolTypedTrue
      FALSE: github.com/99designs/gqlgen/_examples/enum/model.BoolTypedFalse
  BoolUntyped:
    model: github.com/99designs/gqlgen/graphql.Boolean
    enum_values:
      TRUE: github.com/99designs/gqlgen/_examples/enum/model.BoolUntypedTrue
      FALSE: github.com/99designs/gqlgen/_examples/enum/model.BoolUntypedFalse

This is our go model package for binding:

package model

type IntTyped int

const (
	IntTypedOne IntTyped = iota + 1
	IntTypedTwo
)

const (
	IntUntypedOne = iota + 1
	IntUntypedTwo
)

type StringTyped string

const (
	StringTypedOne StringTyped = "ONE"
	StringTypedTwo StringTyped = "TWO"
)

const (
	StringUntypedOne = "ONE"
	StringUntypedTwo = "TWO"
)

type BoolTyped bool

const (
	BoolTypedTrue  BoolTyped = true
	BoolTypedFalse BoolTyped = false
)

const (
	BoolUntypedTrue  = true
	BoolUntypedFalse = false
)

Example of generated type binding implementation for IntTyped enum:

func (ec *executionContext) unmarshalNIntTyped2githubᚗcomᚋ99designsᚋgqlgenᚋ_examplesᚋenumᚋmodelᚐIntTyped(ctx context.Context, v interface{}) (model.IntTyped, error) {
	tmp, err := graphql.UnmarshalString(v)
	res := unmarshalNIntTyped2githubᚗcomᚋ99designsᚋgqlgenᚋ_examplesᚋenumᚋmodelᚐIntTyped[tmp]
	return res, graphql.ErrorOnPath(ctx, err)
}

func (ec *executionContext) marshalNIntTyped2githubᚗcomᚋ99designsᚋgqlgenᚋ_examplesᚋenumᚋmodelᚐIntTyped(ctx context.Context, sel ast.SelectionSet, v model.IntTyped) graphql.Marshaler {
	res := graphql.MarshalString(marshalNIntTyped2githubᚗcomᚋ99designsᚋgqlgenᚋ_examplesᚋenumᚋmodelᚐIntTyped[v])
	if res == graphql.Null {
		if !graphql.HasFieldError(ctx, graphql.GetFieldContext(ctx)) {
			ec.Errorf(ctx, "the requested element is null which the schema does not allow")
		}
	}
	return res
}

var (
	unmarshalNIntTyped2githubᚗcomᚋ99designsᚋgqlgenᚋ_examplesᚋenumᚋmodelᚐIntTyped = map[string]model.IntTyped{
		"ONE": model.IntTypedOne,
		"TWO": model.IntTypedTwo,
	}
	marshalNIntTyped2githubᚗcomᚋ99designsᚋgqlgenᚋ_examplesᚋenumᚋmodelᚐIntTyped = map[model.IntTyped]string{
		model.IntTypedOne: "ONE",
		model.IntTypedTwo: "TWO",
	}
)

[coveralls]

coverage: 75.219% (+0.04%) from 75.181%
when pulling e6ea4bc on nikitacrit:enum-values-binding
into 780bf27 on 99designs:master.

[StevenACoffman]

Thanks!

[StevenACoffman]

@Desuuuu @atzedus Mind taking this for a spin and seeing if you think we should change anything before the next release?

[Desuuuu]

Sure, I'll try doing that before the end of the week.

[Desuuuu]

Here are my thoughts so far:

• I find it weird using the @goModel directive for this purpose as I wouldn't call enum values a model and its other parameters do not apply here.
• It doesn't work in the case the enum model is not explicitly using the @goModel directive (auto-binding).
• I think it would make more sense if an error was returned if only some of the enum values are using the directive.

Also, it feels cumbersome to use to be honest. If I'm already binding an enum to a custom model, I'd rather implement the marshaler/unmarshaler interfaces from gqlgen than having to manually bind each enum value to a go value through the schema/config. For example:

enum Day {
    MONDAY
    TUESDAY
}

package model

import (
	"errors"
	"io"

	"github.com/99designs/gqlgen/graphql"
)

type Day int

const (
	Monday  Day = 0
	Tuesday Day = 1
)

func (d Day) MarshalGQL(w io.Writer) {
	switch d {
	case Monday:
		io.WriteString(w, "MONDAY")
	case Tuesday:
		io.WriteString(w, "TUESDAY")
	}
}

func (d *Day) UnmarshalGQL(v any) error {
	switch v {
	case "MONDAY":
		*d = Monday
		return nil
	case "TUESDAY":
		*d = Tuesday
		return nil
	}

	return errors.New("invalid Day")
}

In the case where enum types do not implement any marshaler/unmarshaler, I feel like it would be useful to attempt generating default ones based on the standard EnumNameValueName pattern, and maybe provide a helpful error message if that's not possible.

[nikitacrit]

I find it weird using the @GoModel directive for this purpose as I wouldn't call enum values a model and its other parameters do not apply here.

Do you have any specific suggestions? I agree, It doesn't fit perfect. Enum is really special case. I just find it easier to implement with existing directive. I can try to work on new directive @goEnum(const: ""). But I'm not sure this will be intuitive that enum uses @goModel directive to bind and enum values uses different one. When I was studying gqlgen, the first thing I tried was to bind each enum value through a @goModel directive exactly as it is done in my implementation, but it didn’t work.

It doesn't work in the case the enum model is not explicitly using the @goModel directive (auto-binding).

This is expected behaviour. Its not trivial to find source of truth of target enum go type. I believe that attempts to define the type from the value in directives can lead to problems.

I think it would make more sense if an error was returned if only some of the enum values are using the directive.

Sounds like a limitation. I specifically made sure that this possibility remained. It seems that if the user leave some enum values without a directive, he intentionally wants to leave them without a handler.

Also, it feels cumbersome to use to be honest.

If you stick to clean architecture, layered architecture or something that adheres to low coupling principles you should separate api logic and service logic. You will face a problem of mapping api package models to service package models. You cant modify service models with gqlgen code (like in your example) because service models should not even know about gqlgen or any other api existence. This problem always can be resolved manually in gqlgen resolvers (gqlgen creates empty resolver for each type that cant be autobinded). But sometimes models of both layers looks same. That's why we use @goModel directive to keep it simple and not write boilerplate code. I just want to reduce boilerplate switch case code for enums.

[Desuuuu]

Do you have any specific suggestions? I agree, It doesn't fit perfect. Enum is really special case. I just find it easier to implement with existing directive. I can try to work on new directive @goEnum(const: ""). But I'm not sure this will be intuitive that enum uses @GoModel directive to bind and enum values uses different one. When I was studying gqlgen, the first thing I tried was to bind each enum value through a @GoModel directive exactly as it is done in my implementation, but it didn’t work.

I do think a separate directive for enum values would make more sense as it would only have the relevant option.

Also I think it would be nice if it worked the same with package-level constants and variables, provided they're of the correct type.

This is expected behaviour. Its not trivial to find source of truth of target enum go type. I believe that attempts to define the type from the value in directives can lead to problems.

The non-trivial work is already done by gqlgen so the go type for the enum should already be known by now. You should also be able to verify whether the target value is of the correct type. I wouldn't expect it to matter whether the binding was explicit or implicit.

Sounds like a limitation. I specifically made sure that this possibility remained. It seems that if the user leave some enum values without a directive, he intentionally wants to leave them without a handler.

Sure, it's a limitation but the behaviour right now is that unmarshaling any enum value that has not been annotated results in the zero-value for the enum type and marshaling it produces an empty string which is invalid according to the schema. I don't see any case in which it makes sense.

[nikitacrit]

I do think a separate directive for enum values would make more sense as it would only have the relevant option.

Sure! It's not that hard to implement this way.

Also I think it would be nice if it worked the same with package-level constants and variables, provided they're of the correct type.

Thanks for your attention! I will check case with same package enums. Using constants for enums is more standard but I think it is good idea to support variables too.

The non-trivial work is already done by gqlgen so the go type for the enum should already be known by now. You should also be able to verify whether the target value is of the correct type. I wouldn't expect it to matter whether the binding was explicit or implicit.

I guess I'm confused. If we agree that the source of truth for type definition is @goModel directive on graphql ENUM object then you're absolutely right. But this discussion started from:

It doesn't work in the case the enum model is not explicitly using the @goModel directive (auto-binding).

I think we should add more restrictions. Directives on enum values without directive on enum should not be allowed.

Sure, it's a limitation but the behaviour right now is that unmarshaling any enum value that has not been annotated results in the zero-value for the enum type and marshaling it produces an empty string which is invalid according to the schema. I don't see any case in which it makes sense.

OK! I understand what you want to protect users from. I will fix it.

I also plan to add recipe in web documentation to make it clearer.

[Desuuuu]

I think we should add more restrictions. Directives on enum values without directive on enum should not be allowed.

I was under the impression that you meant this was motivated by technical reasons. If this is not the case, I honestly don't understand the reasoning behind it.

[nikitacrit]

I was under the impression that you meant this was motivated by technical reasons. If this is not the case, I honestly don't understand the reasoning behind it.

I will check it again later but as long as I remember huge amout of job is done by already existing code (before this pr). This code triggers by placing @goModel directive on ENUM object. Using this approach I was able to implement this feature without a lot of code changes. I would like to draw your attention to the fact that @goModel directive could be used on ENUM object long before this pr, it just did not work as well as we would like. The main goal of additional directive on each enum value is to help @goModel on root ENUM object work correct in special casses.

[StevenACoffman]

I think we should add more restrictions. Directives on enum values without directive on enum should not be allowed.

I was under the impression that you meant this was motivated by technical reasons. If this is not the case, I honestly don't understand the reasoning behind it.

I'm also not a fan of adding restrictions because for aesthetic reasons.

[nikitacrit]

I suggest this, if there is any misunderstanding, you can write it as you would like in the same format with code examples.

This will work:

enum Numbers @goModel(model: "./model.Number") {
    ONE @goEnum(value: "./model.NumberOne")
    TWO @goEnum(value: "./model.NumberTwo")
}

This will fail codegen and send user error "unused goEnum directives":

enum Numbers {
    ONE @goEnum(value: "./model.NumberOne")
    TWO @goEnum(value: "./model.NumberTwo")
}

This will fail codegen and send user error "not all enum values binded"

enum Numbers @goModel(model: "./model.Number") {
    ONE @goEnum(value: "./model.NumberOne")
    TWO
}

This will work as before pr:

enum Numbers @goModel(model: "./model.Number") {
    ONE
    TWO
}

[nikitacrit]

progress status: I plan to finish it by the end of the week when I return from vacation.

[nikitacrit]

@StevenACoffman @Desuuuu Hello! I opened pr with fixes and improvements #3014.

I was under the impression that you meant this was motivated by technical reasons. If this is not the case, I honestly don't understand the reasoning behind it.

I rechecked this moment. I want to add to the thoughts that I wrote above that the situation is complicated by how the modelgen works. I would not want to change the way the plugin works and create an exception for this case.