Missing __typename in fragment str

[mat-sop]

Reported in #253

When the result field's type is an abstract type - union or interface, we add __typename to the query string and corresponding generated model. When this happens as part of the fragment we only add to the model, not to the string - because of this the generated client can't properly create an instance of the model.

Example schema to reproduce the issue:

type Query {
    aaa: TypeA!
}

type TypeA {
    valueA: String!
    fieldB: InterfaceB!
}

interface InterfaceB {
    valueB: String!
}

type TypeB implements InterfaceB{
    valueB: String!
    typeSpecific: String!
}

Example query:

query getA {
  aaa {
    ...FragmentA
  }
}

fragment FragmentA on TypeA {
  valueA
  fieldB {
    valueB
    ... on TypeB {
      typeSpecific
    }
  }
}

[bombsimon]

I started to look into this but this was a bit tricker than I expected, mostly because the information about fragments isn't shared between generators. When the PackageGenerator is created the fragments are both passed to the FragmentGenerator and the PackageGenerator itself.

With the fix in #224 these fragments got passed further to the field parsing but the field and schema state is in the local context for the result fields so I don't really se an obvious way to leverage this when generating the fragment string. And further mutating the fragment definitions dictionary might have unintentional side effects and becomes very order dependent.

I think one approach to not spread this into separate generators would be to somehow pre-process this kind of data modifying the fragment definitions already when creating the PackageGenerator. Right now we don't have any context around different fields but as a test I tried to just recursively ensure __typename is selected on every selections that's not spreads. This would ensure the fragment string generated in each client method select __typename as well and that every result class will have the __typename field.

Have you had the time to look into what would be required to resolve this issue? Is there a downside to always selecting __typename or would that be a viable solution forward? Any pointers on how I could add __typename only to the abstract fields (without adding it twice)?

Also I know that sometimes having to answer people on questions like this just takes more time and energy so feel free to not elaborate on any reply here. I mostly just wanted to try to contribute here since this is blocking us but I don't want to introduce a big refactor if you had something else in mind.

Diff to unconditionally add __typename to all types

diff --git a/ariadne_codegen/client_generators/package.py b/ariadne_codegen/client_generators/package.py
index e17ec5e..137c28a 100644
--- a/ariadne_codegen/client_generators/package.py
+++ b/ariadne_codegen/client_generators/package.py
@@ -2,7 +2,14 @@ import ast
 from pathlib import Path
 from typing import Dict, List, Optional, Set
 
-from graphql import FragmentDefinitionNode, GraphQLSchema, OperationDefinitionNode
+from graphql import (
+    FieldNode,
+    FragmentDefinitionNode,
+    GraphQLSchema,
+    NameNode,
+    OperationDefinitionNode,
+    SelectionSetNode,
+)
 
 from ..codegen import generate_import_from
 from ..exceptions import ParsingError
@@ -22,6 +29,7 @@ from .constants import (
     DEFAULT_BASE_CLIENT_PATH,
     EXCEPTIONS_FILE_PATH,
     GRAPHQL_CLIENT_EXCEPTIONS_NAMES,
+    TYPENAME_FIELD_NAME,
     UNSET_IMPORT,
     UPLOAD_CLASS_NAME,
     UPLOAD_IMPORT,
@@ -374,7 +382,13 @@ def get_package_generator(
         custom_scalars=settings.scalars,
         plugin_manager=plugin_manager,
     )
+
     fragments_definitions = {f.name.value: f for f in fragments or []}
+
+    # Ensure that we always have `__typename` for fragments to match the
+    # generated fragment classes.
+    fragments_definitions = _add_typename_fragment(fragments_definitions)
+
     fragments_generator = FragmentsGenerator(
         schema=schema,
         fragments_definitions=fragments_definitions,
@@ -417,3 +431,37 @@ def get_package_generator(
         custom_scalars=settings.scalars,
         plugin_manager=plugin_manager,
     )
+
+
+def _add_typename_fragment(
+    fragments_definitions: Optional[Dict[str, FragmentDefinitionNode]] = None
+) -> Dict[str, FragmentDefinitionNode]:
+    if fragments_definitions is None:
+        return {}
+
+    for name, fr in fragments_definitions.items():
+        fragments_definitions[name].selection_set = _ensure_typename(fr.selection_set)
+
+    return fragments_definitions
+
+
+def _ensure_typename(selection_set: SelectionSetNode) -> SelectionSetNode:
+    seen_selections = set()
+
+    for selection in selection_set.selections:
+        if not isinstance(selection, FieldNode):
+            continue
+
+        if selection.selection_set:
+            selection.selection_set = _ensure_typename(selection.selection_set)
+
+        seen_selections.add(selection.name.value)
+
+    if TYPENAME_FIELD_NAME in seen_selections:
+        return selection_set
+
+    selection_set.selections = (
+        FieldNode(name=NameNode(value=TYPENAME_FIELD_NAME)),
+    ) + selection_set.selections
+
+    return selection_set