Defined type from a basic type should not need a mandatory scalar + MarshalFunc and UnmarshalFunc

[mstephano]

What happened?

In an existing Golang project where we are trying to add gqlgen, we have defined some types from basic types (string, int, boolean, float) that are being used in other structs. Currently, we need to write a scalar in the GraphQL schema and also do a MarshalFunc and UnmarshalFunc in order to avoid an error message saying <type> is incompatible with <named_type>.

What did you expect?

No error message appear and no need to create a scalar with related MarshalFunc and UnmarshalFunc when go generate.

Minimal graphql.schema and models to reproduce

In a remote package, create some named basic types:

package namedtypes

type (
	NamedString  string
	NamedInt     int
	NamedInt32   int32
	NamedInt64   int64
	NamedBool    bool
	NamedFloat64 float64
)

In another package, create a struct using these named basic types (might need to add this package to autobind):

package usenamedtypes

import "github.com/99designs/gqlgen/integration/namedtypes"

type DummyUser struct {
	NewString  namedtypes.NamedString
	NewInt     namedtypes.NamedInt
	NewInt32   namedtypes.NamedInt32
	NewInt64   namedtypes.NamedInt64
	NewBool    namedtypes.NamedBool
	NewFloat64 namedtypes.NamedFloat64
}

In the GraphQL schema, create a type for the struct:

type DummyUser {
    NewString: String!
    NewInt: Int!
    NewInt32: Int!
    NewInt64: Int!
    NewBool: Boolean!
    NewFloat64: Float!
}

Please note that a PR is coming to fix this.

versions

• go run github.com/99designs/gqlgen version? v0.17.22-dev
• go version? go1.19.3 darwin/arm64

[mstephano]

I am currently trying to fix linting on my PR.

[mstephano]

All linting are fixed, but I now have a security check that failed, and I don't know how to fix it.. some missing file?

[mstephano]

I just noticed a TODO under _examples/scalars about a named boolean needing a scalar with its related MarshalFunc and UnmarshalFunc:

    isBanned: Banned # TODO: This can be a Boolean again once multiple backing types are allowed

This was having this error message when go generate:

validation failed: packages.Load: /.../resolvers.go:21:9: cannot use &(userResolver literal) (value of type *userResolver) as UserResolver value in return statement: *userResolver does not implement UserResolver (missing method IsBanned)

It seems that my PR is fixing this TODO (no need of MarshalFunc and UnmarshalFunc anymore) so here is my related fix commit:
b8eebbd

[StevenACoffman]

I think your original problem is because per the Go spec: https://golang.org/ref/spec#Type_declarations
You're not creating an alias to basic types like string, but defining a new type from them. If what you are looking for is an alias (meaning to have the "NewString" identifier refer to the exact same type as string), instead of:

type NamedString  string

you need to do:

type NewString = string

Then all method calls on NewString will be method calls on string (since both identifier denote the same type) and it will marshall/unmarshal without you having to define custom functions for that. Here is how I would modify your example above:

package namedtypes

type (
	NamedString  = string
	NamedInt     = int
	NamedInt32   = int32
	NamedInt64   = int64
	NamedBool    = bool
	NamedFloat64 = float64
)

But maybe that is not what you are looking for, if that's the case, there may still be other ways around the problem, like type embedding and such.

Often people will deliberately introduce a scalar and define a subtype because they explicitly want more control of how a field is marshalled/unmarshalled. For instance, datetimes, dates without times, times without dates.

If you are proposing that during marshalling/unmarshalling when subtypes lack those methods, we fallback to the basic type, I would want to be careful not to break existing behavior that people rely on for custom marshalling/unmarshalling.

[mstephano]

Here is an example of code in my actual project:

// Manufacturer represent the car manufacturer
type Manufacturer string

// Defines value of Manufacturer
const (
	ManufacturerTesla    Manufacturer = "TESLA"
	ManufacturerHonda    Manufacturer = "HONDA"
	ManufacturerToyota    Manufacturer = "TOYOTA"
	...
)

I want Manufacturer type be returned as string without creating a scalar with related MarshalFunc and UnmarshalFunc.

[StevenACoffman]

Try this:

// Manufacturer represent the car manufacturer
type Manufacturer = string

// Defines value of Manufacturer
const (
	ManufacturerTesla    Manufacturer = "TESLA"
	ManufacturerHonda    Manufacturer = "HONDA"
	ManufacturerToyota    Manufacturer = "TOYOTA"
	...
)

[mstephano]

Yes it works I know, but I would need to update all the existing projects with = everywhere this pattern is used in my employer's code.. not sure they would allow me :(

[mstephano]

Feel free to refuse my PR. I will keep my change in my fork, no worries.

[mstephano]

Often people will deliberately introduce a scalar and define a subtype because they explicitly want more control of how a field is marshalled/unmarshalled. For instance, datetimes, dates without times, times without dates.

If you are proposing that during marshalling/unmarshalling when subtypes lack those methods, we fallback to the basic type, I would want to be careful not to break existing behavior that people rely on for custom marshalling/unmarshalling.

In my integration tests, I did create a scalar with custom marshalling/unmarshalling to make sure that my change is not affecting existing ones.

(at first, my change did affect them but I fixed it by checking on the $type.Definition.BuiltIn in the go template).

Also note that my PR works both ways, with and without the '=' (tests updated accordingly).

[StevenACoffman]

As long as it doesn't break existing functionality, I'm still open to your PR, but I would like to review it more carefully, and I'm off for a holiday vacation. One thing is that in your PR description you use some terms that are unfamiliar to me like Remote named basics instead of the Go specification's terms of new defined type from a basic type.

[mstephano]

You are right, my title reflects where I modified the code instead of using Go specs. I updated the title and removed the word 'remote' because I noticed that I can reproduce my issue with the _examples/scalars and the local type Banned bool by removing the custom marshalling/unmarshalling, remove the scalar Banned in graphql schema (also changed Banned to Boolean) and run go run ../../testdata/gqlgen.go in that folder.

[StevenACoffman]

Please see #2587 which reverts the PRs and re-opens this issue until it can be addressed without causing regressions.