graphql icon indicating copy to clipboard operation
graphql copied to clipboard

Relationship is settable via update mutation, but settable directive is set to false.

Open AccsoSG opened this issue 3 months ago • 9 comments

Describe the bug Setting a field with relationship is possible with an update mutation, although settable onUpdate is set to false.

Type definitions

type Identifier {
  value: String!

  identifierType: IdentifierType! 
    @relationship(
      type: "HAS_IDENTIFIER_TYPE", 
      direction: OUT, 
      nestedOperations: [CONNECT, DISCONNECT],
      aggregate: false)
    @selectable(onRead: true, onAggregate: false)
    @settable(onCreate: true, onUpdate: false)
    @filterable(byValue: true, byAggregate: false)
}

type IdentifierType {
  name: String!
}

To Reproduce Steps to reproduce the behavior:

  1. Run a server with the following code...
  2. Execute the following Mutation...
  • Create
mutation M1 {
  createIdentifierTypes(input: [{ name: "Test1" }, { name: "Test2" }]) {
    identifierTypes {
      name
    }
  }
}
mutation M2 {
  createIdentifiers(input: {
    value: "123",
    identifierType: {
      connect: { where: { node: { name: "Test1"}}}
    }
  }){
    identifiers {
      value
      identifierType {
        name
      }
    }
  }
}

image

  • Update This mutation is valid, but does not work because of the cardinality (Identifier.identifierType required exactly once):

mutation M3 {
  updateIdentifiers(
    where: { value: "123" }
    connect: { identifierType: { where: { node: { name: "Test2" } } } }
  ) {
    identifiers {
      value
      identifierType {
        name
      }
    }
  }
}

This mutation, however, works.

mutation M4 {
  updateIdentifiers(
    where: { value: "123" }
    connect: { identifierType: { where: { node: { name: "Test2" } } } },
    disconnect: { identifierType: { where: { node: { NOT: { name: "Test2"}}}}}
  ) {
    identifiers {
      value
      identifierType {
        name
      }
    }
  }
}

image

Expected behavior If the settable directive for onUpdate is set to false, the relationship should not be able to be changed. The generated schema should not offer this option at all.

System (please complete the following information):

Side question If you set onUpdate to true via the settable directive, there is also the option of setting the relationship via the path

update: { identifierType : { connect: { where: { node: { ... }}}}}

to do exactly the same thing. Are these simply two different ways of doing the same thing, or is there a difference here?

AccsoSG avatar Apr 16 '24 15:04 AccsoSG

Many thanks for raising this bug report @AccsoSG. :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:

neo4j-team-graphql avatar Apr 16 '24 15:04 neo4j-team-graphql

Hi @AccsoSG! I can see how this could be misleading due to the multiple inputs for performing the same action.

As you say, adding @settable(onUpdate: false) removes the option to update the related type using the following input update: { identifierType : { connect: { where: { node: { ... }}}}}. In other words, the field has been removed from the identifierInput input type. This is the expected behaviour for the directive and is documented here.

This prevents users of the api from updating the values of the identifierType when coming from an identifier. If you also want to remove the option to connect/disconnect the relation to an identifierType when coming from an identifier, you'd need to remove the CONNECT/DISCONNECT from the nestedOperations argument of @relationship

Liam-Doodson avatar Apr 18 '24 14:04 Liam-Doodson

Hi @Liam-Doodson Thank you for your answer.

But if I remove Connect/Disconnect, then I can no longer set this relationship in a Create-Operation, can I?

type Identifier {
  value: String!

  identifierType: IdentifierType! 
    @relationship(
      type: "HAS_IDENTIFIER_TYPE", 
      direction: OUT, 
      nestedOperations: [], #<----------------------------
      aggregate: false)
    @selectable(onRead: true, onAggregate: false)
    @settable(onCreate: true, onUpdate: false)
    @filterable(byValue: true, byAggregate: false)
}

type IdentifierType {
  name: String!
}

AccsoSG avatar Apr 18 '24 14:04 AccsoSG

Are you able to just remove the DISCONNECT nestedOperation? That way it would be possible to set the relationship but it would not be possible to change the identifierType when coming from an identifier

Liam-Doodson avatar Apr 18 '24 14:04 Liam-Doodson

Yes, that would work, but there is still the corresponding CONNECT in the generated API. This CONNECT will not work, but it will still be visible.

My expectation of the settable directive in combination with the relationship directive is different...

AccsoSG avatar Apr 18 '24 14:04 AccsoSG

So perhaps it is more of a feature request. The settable directive should really also consider this case.

AccsoSG avatar Apr 18 '24 14:04 AccsoSG

As in you want to have the connect option on a create but not on an update mutation?

If this is the case I think it would be more appropriate to add this as an option to the nestedOperation argument potentially with an interface like this: nestedOperation: { create: [CONNECT] }

Liam-Doodson avatar Apr 18 '24 15:04 Liam-Doodson

I generally mean that the settable directive implies that a field cannot be set in the update case if onUpdate=false. But the fact is that this field can be set if it is a relationship. That is not very intuitive

I would not modify the nestedOperations of the relationship directive as you have suggested. I think that is too complicated. I would simply adapt the implementation of the settable directive so that the connect/disconnect options also disappears from the API if the settable directive is set to false.

AccsoSG avatar Apr 18 '24 15:04 AccsoSG

@Liam-Doodson I would be grateful if we could turn this ticket into a feature request instead of closing it.

AccsoSG avatar Apr 22 '24 11:04 AccsoSG