neo4j-graphql / neo4j-graphql-js

NOTE: This project is no longer actively maintained. Please consider using the official Neo4j GraphQL Library (linked in README).
Other
608 stars 148 forks source link

Relation types in augmented schema: _id bug, update mutation feature request #220

Open calendardays opened 5 years ago

calendardays commented 5 years ago

This might be two issues, but I wanted to bring them up together with a single schema as a reference point for examples.

Minimal schema:

type Journal {
    _id: ID!
    name: String! 
    publisher: String
    Accepts: [Accepts]
}

type Article {
    _id: ID!
    title: String! 
    author: String
}

type Accepts @relation(name: "ACCEPTS") {
    _id: ID!
    from: Journal
    to: Article
    volume: String
    issue: String
    firstPageNumber: Int
}

With "neo4j-graphql-js": "^2.4.2", I use makeAugmentedSchema and set up an Apollo Server. It launches fine, and I can start issuing queries in the Playground.

Bug:

There is a bug if I attempt to query the _id field of an Accepts object.

This works:

{
  Journal {
    _id
    name
    publisher
    Accepts {
      #_id
      Article {
        _id
        title
        author
      }
      volume
      issue
      firstPageNumber
    }
  }

But if I remove the #, then I get this error:

{
  "errors": [
    {
      "message": "Variable `journal_Accepts` not defined (line 1, column 199 (offset: 198))\r\n\"MATCH (`journal`:`Journal` ) RETURN `journal` {_id: ID(`journal`), .name , .publisher ,Accepts: [(`journal`)-[`journal_Accepts_relation`:`ACCEPTS`]->(:`Article`) | journal_Accepts_relation {_id: ID(`journal_Accepts`),Article: head([(:`Journal`)-[`journal_Accepts_relation`]->(`journal_Accepts_Article`:`Article`) | journal_Accepts_Article {_id: ID(`journal_Accepts_Article`), .title , .author }]) , .volume , .issue , .firstPageNumber }] } AS `journal` SKIP $offset\"\r\n                                                                                                                                                                                                       ^",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "Journal"
      ],
      "extensions": {
        "code": "INTERNAL_SERVER_ERROR",
        "exception": {
          "code": "Neo.ClientError.Statement.SyntaxError",
          "name": "Neo4jError",
          "stacktrace": [
            "Neo4jError: Variable `journal_Accepts` not defined (line 1, column 199 (offset: 198))\r",
            "\"MATCH (`journal`:`Journal` ) RETURN `journal` {_id: ID(`journal`), .name , .publisher ,Accepts: [(`journal`)-[`journal_Accepts_relation`:`ACCEPTS`]->(:`Article`) | journal_Accepts_relation {_id: ID(`journal_Accepts`),Article: head([(:`Journal`)-[`journal_Accepts_relation`]->(`journal_Accepts_Article`:`Article`) | journal_Accepts_Article {_id: ID(`journal_Accepts_Article`), .title , .author }]) , .volume , .issue , .firstPageNumber }] } AS `journal` SKIP $offset\"\r",
            "                                                                                                                                                                                                       ^",
            "",
            "    at captureStacktrace (C:\\path\\to\\basedir\\node_modules\\neo4j-driver\\lib\\v1\\result.js:200:15)",
            "    at new Result (C:\\path\\to\\basedir\\node_modules\\neo4j-driver\\lib\\v1\\result.js:73:19)",
            "    at _newRunResult (C:\\path\\to\\basedir\\node_modules\\neo4j-driver\\lib\\v1\\transaction.js:344:10)",
            "    at Object.run (C:\\path\\to\\basedir\\node_modules\\neo4j-driver\\lib\\v1\\transaction.js:253:14)",
            "    at Transaction.run (C:\\path\\to\\basedir\\node_modules\\neo4j-driver\\lib\\v1\\transaction.js:122:26)",
            "    at C:\\path\\to\\basedir\\node_modules\\neo4j-graphql-js\\dist\\index.js:78:25",
            "    at TransactionExecutor._safeExecuteTransactionWork (C:\\path\\to\\basedir\\node_modules\\neo4j-driver\\lib\\v1\\internal\\transaction-executor.js:136:22)",
            "    at TransactionExecutor._executeTransactionInsidePromise (C:\\path\\to\\basedir\\node_modules\\neo4j-driver\\lib\\v1\\internal\\transaction-executor.js:124:32)",
            "    at C:\\path\\to\\basedir\\node_modules\\neo4j-driver\\lib\\v1\\internal\\transaction-executor.js:67:15",
            "    at new Promise (<anonymous>)"
          ]
        }
      }
    }
  ],
  "data": {
    "Journal": null
  }
}

Of course, if I replace ID(journal_Accepts) with ID(journal_Accepts_relation), this Cypher query runs fine in my Neo4j Browser.

Feature request:

The auto-generated mutations include Add/Update/Remove for both Journal and Article. Rather than explicit Add/Update/Remove for Accepts, we have Add/Remove for JournalAccepts (which is a design choice I can understand) but no corresponding Update mutation. Maybe in this example it's silly to Update a field like firstPageNumber, but I was surprised anyway that Update mutations weren't generated for relation types in general.

calendardays commented 5 years ago

On the feature request:

I realized that an attempt to UpdateRelationType raises a larger issue that might not be in the near-future plans.

The arguments of CreateJournal are just all the fields of Journal, with the same nullabilities. For UpdateJournal, though, the first field listed in the schema for Journal is a non-nullable argument used to match existing nodes, and the rest of Journal's fields are nullable arguments used to update the matched nodes. (In my example schema, this is the same, but only because there is exactly one non-nullable field and it's listed first, which is a coincidence.) For RemoveJournal, the only argument is Journal's first listed field, used again for matching.

For RemoveJournalAccepts, matching is done on the basis of the from: Journal and to: Article arguments, with no data argument passed at all. If one Journal had multiple ACCEPTS relations with a single Article in my database, with each relation having different property values, RemoveJournalAccepts would delete all of those relations. (Again, my schema isn't the very best example for showing this.)

If there were an auto-generated UpdateJournalAccepts mutation, it would need a data argument. It could then use the from and to arguments to perform matching the same way the RemoveJournalAccepts mutation does, and would end up updating all relations between a given pair of nodes to have identical, new property values. Even getting to that point would require a decision about whether the data for UpdateJournalAccepts should be of the same input type as the data for AddJournalAccepts (_AcceptsInput) or should be a separately generated input type with different nullabilities for its input fields. And once you did get to that point, it would then be tempting to ask that both UpdateJournalAccepts and RemoveJournalAccepts should be able to match based on nodes AND property values so that one relation out of many between two nodes could be targeted.

So I can see how that's a big can of worms and not necessarily a priority.

Hopefully the bug mentioned in the original post is a simpler fix.

calendardays commented 5 years ago

Just to connect dots, #229 contains work related to the _id bug described here, and #227 is a broad attempt to implement the feature requested here (as well as implementing a solution to the can of worms described in my comment above).

michaeldgraham commented 3 years ago

https://github.com/neo4j-graphql/neo4j-graphql-js/issues/608