graphql

graphql copied to clipboard

Describe the bug When a node has multiple properties using a relationship with the same type, the graphql library gets confused and tries to add all nodes to the same (first?) property.

Type definitions

This is the minimal example I could produce to trigger the error

const typeDefs = gql`
  type OpeningHours {
    monday: OpeningHoursInterval @relationship(type: "CONFIG", direction: OUT)
    tuesday: OpeningHoursInterval @relationship(type: "CONFIG", direction: OUT)
    wednesday: OpeningHoursInterval
      @relationship(type: "CONFIG", direction: OUT)
    thursday: OpeningHoursInterval @relationship(type: "CONFIG", direction: OUT)
    friday: OpeningHoursInterval @relationship(type: "CONFIG", direction: OUT)
    saturday: OpeningHoursInterval @relationship(type: "CONFIG", direction: OUT)
    sunday: OpeningHoursInterval @relationship(type: "CONFIG", direction: OUT)
  }

  type OpeningHoursInterval {
    """
    Milliseconds after midnight when this cinema opens
    """
    from: Int!
    """
    Milliseconds after midnight when this cinema closes
    """
    to: Int!
  }
`;

The mutation:

mutation createOpeningHours($input: [OpeningHoursCreateInput!]!) {
  createOpeningHours(input: $input) {
    info {
      bookmark
      nodesCreated
      relationshipsCreated
    }

    openingHours {
      monday {
        from
        to
      }
      tuesday {
        from
        to
      }
      wednesday {
        from
        to
      }
      thursday {
        from
        to
      }
      friday {
        from
        to
      }
      saturday {
        from
        to
      }
      sunday {
        from
        to
      }
    }
  }
}

with variables:

{
   "input":{
      "monday":{
         "create":{
            "node":{
               "from":41,
               "to":2
            }
         }
      },
      "tuesday":{
         "create":{
            "node":{
               "from":36000000,
               "to":79200000
            }
         }
      },
      "wednesday":{
         "create":{
            "node":{
               "from":36000000,
               "to":85500000
            }
         }
      },
      "thursday":{
         "create":{
            "node":{
               "from":44100000,
               "to":89100000
            }
         }
      },
      "friday":{
         "create":{
            "node":{
               "from":44100000,
               "to":90000000
            }
         }
      },
      "saturday":{
         "create":{
            "node":{
               "from":39600000,
               "to":89100000
            }
         }
      },
      "sunday":{
         "create":{
            "node":{
               "from":40500000,
               "to":86400000
            }
         }
      }
   }
}

To Reproduce Steps to reproduce the behavior:

1. Run a server with the type definitions from above
2. Execute the mutation with the variables provided
3. See error

{
  "errors": [
    {
      "message": "OpeningHours.monday must be less than or equal to one",
      "locations": [
        {
          "line": 6,
          "column": 3
        }
      ],
      "path": [
        "createOpeningHours"
      ],
      "extensions": {
        "code": "INTERNAL_SERVER_ERROR",
        "exception": {
          "stacktrace": [
            "Neo4jGraphQLRelationshipValidationError: OpeningHours.monday must be less than or equal to one",
            "    at execute (/Users/trond/projects/tenants/api/node_modules/@neo4j/graphql/dist/utils/execute.js:98:23)",
            "    at processTicksAndRejections (node:internal/process/task_queues:96:5)",
            "    at async resolve (/Users/trond/projects/tenants/api/node_modules/@neo4j/graphql/dist/schema/resolvers/create.js:33:31)"
          ]
        }
      }
    }
  ],
  "data": null
}

Expected behavior Mutation to work and relationships added

System:

• OS: macOS
• Version: @neo4j/[email protected] with neo4j-4.4.5-enterprise
• Node.js version: 16.3.0

Additional context If the relationships all have unique types, this error goes away and behaves as expected

Many thanks for raising this bug report @megatrond. :bug: We will now attempt to reproduce the bug based on the steps you have provided.

Please ensure that you've provided the necessary information for a minimal reproduction, including but not limited to:

• Type definitions
• Resolvers
• Query and/or Mutation (or multiple) needed to reproduce

If you have a support agreement with Neo4j, please link this GitHub issue to a new or existing Zendesk ticket.

Thanks again! :pray:

Hey @megatrond! We've had a chat about this in the team. Whilst we agree that this behaviour is quite unexpected from the GraphQL point of view, it makes quite a lot of sense when you think about the underlying data model. Let me dive into that a little.

Using arrows.app I have drawn out what you currently have:

You can likely spot the issue - although in the GraphQL type definitions you have the field name to indicate the day of the week, when you map this out into the underlying database, all context of the day of the week is lost, so you just have one OpeningHours node connected to 7 random OpeningHoursInterval nodes. When querying and mutating the data, although GraphQL has a good idea of what it's doing, as soon as it hits the database then it essentially loses all context and we start experiencing behaviour like you have seen.

The solution here in my opinion is to model the opening hours as the one-to-many relationship that it is. This could be done in a couple of ways, for example adding a dayOfWeek field to the OpeningHoursInterval type:

type OpeningHours {
  days: [OpeningHoursInterval!]! @relationship(type: "CONFIG", direction: OUT)
}

type OpeningHoursInterval {
  dayOfWeek: String!
  """
  Milliseconds after midnight when this cinema opens
  """
  from: Int!
  """
  Milliseconds after midnight when this cinema closes
  """
  to: Int!
}

Potentially an intermediate type fits the model better:

type OpeningHours {
  days: [OpeningDay!]! @relationship(type: "HAS_DAY_OF_WEEK", direction: OUT)
}

type OpeningDay {
  name: String!
  interval: OpeningHoursInterval! @relationship(type: "HAS_INTERVAL", direction: OUT)
}

type OpeningHoursInterval {
  """
  Milliseconds after midnight when this cinema opens
  """
  from: Int!
  """
  Milliseconds after midnight when this cinema closes
  """
  to: Int!
}

However you do it, you can use something like the @cypher directive to add nice helper fields for each day of the week like you had before:

type OpeningHours {
  days: [OpeningDay!]! @relationship(type: "HAS_DAY_OF_WEEK", direction: OUT)
  monday: OpeningHoursInterval! @cypher(statement: "MATCH (this)-[:HAS_DAY_OF_WEEK]->(day:OpeningDay)->[:HAS_INTERVAL]->(interval:OpeningHoursInterval) WHERE day.name = "monday" RETURN interval")
  tuesday: OpeningHoursInterval! @cypher(statement: "MATCH (this)-[:HAS_DAY_OF_WEEK]->(day:OpeningDay)->[:HAS_INTERVAL]->(interval:OpeningHoursInterval) WHERE day.name = "tuesday" RETURN interval")
  wednesday: OpeningHoursInterval! @cypher(statement: "MATCH (this)-[:HAS_DAY_OF_WEEK]->(day:OpeningDay)->[:HAS_INTERVAL]->(interval:OpeningHoursInterval) WHERE day.name = "wednesday" RETURN interval")
  thursday: OpeningHoursInterval! @cypher(statement: "MATCH (this)-[:HAS_DAY_OF_WEEK]->(day:OpeningDay)->[:HAS_INTERVAL]->(interval:OpeningHoursInterval) WHERE day.name = "thursday" RETURN interval")
  friday: OpeningHoursInterval! @cypher(statement: "MATCH (this)-[:HAS_DAY_OF_WEEK]->(day:OpeningDay)->[:HAS_INTERVAL]->(interval:OpeningHoursInterval) WHERE day.name = "friday" RETURN interval")
  saturday: OpeningHoursInterval! @cypher(statement: "MATCH (this)-[:HAS_DAY_OF_WEEK]->(day:OpeningDay)->[:HAS_INTERVAL]->(interval:OpeningHoursInterval) WHERE day.name = "saturday" RETURN interval")
  sunday: OpeningHoursInterval! @cypher(statement: "MATCH (this)-[:HAS_DAY_OF_WEEK]->(day:OpeningDay)->[:HAS_INTERVAL]->(interval:OpeningHoursInterval) WHERE day.name = "sunday" RETURN interval")
}

type OpeningDay {
  name: String!
  interval: OpeningHoursInterval! @relationship(type: "HAS_INTERVAL", direction: OUT)
}

type OpeningHoursInterval {
  """
  Milliseconds after midnight when this cinema opens
  """
  from: Int!
  """
  Milliseconds after midnight when this cinema closes
  """
  to: Int!
}

Note that this will only work for querying, but could be a helpful way of reading the information in the same way as you're used to. Also appreciate that this is very verbose - I think there is some improvements we could make to facilitate tasks like this.

From our end, as strange as it sounds, our most sensible course of action is to throw an error when we encounter a type definition like you originally posted, with a link to some appropriate graph modelling guidance in the error message.

Apologies if this isn't the answer you were hoping for, but I hope it makes sense? Do reach out if you need further guidance!

Yeah that makes sense when I see it from the neo4j db side, I agree, but I also agree the behaviour is confusing.

The problem I have with your suggested remodelling though is that using that, there's no real way (except through writing validation code) to make sure that there is exactly one OpeningHoursInterval per day, and that there is one for every day of the week.

I guess it wouldn't make any difference from the graphql side if we rename the relationship types to OPENINGHOURS_MONDAY, OPENINGHOURS_TUESDAY, and so on, and that would guarantee that the data is valid as well.

From our end, as strange as it sounds, our most sensible course of action is to throw an error when we encounter a type definition like you originally posted, with a link to some appropriate graph modelling guidance in the error message.

This sounds like a great option, with your explanation above! I'll leave a link to a post about Elm's error messages here for inspiration! https://elm-lang.org/news/compiler-errors-for-humans

The problem I have with your suggested remodelling though is that using that, there's no real way (except through writing validation code) to make sure that there is exactly one OpeningHoursInterval per day, and that there is one for every day of the week.

For sure, totally get that. It's on our roadmap to add in more advanced relationship validation - for instance by saying the length of days can be no less than and no greater than 7.

Thanks for the post from Elm, an interesting read! I'll leave this issue open as a placeholder for adding in this new error behaviour.

In the upcoming version 4.0.0, we are taking a much more opinionated approach and always throwing an error in this scenario. 🙂