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

null value in column "id" violates not-null constraint #189

Closed
bkonkle opened this issue Mar 1, 2018 · 7 comments
Closed

null value in column "id" violates not-null constraint #189

bkonkle opened this issue Mar 1, 2018 · 7 comments
Labels
Milestone

Comments

@bkonkle
Copy link

bkonkle commented Mar 1, 2018

Related to this issue discussed on the graphql_ppx project: mhallin/graphql_ppx#26

For fields with default values, NULL is sent across rather than DEFAULT or omitting the values. From the postgres log:

insert into "events"."event" (
  "id", "created_at", "updated_at", "name", "date", "event_type", "location", "description", "owner"
) values(
  NULL, NULL, NULL, $1, $2, $3, $4, $5, NULL) returning *
)

I wrote this tiny plugin to fix the issue in a very manual way:

const {dissocPath, reduce, keys} = require('ramda')

/**
 * Strip out null values from fields with defaults
 */
exports.stripNullsFromDefaultFields = (builder) => builder.hook('GraphQLObjectType:fields:field',
  (field, {pgSql: sql}, {scope: {isRootMutation, fieldName}}) => {
    const entityFields = ['id', 'createdAt', 'updatedAt']

    const fieldNameMap = {
      createUser: entityFields,
      createEvent: entityFields,
    }

    if (!isRootMutation) {
      return field
    }
    if (keys(fieldNameMap).includes(fieldName)) {
      const defaultResolver = obj => obj[fieldName]
      const {resolve: oldResolve = defaultResolver, ...rest} = field

      return {
        ...rest,
        resolve: function (root, input, context, ast) {
          const newInput = reduce(
            // Remove each field from the object
            (result, field) => dissocPath(['input', 'event', field], result),
            // Use the input as the initial accumulator
            input,
            // Remove the fields from the map above for the current field name
            fieldNameMap[fieldName]
          )

          const newAst = reduce(
            (result, field) => dissocPath(['variableValues', 'input', 'event', field], result),
            ast,
            fieldNameMap[fieldName]
          )

          return oldResolve(root, newInput, context, newAst)
        },
      }
    }
    return field
  }
)

Is there a way to solve this for all fields with default values, though?

Thanks for maintaining/expanding such an outstanding project!

@benjie
Copy link
Member

benjie commented Mar 1, 2018

If you want to use the defaults simply don't specify that you want the column value to be null - if you specify null then we take you at your word. This is the same behaviour as SQL.

e.g.

mutation CreateEvent($name: String!, date: Date!) {
  createEvent(input: {event: {name: $name, date: $date}}) {
    ...
  }
}

@benjie benjie added the question label Mar 1, 2018
@bkonkle
Copy link
Author

bkonkle commented Mar 1, 2018

@benjie According to @leebyron in this thread:

In GraphQL input variables, there is no difference between null and undefined. The value null means "lack of a value".

It sounds like your understanding of this is different? How can I work around this issue when interacting with postgraphile from OCaml?

Further detail from that thread:

We do not plan on supporting this since most environments do not have different null and undefined sentinel values - that's a specific feature of JavaScript and GraphQL is designed to be supported by many more platforms.

@bkonkle
Copy link
Author

bkonkle commented Mar 1, 2018

From the results of the discussion I linked above, It sounds like postgraphile should only be sending null to Postgres when it receives a NullValue scalar, rather than a primitive null - or, am I misunderstanding?

@benjie
Copy link
Member

benjie commented Mar 1, 2018

A NullValue scalar in GraphQL specification speak is a primitive null in JSON (which is what we communicate with); read this comment regarding omitting the key from the dictionary/map that represents the input value.

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).

I hope you can find an acceptable solution - if you do I'd really appreciate it if you could share it here.

Good luck!

@bkonkle
Copy link
Author

bkonkle commented Mar 1, 2018

Thank you so much for the detail! This is very helpful. I'll go back to the author of graphql_ppx, the library generating the input mappers.

@leebyron
Copy link

leebyron commented Mar 1, 2018

Just to follow up - it’s correct that graphql should interpret an explicit null differently from an implicit missing value. That thread linked above walks through the decision making process from a couple years ago. We actually amended the spec to account for this as a result.

@benjie
Copy link
Member

benjie commented Mar 7, 2018

[semi-automated message] To keep things manageable I'm going to close this issue as I think it's solved; but if not or you require further help please re-open it.

@benjie benjie closed this as completed Mar 7, 2018
@benjie benjie added this to the 4.0 milestone Aug 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants