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

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

Open
AccsoSG opened this issue Apr 16, 2024 · 9 comments
Labels
feature request New feature or request reopened

Comments

@AccsoSG
Copy link

AccsoSG commented Apr 16, 2024

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

  • Version: @neo4j/graphql@5.3.2

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 AccsoSG added the bug report Something isn't working label Apr 16, 2024
@neo4j-team-graphql neo4j-team-graphql added this to Bug reports in Bug Triage Apr 16, 2024
@neo4j-team-graphql
Copy link
Collaborator

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

@AccsoSG AccsoSG changed the title Relationship is settable via update mutation, but settable is set to false. Relationship is settable via update mutation, but settable directive is set to false. Apr 16, 2024
@Liam-Doodson
Copy link
Contributor

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 Liam-Doodson closed this as not planned Won't fix, can't repro, duplicate, stale Apr 18, 2024
@AccsoSG
Copy link
Author

AccsoSG commented Apr 18, 2024

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!
}

@Liam-Doodson
Copy link
Contributor

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

@AccsoSG
Copy link
Author

AccsoSG commented Apr 18, 2024

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
Copy link
Author

AccsoSG commented Apr 18, 2024

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

@Liam-Doodson
Copy link
Contributor

Liam-Doodson commented Apr 18, 2024

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] }

@AccsoSG
Copy link
Author

AccsoSG commented Apr 18, 2024

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
Copy link
Author

AccsoSG commented Apr 22, 2024

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

@Liam-Doodson Liam-Doodson reopened this Apr 22, 2024
@Liam-Doodson Liam-Doodson added feature request New feature or request and removed bug report Something isn't working labels Apr 22, 2024
@Liam-Doodson Liam-Doodson removed this from Bug reports in Bug Triage Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request reopened
Projects
None yet
Development

No branches or pull requests

3 participants