99designs / gqlgen

go generate based graphql server library
https://gqlgen.com
MIT License
9.97k stars 1.17k forks source link

Input types: null vs unspecified #505

Closed danilobuerger closed 5 years ago

danilobuerger commented 5 years ago

Expected Behaviour

Per spec:

If the value null was provided for an input object field, and the field’s type is not a non‐null type, an entry in the coerced unordered map is given the value null. In other words, there is a semantic difference between the explicitly provided value null versus having not provided a value.

Actual Behavior

Right now its not possible (as far as I know) to differentiate between null and not having provided a value.

Minimal graphql.schema and models to reproduce

input UpdateUserInput {
  id: ID!
  name: String
  age: Int
}

produces

type UpdateUserInput struct {
    ID   string  `json:"id"`
    Name *string `json:"name"`
    Age  *int    `json:"age"`
}

If the user now submits the input with only id and name, I don't know what to do with the age. Did the user want to clear his age (i.e. set it to null) or did he not want to update it (i.e. not specified)?

mathewbyrne commented 5 years ago

I think the previous statement in the spec is probable the relevant part here:

If no default value is provided and the input object field’s type is non‐null, an error should be thrown. Otherwise, if the field is not required, then no entry is added to the coerced unordered map.

Unfortunately this is probably part of the spec that is a little too coupled to it's JS reference implementation. We want the type safety of a generated struct from your input type, but there's no immediately obvious way to differentiate between a null value and a value that was not provided.

I'm not 100% sure of a good solution here. Part of me thinks trying to differentiate like this might be a bit of a design smell, but I also think your use-case of unset vs ignore is valid. @vektah any thoughts?

vektah commented 5 years ago

There is no way to represent undefined on a struct, to get the raw unordered map instead of a struct you can ask for it:

models:
  UpdateUserInput:
    model: map[string]interface{}

But you will need to deal with casting yourself.

A better api is probably one that moves away from random field updates, and has separate updateAge, updateName mutations where you can put validation and buisiness logic.

danilobuerger commented 5 years ago

A few options:

type UpdateUserInput struct {
    ID   string  `json:"id"`
    Name NullableString `json:"name"`
}
type NullableString struct {
    String  *string
    Touched bool
}

Then if Touched == false its unspecified.

type UpdateUserInput struct {
    ID   string  `json:"id"`
    Name NullableString `json:"name"`
}
func (i *UpdateUserInput) Name(v *string) {
  i.Name = NullableString{v, true}
}

(or any other way of keeping tracked if it was unspecified)

Currently only the parsed Args are available in ResolverContext. This could be changed to also include rawArgs. Then I don't have to do any casting myself and can look up if the field was specified.

vektah commented 5 years ago

You can already do 1 but you need to implement the custom scalars interface.

You can already get raw args by doing https://github.com/99designs/gqlgen/issues/505#issuecomment-456999324.

2 is interesting, but lets track it separatly.

danilobuerger commented 5 years ago

A better api is probably one that moves away from random field updates, and has separate updateAge, updateName mutations where you can put validation and buisiness logic.

I don't think thats a good assumption to make. Coarse vs fine mutations, different people will see this differently. Shouldn't a library like this cater to both and rely on the spec?

You can already do 1 but you need to implement the custom scalars interface.

I don't actually see how this can be achieved while keeping the scalar in the schema as String. As far as I can tell from the docs, I can either add a custom type (don't want that as it should be String) or for a type I don't control, but then I would have to return a string in UnmarshalString. Is there another way thats not documented?

You can already get raw args by doing #505 (comment).

But I loose everything else. Thats why I think it would be better to access it via the context.

2 is interesting, but lets track it separatly.

If that is something you guys are interested in (see #506), I would create a PR for it.

vektah commented 5 years ago

I don't actually see how this can be achieved while keeping the scalar in the schema as String. As far as I can tell from the docs, I can either add a custom type (don't want that as it should be String) or for a type I don't control, but then I would have to return a string in UnmarshalString. Is there another way thats not documented?

Scalars dont need to be strings, they are just undivisible in the graph. You should be able to create a NullableString as you describe by implementing the marshaler interface.

But I loose everything else. Thats why I think it would be better to access it via the context.

What do you lose? In order to work with raw args you're going to need to typecast.

danilobuerger commented 5 years ago

Scalars dont need to be strings, they are just undivisible in the graph. You should be able to create a NullableString as you describe by implementing the marshaler interface.

When I try this

type UpdateUserInput struct {
    ID   string  `json:"id"`
    Name NullableString `json:"name"`
}
type NullableString struct{}
func (s *NullableString) UnmarshalGQL(v interface{}) error {
    return nil
}
func (s NullableString) MarshalGQL(w io.Writer) {
}

it returns an error

field has wrong type: *string is not compatible with a.b.c.NullableString. 

As I said I want to keep String in the schema itself and not use a custom scalar. Is there something I am missing? Maybe you could please show an example so it becomes clearer?

What do you lose? In order to work with raw args you're going to need to typecast.

If I have to use map[string]interface{} for UpdateUserInput I loose the whole type safety and automatic unmarshaling. Whats the reason for not including rawArgs in the ResolverContext so they are easily gettable?

vektah commented 5 years ago

Ah, part of the work in 0.8 is to allow multiple backing go types for a given graph type. This will allow you to add a custom string implementation to be used for that one context.

If I have to use map[string]interface{} for UpdateUserInput I loose the whole type safety and automatic unmarshaling.

You are never going to get type safety from a map[string]interface{}, in context or otherwise.

Whats the reason for not including rawArgs in the ResolverContext so they are easily gettable?

Its already on resolver context as Args, the only difference between that map and what gets passed to the method is a final type assertion.

danilobuerger commented 5 years ago

You are never going to get type safety from a map[string]interface{}, in context or otherwise.

Its already on resolver context as Args, the only difference between that map and what gets passed to the method is a final type assertion.

Thats not what I mean. The current Args in the ResolverContext are already cast to their respective structs. So in order to check if the field was specified I could look it up in rawArgs if it was specified. I can't do that with the current Args. And therefor, the type safety of rawArgs does not matter, because I get the usual struct, but have additionally the rawArgs to look up if each field was actually specified.

Anyway that was just one of my proposed solutions. Being able to have a custom string implementation (1. solution) or setters (2. solution) would solve the problem in a nicer way.

vektah commented 5 years ago

Anyway that was just one of my proposed solutions. Being able to have a custom string implementation (1. solution) or setters (2. solution) would solve the problem in a nicer way.

Ok, I'm going to close this in favour of input setters.

desdic commented 5 years ago

Did any of you ever got this working ? I took the tutorial and expaned it with a nullablestring like:

schema: type Todo { id: ID! text: NullableString done: Boolean! user: User! }

type User { id: ID! name: String! }

type Query { todos: [Todo!]! }

input NewTodo { text: NullableString userId: String! }

type Mutation { createTodo(input: NewTodo!): Todo! }

scalar NullableString

todo.go: type NullableString struct { String *string Touched bool }

func (n *NullableString) UnmarshalGQL(v interface{}) error { n.Touched = true s, ok := v.(string) if !ok { return fmt.Errorf("Not a string, got %v", v) }

    n.String = &s
    return nil

}

func (n NullableString) MarshalGQL(w io.Writer) { if !n.Touched { fmt.Fprintf(w, "nill") // just to test with a value } else if n.Touched && n.String == nil { fmt.Fprintf(w, "null") } else { fmt.Fprintf(w, "%s", *n.String) } }

type Todo struct { ID string Text NullableString Done bool UserID string }

If I send this request:

mutation createTodo { createTodo(input:{userId:"1"}) { user { id } text done } } I would expect that text is null .. its not its 'nill'

If I send:

mutation createTodo { createTodo(input:{text:null, userId:"1"}) { user { id } text done } }

I would expect the nullablestring to have set Touched but its not .. I get nill again.

It seems that if the input value has the value null the UnmarshalGQL is never called. Can anyone tell me what I'm missing ?

jszwedko commented 5 years ago

Just for reference, it doesn't seem like a custom implementation of the String scalar helps with this as UnmarshalGQL isn't called when the field is explicitly set to null in the input; i.e. it still isn't possible to determine if the field is absent or if it is explicitly set to null.

steebchen commented 5 years ago

So what's the recommended way to do this? As @jszwedko mentioned the NullString does not work and it seems that #506 is also not implemented yet, which would mean there's no solution at the moment.

I also had an idea where the implementation could use a double pointer (ugly, but it would work) like **string to differentiate between null and undefined

RichardLindhout commented 4 years ago

This seems to work and results in types which are explicitly set by the user!

    yes, ok := requestContext.Variables["input"].(map[string]interface{})
    fmt.Println(ok)
    for key, _ := range yes {
        fmt.Println(key)
    }
briskt commented 4 years ago

Still looking for a workaround for this. Best we've come up with is to never omit fields so we know that a null is really meant to be removing a value. But that makes more work on the client side to send field values other than what needs to be changed. I tried accessing Variables as @RichardLindhout suggested, but it's always empty.

RichardLindhout commented 4 years ago

I generate code which return whitelisted sqlboiler update keys based in the input so I know which fields are set and which not. https://github.com/web-ridge/gqlgen-sqlboiler

func getInputFromContext(ctx context.Context, key string) map[string]interface{} {
    requestContext := graphql.GetRequestContext(ctx)
    m, ok := requestContext.Variables[key].(map[string]interface{})
    if !ok {
        fmt.Println("can not get input from context")
    }
    return m
}
func {{ .Name|go }}ToBoilerWhitelist(ctx context.Context, extraColumns ...string) boil.Columns {
            input := getInputFromContext(ctx, "input")
            columnsWhichAreSet := []string{}
            for key, _ := range input {
                switch key {
                    {{ range $field := .Fields -}}
                    case "{{ $field.CamelCaseName }}":
                        columnsWhichAreSet = append(columnsWhichAreSet, models.{{ $model.BoilerName|go }}Columns.{{- $field.BoilerName|go }})
                    {{ end -}}
                }
            }
            columnsWhichAreSet = append(columnsWhichAreSet, extraColumns...)
            return boil.Whitelist(columnsWhichAreSet...)
        }
briskt commented 4 years ago

@RichardLindhout I tried that, but I just get an empty map in RequestContext.Variables.

@danilobuerger, have you come up with a workable alternative? Haven't seen anything from you recently.

RichardLindhout commented 4 years ago

@Schparky I think maybe because I'm specifying that the input should be called input maybe you don't have that

input := getInputFromContext(ctx, "input")

Based on Schermafbeelding 2020-02-17 om 17 28 18

https://blog.apollographql.com/designing-graphql-mutations-e09de826ed97

briskt commented 4 years ago

@RichardLindhout , thanks, I have that. The difference comes before, though. If I print the Variables field, like:

    requestContext := graphql.GetRequestContext(ctx)
    fmt.Printf("------ vars: %+v\n", requestContext.Variables)

I get this

------ vars: map[]

I'm still on 0.10.1. Perhaps 0.10.2 changes this? I've seen that GetRequestContext is deprecated in favor of GetOperationContext but not sure if that has anything to do with what I'm seeing.

RichardLindhout commented 4 years ago

I think it probably does, will update in the future to the new gqlgen to test this.

frederikhors commented 3 years ago

@RichardLindhout are you still using your solution with getInputFromContext()?

Or is there something simpler today?

RichardLindhout commented 3 years ago

Yes: https://github.com/web-ridge/utils-go/blob/main/boilergql/input.go#L11

So only these fields get force updated and the other ones are skipped.

frederikhors commented 3 years ago

I think we should re-open this, please, @vektah.

frederikhors commented 3 years ago

@jszwedko did you find a solution for this?

TuringJest commented 3 years ago

@RichardLindhout @Schparky how did you solve this? I'm on 0.13.0 and my requestContext.Variables is also empty. Is this a bug, do we have to build our own map from requestContext.Operation.SelectionSet now?

briskt commented 3 years ago

@TuringJest I'm not sure. We just worked within the confines of what gqlgen provides, and defined our own rules. For instance, if a property is nullable, a null/empty value nullifies it. That means that the consumer must always provide the old value if they do not intend to change it. Awkward but workable. Actually, for other reasons, we have moved away from gqlgen and GraphQL in general. So I don't intend to look for a better solution.

RichardLindhout commented 3 years ago

@RichardLindhout @Schparky how did you solve this? I'm on 0.13.0 and my requestContext.Variables is also empty. Is this a bug, do we have to build our own map from requestContext.Operation.SelectionSet now?

I did solve this

Yes: https://github.com/web-ridge/utils-go/blob/main/boilergql/input.go#L11

I'm creating automatic converts for my ORM models https://github.com/web-ridge/gqlgen-sqlboiler-examples/blob/main/social-network/helpers/convert_input.go#L36 https://github.com/web-ridge/gqlgen-sqlboiler-examples/blob/main/social-network/helpers/convert_input.go#L52 So only these fields get force updated and the other ones are skipped.

RichardLindhout commented 3 years ago

See here how to fetch keys https://github.com/web-ridge/gqlgen-sqlboiler-examples/blob/4e6cce8c097febf145ca0fd7715e99f8c53df860/social-network/resolver.go#L65

RichardLindhout commented 3 years ago

The GetInputFromContext code: https://github.com/web-ridge/utils-go/blob/main/boilergql/input.go#L11

func GetInputFromContext(ctx context.Context, key string) map[string]interface{} {
    requestContext := graphql.GetOperationContext(ctx)
    variables := requestContext.Variables

    // pick nested inputs
    var ok bool
    for _, splitKey := range strings.Split(key, ".") {
        variables, ok = variables[splitKey].(map[string]interface{})
        if !ok {
            fmt.Println("can not get input from context for key: "+key+" splitted key: ", splitKey)
        }
    }

    return variables
}
frederikhors commented 3 years ago

Actually, for other reasons, we have moved away from gqlgen and GraphQL in general.

@Schparky can I ask you what are you using now that you don't use GraphQL anymore?

briskt commented 3 years ago

Actually, for other reasons, we have moved away from gqlgen and GraphQL in general.

@Schparky can I ask you what are you using now that you don't use GraphQL anymore?

Good, old fashioned RESTful API. Among other reasons, we decided that our application didn't need GraphQL because we only have one client. So the dynamic benefit of the GraphQL API wasn't really a payoff.

frederikhors commented 3 years ago

Are you using some magic JS library in the browser app (if any) for state-while-revalidation cache?

briskt commented 3 years ago

@frederikhors Our client-side cache is not all that sophisticated. Just using Svelte stores as a simple write-through cache with periodic refresh. Want to take the conversation over to the Gophers Slack or Discord? (I'm Schparky on Slack and redspige on discord.)

TuringJest commented 3 years ago

The GetInputFromContext code: https://github.com/web-ridge/utils-go/blob/main/boilergql/input.go#L11

func GetInputFromContext(ctx context.Context, key string) map[string]interface{} {
  requestContext := graphql.GetOperationContext(ctx)
  variables := requestContext.Variables

  // pick nested inputs
  var ok bool
  for _, splitKey := range strings.Split(key, ".") {
      variables, ok = variables[splitKey].(map[string]interface{})
      if !ok {
          fmt.Println("can not get input from context for key: "+key+" splitted key: ", splitKey)
      }
  }

  return variables
}

@RichardLindhout Maybe there is something really basic I'm missing...

I just tried it again on the CreateReview mutation from the gqlgen Starwars example but Variables inside the OperationContext is always empty.

func (r *mutationResolver) CreateReview(ctx ... 

    v := graphql.GetOperationContext(ctx).Variables    // nil

The only field with useful data is OperationContext.Operation.SelectionSet.Arguments

Is anything else required to get this to work?

@Schparky I wasn't too happy with sending all the fields back as we have rather big objects and switched to merging the input with mapstructure instead. But only updating from selected fields seems much easier to work with...

molon commented 4 weeks ago
// Helper function to recursively collect argument fields, including nested fields
func CollectArgumentFields(ctx context.Context) map[string]any {
    argumentFields := map[string]any{}
    fieldCtx := graphql.GetFieldContext(ctx)
    if fieldCtx == nil {
        return argumentFields
    }

    // Loop through the arguments of the current field
    for _, arg := range fieldCtx.Field.Arguments {
        argMap := map[string]any{}
        argumentFields[arg.Name] = argMap
        processValueChildren(arg.Value.Children, argMap)
    }

    return argumentFields
}

// Recursive function to process nested fields and build a map[string]any
func processValueChildren(children []*ast.ChildValue, parentMap map[string]any) {
    for _, child := range children {
        if len(child.Value.Children) > 0 {
            // If there are nested fields, create a new map for the child
            childMap := map[string]any{}
            parentMap[child.Name] = childMap
            processValueChildren(child.Value.Children, childMap)
        } else {
            // Otherwise, just mark the field as present
            parentMap[child.Name] = nil
        }
    }
}
    inputFields, _ := CollectArgumentFields(ctx)["input"].(map[string]any)
    if _, ok := inputFields["description"]; ok {
        user.Description = input.Description
    }