Skip to content
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

None's are converted to null, which Postgres doesn't like #26

Open
bkonkle opened this issue Feb 28, 2018 · 18 comments · May be fixed by #64
Open

None's are converted to null, which Postgres doesn't like #26

bkonkle opened this issue Feb 28, 2018 · 18 comments · May be fixed by #64
Labels
api-breaking-change This feature/bug requires a breaking API change and a new major version enhancement

Comments

@bkonkle
Copy link

bkonkle commented Feb 28, 2018

I'm using postgraphile on the backend to speed up data development.

I'm creating a new object, and the type in the module generated from the %graphql expression results in option(Js.Json.t) for my id. I pass None for the id, and it's sent across in the graphql request in the variables as "id": null.

Postgraphile sees that "id": null and tries to set the id field in the database to null, which Postgres understandably complains about: null value in column "id" violates not-null constraint

Here's my module:

module CreateEvent = [%graphql
  {|
    mutation CreateEvent($input: CreateEventInput!) {
      createEvent(input: $input) {
        event {
          id
          createdAt
          updatedAt
          name
          date
          eventType
          location
          description
          owner: userByOwner {
            id
            name
            headline
          }
        }
      }
    }
  |}
];

And the resulting signature of the make function's arguments:

(
  ~input: Js.t(
    {
      ..
      clientMutationId : option(string),
      event : Js.t(
        {
          ..
          createdAt : option(Js.Json.t),
          date : Js.Json.t,
          description : option(string),
          eventType : [< `DEFAULT | `MEETUP ],
          id : option(Js.Json.t),
          location : option(string),
          name : string,
          owner : option(Js.Json.t),
          updatedAt : option(Js.Json.t)
        }
      )
    }
  ),
  unit
)

If I pass None for any of those optional values, they're sent across to the server as null. The server thinks I'm intentionally nullifying those fields. How can I work around?

Thanks for a great project!

@bkonkle
Copy link
Author

bkonkle commented Feb 28, 2018

Demo project here, with both sides of the interaction: https://github.com/ecliptic/reason-events

@mhallin
Copy link
Owner

mhallin commented Mar 1, 2018

Hello!

What would be the correct behavior for graphql_ppx here? To me, the problem seems to be the fact that the fields aren't really optional if you can't set them to null.

@bkonkle
Copy link
Author

bkonkle commented Mar 1, 2018

@mhallin It's the id, createdAt, and updatedAt fields - fields that are automatically set by Postgres and don't need to be provided initially.

@bkonkle
Copy link
Author

bkonkle commented Mar 1, 2018

I misspoke above - passing None for description and owner are fine because they are nullable.

@bkonkle
Copy link
Author

bkonkle commented Mar 1, 2018

screen shot 2018-03-01 at 3 25 59 am

^ My database table structure

@mhallin
Copy link
Owner

mhallin commented Mar 1, 2018

Hmm, I see. So how would you say "use the default value for this column" - would you just omit the argument from the object?

Conceptually, omitting an argument optional argument and passing an explicit null should be treated the same by a server. There's some discussion on this here: graphql/graphql-js#133. Unfortunately, since JavaScript has both undefined and null, some JS implementations seem to make a distinction between the two.

I don't think this can be resolved from graphql_ppx's standpoint unfortunately. The option type only has two states, while a three-state enum would be required to represent "absent", "null", and "has a value".

@bkonkle
Copy link
Author

bkonkle commented Mar 1, 2018

Okay, this definitely makes sense. I can probably write a quick postgraphile plugin to strip the offending fields from create mutations (and yes, omitting them from the object seems to be the solution).

Looking through some bug reports elsewhere, it's also possible that this is related to my usage of uuid's for the primary key. I'm going to give it another go using classic serial ids and report back.

@bkonkle
Copy link
Author

bkonkle commented Mar 1, 2018

No luck, sadly. Classic integer-based serial id's suffer from the same issue. I'll close this ticket and pursue as a server plugin.

Thanks!

@bkonkle
Copy link
Author

bkonkle commented Mar 1, 2018

@mhallin I got some context from the postgraphile maintainer, and I think he has the right perspective:

To the best of my knowledge PostGraphile's behaviour is correct in this regard. Besides changing the behaviour would be a breaking change and it would violate the Principle of Least Astonishment given it maps a PostgreSQL database which works in the same way (omit the value to use the default, specify NULL if you want to override the default with an explicit null value) and changing it would reduce the expressiveness and functionality of the system. Ultimately passing {"a": 1"} vs {"a": 1, "b": null} to createEvent express different things - the latter is explicitly saying that b should be set to null, whereas the former has no information on b so it should be left as its default value.

I can totally relate to your issue though! I believe the issue is that your mapper is dealing with the input value as if it were a concrete type with fixed keys and values. Really it's a dictionary with a whitelist of keys and types those keys can be, but where every non-nullable key is optional (the entries in the dictionary itself are optional - not just the values).

It sounds like we need to use something other than Option to represent nullable values that have a default, because there are three possible intents: providing a value, providing a null value, and not providing a value.

What do you think?

@bkonkle bkonkle reopened this Mar 1, 2018
@Neitsch
Copy link
Contributor

Neitsch commented Mar 1, 2018

While I see the issue in this case, it appears to be more of a weakness in the ReasonML spec, since it does not allow for the concept of absent.

For your case, why does CreateEventInput have an ID field in the first place? It looks like it will always be auto-generated.

@wokalski
Copy link
Contributor

wokalski commented Mar 1, 2018

@Neitsch that's not right.

  • Some(None) represents a nullable null value
  • Some(Some(value)) represents a non-null value
  • None represents nothing

@Neitsch
Copy link
Contributor

Neitsch commented Mar 1, 2018

My mistake, thank you for the correction!!

@mhallin
Copy link
Owner

mhallin commented Mar 1, 2018

Alright, so one workaround right now would be to break up the input object in the query, like

mutation ($name: String!) {
  createEvent(input: { event: { name: $name } }) {
    # ...
  }
}

There will be a lot of variables unfortunately, but it should work right now.

As for the larger point, it seems the GraphQL spec has been amended with a distinction of null and absent values. Is this only for input object fields or for all arguments? If it's only for input object fields we could probably use doubly-nested optionals like @wokalski said, but I can't really discern this from the specification.

@wokalski
Copy link
Contributor

wokalski commented Mar 2, 2018

@mhallin why is it impossible to implement for all arguments?

@alexeygolev
Copy link

I like @wokalski solution. It makes sense for partial updates as well. At the moment, unless you use @mhallin solution of passing all values separately you can't really distinguish two scenarios — updating a field to null and not touching a field by omitting it from the input object

@mhallin mhallin added enhancement api-breaking-change This feature/bug requires a breaking API change and a new major version labels Jun 26, 2018
@bihnkim
Copy link

bihnkim commented Jul 10, 2018

@mhallin After some back and forth, I think your idea of breaking up the input object in the query makes the most sense and is actually the most eloquent. The parameters declared in the first line of the query is the one responsible for defining the shape of the variables, rather than the input object. This gives fine-grain control and definition to our intensions, and though it may lead to more typing I think it's for the better and likely to cause less confusion than double-nesting optionals.

@tmattio
Copy link

tmattio commented Aug 18, 2018

I am using Absinthe for my backend and I ran into the same issue.

I have been using Elm for the frontend before ReasonML and the reference library (GraphqElm) implements the field with Absent | None | Maybe(v). @mhallin I am not sure I understand why it is not practical in the case of graphql_ppx?

@baransu baransu linked a pull request Dec 9, 2018 that will close this issue
@baransu
Copy link
Contributor

baransu commented Oct 5, 2019

I've ported #64 into baransu/graphql_ppx_re. If you need such functionality you can try the latest version :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-breaking-change This feature/bug requires a breaking API change and a new major version enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants