Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Input Schema optional #866

Closed
mlazuardy-dyned opened this issue Sep 18, 2019 · 35 comments
Closed

Make Input Schema optional #866

mlazuardy-dyned opened this issue Sep 18, 2019 · 35 comments

Comments

@mlazuardy-dyned
Copy link

What happened?

I have some minimal apps using gqlgen that need to update the User Model, updating the using

But when regenerate gqlgen it should use pointers for the password where the password are not inputed from user and then causing (nil pointer)

What did you expect?

So when updating the User , the user can optionally input password or not.

Minimal graphql.schema and models to reproduce

type Mutation {
    updateUser(input: UpdateUser!): User!
}

input UpdateUser {
    id: ID!
    name: String!
    password: String
}

versions

  • gqlgen version? V.0.9.3
  • go version? 1.12.9
  • dep or go modules? gomod
@asiman161
Copy link

you need to use if in your resolver to solve this problem.

func(r *mutationResolver) updateUser(ctx context.Context, input UpdateUser) {
    if input.Password != nil {
        doWhatYouWant(input.Password)
    }
}

@stale
Copy link

stale bot commented Dec 1, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Dec 1, 2019
@maddoger
Copy link

maddoger commented Dec 4, 2019

The problem is there is no difference between undefined and null value in code.
For example there is an input type:

input {
  ...
  categoryId: ID
}

And you can use it in three different ways:

  1. input: { categoryId: "someid" } - you want to set a new value
  2. input: { categoryId: null } - you want to reset to null
  3. input: {} - you don't want to change categoryId

In cases 2 and 3 you will get input.categoryId == nil. So you can't figure out what to do.

@stale stale bot removed the stale label Dec 4, 2019
@asiman161
Copy link

if you do some params not required then it will be a pointer otherwise it will be a value.
so if you write input Obj {id: Int} -> pointer and could be nil so you must to check if it nil.
if you write input Obj {id: Int!} it will be a value and can't be nil.

@maddoger
Copy link

maddoger commented Dec 4, 2019

That's the problem. You can't set required field to null, but you need it to reset the value. So it needs to be optional.
And if it's optional there is no difference between passed null value and not set value: it always will be just nil.

@stale
Copy link

stale bot commented Feb 23, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Feb 23, 2020
@frederikhors
Copy link
Collaborator

Nope @Stale...

@stale stale bot removed the stale label Feb 23, 2020
@stale
Copy link

stale bot commented Apr 23, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 23, 2020
@frederikhors
Copy link
Collaborator

Nope @Stale...

@stale stale bot removed the stale label Apr 23, 2020
@cwimberger
Copy link

I have to fall back to map[string]interface{} in such cases. Is there a better way to write it at the moment?

func (r *mutationResolver) UpdatePackage(ctx context.Context, packageID int, input map[string]interface{}) (*Package, error) {
   orderDate, exists := input["orderDate"]
   if exists {
      if orderDate == nil {
         // set to null
      } else {
         // set to value
      }
   }

   ...

@stale
Copy link

stale bot commented Aug 29, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 29, 2020
@frederikhors
Copy link
Collaborator

Nope, stale.

@stale stale bot removed the stale label Aug 29, 2020
@stale
Copy link

stale bot commented Nov 28, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 28, 2020
@NateScarlet
Copy link

A workaround is to use an empty string as null value, so there is no need to check whether the input value is null.

@NateScarlet
Copy link

gqlgen should provide a better way than use a map[string]interface{} as model, maybe auto generate models like sql.Null* with directive @goField(nullable: true) so we can check field type by value.IsNull

@frederikhors
Copy link
Collaborator

What's the problem with using something like this: https://gqlgen.com/reference/changesets/ ?

@NateScarlet
Copy link

- **gqlgen priortizes Type safety** — You should never see `map[string]interface{}` here.

@frederikhors
Copy link
Collaborator

I'm using map[string]interface{} for input types only.

I use mapstructure for each of them and than I work on normal (non-input) types.

Is there a way (today) to not handwrite the following code for each field?

if input.PhoneNumber != nil {
    output.PhoneNumber = *input.PhoneNumber
}
if input.Password != nil {
    output.Password = *input.Password
}

@NateScarlet
Copy link

map[string]interface{} has no build time type check

@NateScarlet
Copy link

NateScarlet commented Dec 2, 2020

Is there a way (today) to not handwrite the following code for each field?

if input.PhoneNumber != nil {
    output.PhoneNumber = *input.PhoneNumber
}
if input.Password != nil {
    output.Password = *input.Password
}

Set each field explicitly looks good to me.
And you can always convert a struct to map, like:

// MapFromStruct convert struct to map.
// returns nil if input value is not a struct.
func MapFromStruct(v interface{}) (ret map[string]interface{}) {
	var rv = reflect.ValueOf(v)
	var rt = rv.Type()
	if rt.Kind() != reflect.Struct {
		return
	}
	ret = make(map[string]interface{})
	for i := 0; i < rv.NumField(); i++ {
		var f = rt.Field(i)
		// Skip un-exported fields.
		if f.PkgPath != "" {
			continue
		}
		ret[f.Name] = rv.Field(i).Interface()
	}
	return
}

@frederikhors
Copy link
Collaborator

frederikhors commented Dec 2, 2020

Why are you suggesting to transform the struct into a map? Doesn't gqlgen do this automatically if I configure it in gqlgen.yml with:

models:
  UserChanges:
    model: "map[string]interface{}"

I too would like to avoid map and reflect in my code altogether and would mostly like to generate code automatically.

I am stuck like others people at the point where I don't know if in the coming input object the fields are null or empty. And this is ESSENTIAL in some usecase.

Can I ask you to write a usage example with types like sql.Null?

There is no other way to accomplish this besides map[string]interface{} and sql.Null, is there?

Thanks for your help, really.

@NateScarlet
Copy link

NateScarlet commented Dec 2, 2020

@frederikhors I thought you want to use mapstructure to work with struct

Usage example

input UpdateTodoInput {
  id: ID!
  deadline: Time @goField(nullable: true)
}

type Mutation {
    updateTodo(input: UpdateTodoInput!) Boolean
}

expected resolver

type Time = time.Time

type NullTime struct {
	 Time Time
	 IsNull bool
}

type UpdateTodoInput struct {
	ID string
	Deadline *NullTime
}

func (r *mutationResolver) updateTodo(ctx context.Context, input UpdateTodoInput) (*bool, error) {
	if input.Deadline == nil {
		print("input value is undefined")
	} else if input.Deadline.IsNull {
		print("input value is null")
	} else {
		print(input.Deadline.Time)
	}
	panic("not implemented")
}

For now we can also check ArgumentMap from context, mentioned by #977 (comment)
But it is difficult to use when null field is in a array.

@frederikhors
Copy link
Collaborator

Thank you so much. These are invaluable advice.

Can I ask you to explain But it is difficult to use when null field is in a array?

Thanks again.

@NateScarlet
Copy link

Because if null field in a array, like:

input UpdateInputTodoInputData! {
    id: ID!
    deadline: Time
}

input UpdateTodoInput {
    data: [UpdateInputTodoInputData!]
}

Then current index in array is required to check whether deadline is null. So i have to pass index to other function just for this, or create a intermediate struct to carry deadline type info.

@frederikhors
Copy link
Collaborator

frederikhors commented Dec 2, 2020

Ok, just to understand if I get it.

Today we can handle the null / not-specified field issue with:

  1. model: "map[string]interface{}" (https://gqlgen.com/reference/changesets/) or

  2. nullable types (Make Input Schema optional #866 (comment)) or

  3. checking ArgumentMap from context (changesets without using reflections? #977 (comment))

I'll briefly share what I'm using today to figure out if I'm SERIOUSLY wrong:

  • player.graphql
type Player {
  name: String!
  notes: String
}

input PlayerInput {
  name: String
  notes: String
}
  • gqlgen.yml
---
models:
  PlayerInput:
    model: "map[string]interface{}"
  • player.go
type Player struct {
	Name   string `validate:"required"`
	Notes  *string
}
  • player.resolvers.go
func (r *mutationResolver) PlayerCreate(ctx context.Context, input map[string]interface{}) (*entities.Player, error) {
	return r.Controllers.Player.Create(ctx, input)
}

func (r *mutationResolver) PlayerUpdate(ctx context.Context, id int, input map[string]interface{}) (*entities.Player, error) {
	return r.Controllers.Player.Update(ctx, id, input)
}
  • player.controller.go
func (controller *controller) Create(ctx context.Context, input map[string]interface{}) (*entities.Player, error) {
	var finalPlayer entities.Player

  mergeInputToFinalPlayer(input, &finalPlayer)
  
  validateFinalPlayer(&finalPlayer)

  businessLogicBeforeCreate(&finalPlayer)

	controller.Create(ctx, &finalPlayer)

	return &finalPlayer, nil
}

As you can see, all the validation and business logic is therefore on the final types (Player), not on the input types (PlayerInput).

And hence my question: what do you think could happen so badly?

I know that I lose type safety, but having GraphQL as a contract for API users I think it can be fine (ex: if there are more fields on the wrong request a "field not existing" error is issued).

Am I missing something?

Is it a good-enough way in your opinion?

Again, thanks for your precious time. 😄

@NateScarlet
Copy link

@frederikhors nullable types is not implemented by gqlgen, just a idea to handle this problem.

type safety is why i like gqlgen, map solution is not good enough for me as:

  • when writting mergeInputToFinalPlayer first time, IDE can auto-complete field name, but not map keys. unnoticed typo will result a runtime error.

  • when someone changed entities.Player type in a incompatible way. I want a build error rather than a runtime error.

  • "field not existing" is the kind of error that gqlgen should handled before resolver call.

  • map is completely avoidable even today, just check ArgumentMap by write a util function like func ArgumentIsNull(ctx context.Context, path string) bool

@NateScarlet
Copy link

NateScarlet commented Dec 3, 2020

I wrote a util function to check value IsNull

import (
	"context"

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

type argumentSelector = func(v interface{}) (ret interface{}, ok bool)

// ArgumentsQuery to check whether arg value is null
type ArgumentsQuery struct {
	args      map[string]interface{}
	selectors []argumentSelector
}

func (a ArgumentsQuery) selected() (ret interface{}, ok bool) {
	ret, ok = a.args, true
	for _, fn := range a.selectors {
		ret, ok = fn(ret)
		if !ok {
			break
		}
	}
	return
}

// IsNull return whether selected field value is null.
func (a ArgumentsQuery) IsNull() bool {
	v, ok := a.selected()
	return ok && v == nil
}

func (a ArgumentsQuery) child(fn argumentSelector) ArgumentsQuery {
	var selectors = make([]argumentSelector, 0, len(a.selectors)+1)
	selectors = append(selectors, a.selectors...)
	selectors = append(selectors, fn)
	return ArgumentsQuery{
		args:      a.args,
		selectors: selectors,
	}
}

// Field select field by name, returns a new query.
func (a ArgumentsQuery) Field(name string) ArgumentsQuery {
	return a.child(func(v interface{}) (ret interface{}, ok bool) {
		var m map[string]interface{}
		if m, ok = v.(map[string]interface{}); ok {
			ret, ok = m[name]
		}
		return
	})

}

// Index select field by array index, returns a new query.
func (a ArgumentsQuery) Index(index int) ArgumentsQuery {
	return a.child(func(v interface{}) (ret interface{}, ok bool) {
		if index < 0 {
			return
		}
		var a []interface{}
		if a, ok = v.([]interface{}); ok {
			if index > len(a)-1 {
				ok = false
				return
			}
			ret = a[index]
		}
		return
	})
}

// Arguments query to check whether args value is null.
// https://github.com/99designs/gqlgen/issues/866
func Arguments(ctx context.Context) (ret ArgumentsQuery) {
	fc := graphql.GetFieldContext(ctx)
	oc := graphql.GetOperationContext(ctx)

	if fc == nil || oc == nil {
		return
	}
	ret.args = fc.Field.ArgumentMap(oc.Variables)
	return
}

Usage:

func (r *mutationResolver) updateTodo(ctx context.Context, input UpdateTodoInput) (*bool, error) {
	inputArgs = Arguments(ctx).Field("input")
	for index, d := range input.Data {
		dataArgs = inputArgs.Field("data").Index(index)
		if dataArgs.Field("deadline").IsNull() {
			print("input value is null")
		} else if d.Deadline == nil {
			print("input value is undefined")
		} else {
			print(d.Deadline)
		}
	}
	panic("not implemented")
}
``

@frederikhors
Copy link
Collaborator

@NateScarlet, your work here is amazing!

What I'm afraid of is writing all this code manually:

func (r *mutationResolver) updateTodo(ctx context.Context, input UpdateTodoInput) (*bool, error) {
	inputArgs = Arguments(ctx).Field("input")
	for index, d := range input.Data {
		dataArgs = inputArgs.Field("data").Index(index)
		if dataArgs.Field("deadline").IsNull() {
			print("input value is null")
		} else if d.Deadline == nil {
			print("input value is undefined")
		} else {
			print(d.Deadline)
		}
	}
	panic("not implemented")
}

In this code there are multiple strings that don't help in IDE or at build time. Or am I wrong?

  1. "input"

  2. "data"

  3. "deadline"

And I have to write this code for each field of my structs!

Hard to think you can write it by hand, right?

Were you thinking about automatic code generation?

@NateScarlet
Copy link

Yes, this is the limitation for ArgumentMap solution. when you have typo on field name, it just returns false.

Auto generated null type by gqlgen still a better way to solve this problem.

For now, i am using https://github.com/NateScarlet/gotmpl to generate repeative code. String path can be converted from field name using lowerFirst, and field name has build time check.

@frederikhors
Copy link
Collaborator

frederikhors commented Dec 9, 2020

@NateScarlet I'm taking advantage of your patience again. And I thank you in advance. ❤️

I'm trying your code and what I'm using right now is only this:

type PlayerInput struct {
  Username *string
  GamingDetail *GamingDetail
}

type GamingDetail struct {
  Towns []TownInput
  Phones []PhoneInput
}

type TownInput struct {
  Name   *string
  People *string
}

type PhoneInput struct {
  Name  *string
  Phone *string
}

func (r *mutationResolver) PlayerUpdate(ctx context.Context, id int, input PlayerInput) (*Player, error) {

  var finalPlayer *Player

  DB.GetPlayer(finalPlayer)

  inputArgs := gqlgen.Arguments(ctx).Field("input")

  if inputArgs.Field("username").IsNull() {
    print("username is null")
  } else if input.Username == nil {
    print("username is undefined")
    finalPlayer.Username = input.Username
  } else {
    print(input.Username)
    finalPlayer.Username = input.Username
  }

  if inputArgs.Field("gamingDetail").IsNull() {
    print("gamingDetail is null")
  } else if input.GamingDetail == nil {
    print("gamingDetail is undefined")
    finalPlayer.GamingDetail = input.GamingDetail
  } else {
    if inputArgs.Field("gamingDetail").Field("towns").IsNull() {
      print("towns is null")
    } else if input.GamingDetail.Towns == nil {
      print("towns is undefined")
      finalPlayer.GamingDetail.Towns = input.GamingDetail.Towns
    } else {
      print(input.GamingDetail.Towns)
      finalPlayer.GamingDetail.Towns = input.GamingDetail.Towns
    }
  }

  r.Controllers.Player.Update(id, finalPlayer)

  return finalPlayer, nil
}

and it works.

  1. But I can't understand why you're using this:

    for index, d := range input.Data {
      dataArgs = inputArgs.Field("data").Index(index)
      ...
    }

    What is Index() for?

  2. Is there a way to handle GamingDetail on his own? I mean I'm using it many times in other structs and mutations too. How can I handle it separately?

Thank you again.

@NateScarlet
Copy link

NateScarlet commented Dec 9, 2020

@frederikhors

  1. Index used because data is a array

  2. You can attach selected arguments to context, then pass modified context to other function.

@frederikhors
Copy link
Collaborator

@NateScarlet, what do you think about this simpler code from #505 (comment)?

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
}

@NateScarlet
Copy link

@NateScarlet, what do you think about this simpler code from #505 (comment)?

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
}

I think this implementation is not support literal variable inside query gql, or variable names that not same as argument name defined in schema.

@frederikhors
Copy link
Collaborator

@NateScarlet I was looking for a solution that uses nullable types, as you also suggested, I came across this thread where @jszwedko says they don't solve the problem anyway.

It's true? What do you think about it?

@NateScarlet
Copy link

@frederikhors null type need gqlgen support, you can't just write a custom null type as model. Because gqlgen not call unmarshall when input value is null.

@frederikhors frederikhors converted this issue into discussion #1845 Jan 25, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

No branches or pull requests

7 participants