Check for duplicate or conflicting operations

[AlecAivazis]

For example, the following should throw an error

mutation {
    addUser { 
        ...listA_insert
        ...listA_remove
    }
}

And the following should only result in a single operation in the artifact, not two

mutation {
    addUser { 
        ...listA_insert
        ...listA_insert
    }
}

[ledesmablt]

I think if there are duplicate operations / selections, the generate command should throw an error instead of returning only one operation in the artifact. If the developer does something like this:

mutation AddItem($input: AddItemInput!) {
  addItem(input: $input) {
    error {
      message
    }
  }
  addItem(input: $input) {  # same `addItem` but different selections
    item {
      id
      id # duplicate id
    }
    error {
      message
    }
  }
}

Then the generated types would look like this (tested it myself)

export type AddItem$result = {
    readonly addItem: {
        readonly error: {
            readonly message: string
        } | null
    },
    readonly addItem: { // would overwrite the first addItem i think
        readonly item: {
            readonly id: string
            readonly id: string  // duplicate id
        } | null,
        readonly error: {
            readonly message: string
        } | null
    }
};

I think the CLI should discourage duplicate selections. If you think this behavior makes sense then I can try putting together a PR for it.

Regarding the first concern on conflicting operations, I'm not sure if the schema has "awareness" on how the mutations are resolved. The developer can implement the delete mutation behave exactly the same as the add mutation under the hood 😆

[AlecAivazis]

Hey @ledesmablt!

Thanks for the perspective! I'm not really sure I understand the issue with duplicate fields in general. The example you provided with the typescript generator looks like a bug to me. I think we should let the user structure their queries how they want unless we can clearly identify it as a bug, which I think is the case for list operations. For example, a user couldn't delete the same entity from a list twice, that doesn't make sense. Similarly, I think its much more likely that having multiple *_insert fragments is likely a mistake.

Regarding the first concern on conflicting operations, I'm not sure if the schema has "awareness" on how the mutations are resolved. The developer can implement the delete mutation behave exactly the same as the add mutation under the hood

This comment leads me to think that we aren't talking about the same thing. By list operations I mean the generated fragments that houdini uses to embed operations in mutation response. listA_insert tells the runtime to insert that object in the listA list

[ledesmablt]

Hey @AlecAivazis , thank you for your explanation. My bad for not figuring out that you were talking about the @list directive; I thought you were talking about duplicate operations in general. In that case, your stance makes complete sense! I'll see if I can help out here as well :)

Regarding the typescript generator -- after doing some more thorough testing, I think there is a bug if there are duplicate fields. I would open this under another issue, but am not sure what to name it since it also concerns "duplicate" operations (although not regarding the @list directive)

Take for example this mutation:

mutation AddItem($input: AddItemInput!) {
  addItem(input: $input) {
    item {
      id
      id
    }
  }
  addItem(input: $input) {
    error {
      message
    }
    item {
      text
    }
  }
}

Our generated typescript result variable looks like this:

export type AddItem$result = {
    readonly addItem: {
        readonly item: {
            readonly id: string,
            readonly id: string
        } | null
    },
    readonly addItem: {
        readonly error: {
            readonly message: string
        } | null,
        readonly item: {
            readonly text: string
        } | null
    }
};

However, when we look at the graphql result for the addItem mutation, our response is:

This leads me to believe that if there are duplicate selections in a node, the graphql server considers it as valid and performs a deep merge to produce the result we see in the screenshot above. I can rework #173 to do that deep merge on the generated fields so that the generated types match the actual graphql result. What do you think?

Edit: Personally though, even though duplicate selections are considered valid by graphql I think they should be discouraged (should throw error). I find this "deep merge" behavior to be an antipattern since it's not as intuitive or organized as opposed to keeping everything under the same operation (which is how we usually write queries anyway). But then again, it's valid graphql. I'll leave the final say up to you =)

[AlecAivazis]

yes, that's correct - every server implementation i know of performs this merge before resolving the query. One reason is so that a field's resolver only runs once if it shows up multiple times (common when users start to organize their queries with fragments).

In general, I'm not a fan of imposing constraints on the devs unless it is necessary to support some feature of houdini. For example, query names are optional in GraphQL but requires in houdini so that the artifact generator can create the appropriate file and import.

All of that being said, I would like to keep supporting duplicate fields. I opened #175 to track the issue with the typescript generator. I'll give it a shot once I finish up #157.