Bad JSON marshaling for custom Marshaler

[limoli]

I am using the library github.com/guregu/null in order to provide an easy management of null fields on my database entities. The library provides the following structures:

• null.String
• null.Int
• null.Bool
• null.Time

Every structure implements JSONMarshaler and JSONUnmarshaler interfaces, so a simple json.Marshalreturns the expected JSON output.

Expected Result (Simple json.Marshal)

{
    "mobile": "1234567891"
}

Current Result (GraphQL response)

{
    "mobile": "{{1234567891 true}}"
}

I imagine that this happens because the structure has two exported fields (Value and Valid). But this should not happen since the structure implements the JSON marshaler and unmarshaler interfaces.

I think that the problem is in scalars.go file. Take String scalar as example. The coerceString casts the string to a string pointer when the interface implements the Marshaler interface.

func coerceString(value interface{}) interface{} {
	if v, ok := value.(*string); ok {
		if v == nil {
			return nil
		}
		return *v
	}
	return fmt.Sprintf("%v", value)
}

// String is the GraphQL string type definition
var String = NewScalar(ScalarConfig{
	Name: "String",
	Description: "The `String` scalar type represents textual data, represented as UTF-8 " +
		"character sequences. The String type is most often used by GraphQL to " +
		"represent free-form human-readable text.",
	Serialize:  coerceString,
	ParseValue: coerceString,
	ParseLiteral: func(valueAST ast.Value) interface{} {
		switch valueAST := valueAST.(type) {
		case *ast.StringValue:
			return valueAST.Value
		}
		return nil
	},
})

I don't know what is the best practice for this library, but I would like to get help about this and provide a smart solution.

[limoli]

I just fixed this creating my own custom scalars. I will make a public repository if someone is interested.

[Fontinalis]

@limoli Thank you for reporting the issue! Sorry, I just got to look into this.

I think we have to fix this issue since many of us are using some kind of custom type in their schemas, so this feature would make our work easier and faster.

And the solution would look like this I think:

func coerceString(value interface{}) interface{} {
	if v, ok := value.(json.Marshaler); ok {
		return v
	} else if v, ok := value.(*string); ok {
		if v == nil {
			return nil
		}
		return *v
	}
	return fmt.Sprintf("%v", value)
}

And probably that goes into the other coerce functions too.

/cc @egonelbre @chris-ramon

[limoli]

Very good idea @Fontinalis! This could be a very elegant and fast fix!

[Fontinalis]

I'll make a PR this weekend.

[limoli]

I think that there are still some issues about the compatibility between nullable fields and this library. I attach some images in order to clarify better.

Expected deletedAt: null DeletedAt is a nullable datetime field coming from a structure where it is declared as DeletedAt null.Time.

Result deletedAt: "null" DeletedAt value is converted to a string because of Serialize interface method of this library.

Debug

Bug I think that the problem is on serializeDateTime function in scalars.go. As you can see in the images below, aMarshalText can return an array of bytes, but the result can be a null value. This value is returned as a string that is not correct since the function can manage and return nil values.

[limoli]

Since I think that GraphQL library hasn't to handle this case, I will write a custom scalar for nullable date times compatible with null.Time type.

[limoli]

The fixed library problem I discovered that the nullable library had a problem with the null.Time text-marshaling. It returned "null" instead of an empty string. Now I fixed that and if a time is nullable, MarshalText implementation returns an empty string.

Error is still out there However, the problem is still here. Why does a graphql.DateTime field return the string "null" instead of a JSON null? For what I can see from Graphql code, the datetime serialization function works as below:

func serializeDateTime(value interface{}) interface{} {
	if v, ok := value.(encoding.TextMarshaler); ok {
		bs, err := v.MarshalText()
		if err == nil {
			return string(bs)
		}
	}
	switch value := value.(type) {
	case time.Time:
		buff, err := value.MarshalText()
		if err != nil {
			return nil
		}

		return string(buff)
	case *time.Time:
		if value == nil {
			return nil
		}
		return serializeDateTime(*value)
	default:
		return nil
	}
}

Implementing the text Marshaler, the passed value should immediately return the empty string. However, I don't know why the output is still "null".

Serialize method is used by completeLeafValue function:

// completeLeafValue complete a leaf value (Scalar / Enum) by serializing to a valid value, returning nil if serialization is not possible.
func completeLeafValue(returnType Leaf, result interface{}) interface{} {
	serializedResult := returnType.Serialize(result)
	if isNullish(serializedResult) {
		return nil
	}
	return serializedResult
}

Then the problem has this location:

// Returns true if a value is null, undefined, or NaN.
func isNullish(src interface{}) bool {
	if src == nil {
		return true
	}
	value := reflect.ValueOf(src)
	if value.Kind() == reflect.Ptr {
		if value.IsNil() {
			return true
		}
		value = value.Elem()
	}
	switch value.Kind() {
	case reflect.String:
		// if src is ptr type and len(string)=0, it returns false
		if !value.IsValid() {
			return true
		}
	case reflect.Int:
		return math.IsNaN(float64(value.Int()))
	case reflect.Float32, reflect.Float64:
		return math.IsNaN(float64(value.Float()))
	}
	return false
}

I hope to close this problem once and for all. @Fontinalis

[limoli]

The possible solution

Analyzing the graphql code, I understood how it works and where the "problem" is. In a few words, we should check if the serialized result is a nil value or an empty string.

However, I am not very satisfied with this implementation since TextMarshaler should return a not nil bytes slice in case of success (error = nil).

So, we could keep only the empty string condition. Indeed a valid datetime shouldn't be an empty string. I don't know exactly how the library manages everything, but essentially I see these cases:

1. NOT NULLABLE: graphql.NewNonNull(graphql.DateTime)

2. NULLABLE: graphql.DateTime

3. If you pass an empty string to a NOT NULLABLE, an error should be retrieved since the datetime value is not correct (because considered nullable with the new logic).

4. If you pass an empty string to a NULLABLE, the null error is retrieved.

So, the best fix is the following one:

1. Check serialized result
2. If there is no error and the result is an empty string, return nil

What do you think about this @Fontinalis ?

func serializeDateTime(value interface{}) interface{} {
	if v, ok := value.(encoding.TextMarshaler); ok {
		bs, err := v.MarshalText()
		if err == nil {
                       // THIS IS THE CHANGE
			if len(bs) == 0{
				return nil
			}
			return string(bs)
		}
	}
	switch value := value.(type) {
	case time.Time:
		buff, err := value.MarshalText()
		if err != nil {
			return nil
		}

		return string(buff)
	case *time.Time:
		if value == nil {
			return nil
		}
		return serializeDateTime(*value)
	default:
		return nil
	}
}

[limoli]

I have just tested that this little fix and it works perfectly to me:

if bs == nil || len(bs) == 0{
	return nil
}

@Fontinalis

[scottix]

Any hope this comes to fruition. I can write my own types but they just become redundant and make the documentation more confusing. All my types have a marshaling function. I feel like it should be easy to incorporate.