Create Mutation

[danstarns]

This PR covers this card. It's tested and fully functional and here I will explain a few points and gotchas.

Conditional Cypher

Please see conditional-cypher-execution

The nature of connect means that we are matching nodes that could or could not exist. Say we have a product and we want to connect it to a photo. We know that the product will exist, as we are creating it, but the photo is an unknown. One could go ahead and write the following;

// GraphQL
mutation {
  createProducts(
    input: [
      { name: "Beer", photos: { connect: [{ where: { id: "321" } }] } }
    ]
  ) {
    id
  }
}

// Cypher
CREATE (product:Product {name: "Beer"})
WITH product
MATCH (photo:Photo)
WHERE photo.id = "321" // break
MERGE (product)-[:HAS_PHOTO]->(photo)
RETURN product

But quickly this becomes a problem when photo is not found... The query will 'break' and not return the created product or execute any lines under the "not found" photo. To combat this unwanted behavior one would think to use sub subquerys such as;

CREATE (product:Product {name: "Beer"})
WITH product
CALL {
  OPTIONAL MATCH (photo:Photo)
  WHERE photo.id = "321" 
  MERGE (product)-[:HAS_PHOTO]->(photo)
}
RETURN product

Unfortunately, the above cypher will not work and there is currently an investigation into the discussed and reported bug here. Due to the issue discussed we need to fall back to using the FOREACH technique explained in conditional-cypher-execution;

CREATE (product:Product {name: "Beer"})
WITH product
OPTIONAL MATCH (photo:Photo)
WHERE photo.id = "321" 
FOREACH(_ IN CASE photo WHEN NULL THEN [] ELSE [1] END |
    MERGE (product)-[:HAS_PHOTO]->(photo)
)
RETURN product

It's worth pointing out that FOREACH will be removed at some point. We should keep its deprecation in mind & continue liaising with the cypher team as to when the issue will be fixed.

Projections

One can create many product with one mutation;

mutation {
  createProducts(
    input: [
      { name: "Beer" },
      { name: "Cake" }
    ]
  ) { // selection set start
    name
  } // selsection set end
}

Although the ability to create many product users only has the ability to specify one projection. In this PR, we have some 'magic' to reuse the same projection for each created product here. This logic saves us from creating O(n) projections and params.

Usefull Tests

All tests are useful although you will find the following; here & here extremely handy in the context of this PR.

Cypher Generation

You may have noticed the 'ugly' cypher generation. Efforts have been made to keep this as readable as possible however, it's unrealistic to generate pretty patterns with cypher. One who is creating a node and creating relationships may write something like;

CREATE (product:Product)-[:HAS_PHOTO]->(photo:Photo)-[:HAS_COLOR]->(color:Color)

Seeming we expose the ability to generate complex cypher this PR sticks to using a more 'procedural' style such as;

CREATE (product:Product)
MATCH (photo:Photo)
MERGE (product)-[:HAS_PHOTO]->(photo)
MATCH (color:Color)
MERGE (photo)-[:HAS_COLOR]->(color)

The usage of procedural cypher becomes apparent when one is connecting large subgraphs such as the one here. It means the code is simpler as we only ever 'concatenate' a connection between 2 nodes plus it leaves room for further operations such as; update & disconnect.

[oskarhane]

First of all, thank you for a superb PR description.
It's very useful to read about your reasoning and the path you got to to get to this solution.

As for the changes, I think the separation into separate files are useful and makes it easy to read, so 👍 on that!
The code looks good, I just had some minor comment.

And love the tests, thanks for writing good tests.
I found a bug when using float numbers when using query variables (unrelated to this PR), but something to think about in the future is to include most datatypes in each higher level tests (tck + integration).

One thing I wonder though is if the non-transactional behavior is expected.
The case when you want to create X and connect it Y and Y doesn't exist. In the database world that might live in a transaction that will be rolled back when it fails.

I think we should discuss the last point with PM/Will before I approve this PR.

[oskarhane]

wdyt about using arrays to store the strings and joining them with line-break at the end (as you do in other areas of the code) instead?
I find the \nString everywhere a bit dirty.

[danstarns]

Yeah, I agree, in most use cases an array is fine. This was me being inconsistent & a good spot. Ill tackle this with the next few commits.

[oskarhane]

I love these tests. Very useful and easy to read!

[danstarns]

One thing I wonder though is if the non-transactional behavior is expected.
The case when you want to create X and connect it Y and Y doesn't exist. In the database world that might live in a transaction that will be rolled back when it fails.

I think we should discuss the last point with PM/Will before I approve this PR.

@oskarhane Thanks for your feedback. Interesting thoughts on the transaction & I guess your specific point highlighted here touches on the usage of FOREACH ?

Currently, as it stands, if one was to "Create a product & connect to a not found photo";

mutation {
    createProduct(input: [
        {
            name: "Pringles",
            connect: {
                photo: [{ where { name: "not found" }}]
            }
        }
    ])
}

this ^ mutation would pass. Just to clarify your question here... are you asking if either this should or should not succeed?

[oskarhane]

this ^ mutation would pass. Just to clarify your question here... are you asking if either this should or should not succeed?

Yes, correct.
Should the create be "all or nothing" or "whatever connects, connects"?

Personally, I'm not sure.
Maybe it makes sense to introduce a merge in addition to the create and connect on relationship fields, or maybe the behavior in this PR is what we want.

Something to discuss internally.

[danstarns]

Should the create be "all or nothing" or "whatever connects, connects"?

Interesting point, if we went for the "all or nothing" approach we would need an explicit way of throwing an exception. Something like;

CREATE (product:Product {name: "Beer"})
WITH product
OPTIONAL MATCH (photo:Photo)
WHERE photo.id = "321" 
FOREACH(_ IN CASE photo WHEN NULL THEN [1] ELSE [] END |
    throw new Error("photo not found") 
)
RETURN product

[danstarns]

Interesting point, if we went for the "all or nothing" approach we would need an explicit way of throwing an exception.

One could use apoc.util.validate to throw an exception here.

[oskarhane]

I think this is good to go, it sounds like we want the behavior to "connect and ignore if there's nothing to connect to" rather than "all or nothing".
🤖