Skip to content
This repository was archived by the owner on Sep 3, 2021. It is now read-only.

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

Open
calendardays opened this issue Mar 21, 2019 · 3 comments

Comments

@calendardays
Copy link

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

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 calendardays mentioned this issue Apr 16, 2019
@calendardays
Copy link
Author

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

#608

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants