[Codegen] Add CompiledArgumentDefinition #5797

BoD · 2024-04-10T16:40:12Z

Introduce CompiledArgumentDefinition instead of storing schema argument details (isKey, isPagination) in CompiledArgument
They are added as fields inside the Type class, with the form __fieldName__argumentName
To limit the size of the generated code, track arguments that are used by operations and output only the definition for those
- To do that, a wrapper class UsedCoordinates is introduced, to avoid manipulating maps directly.
- The used coordinated are now tracked by the CodegenTest in used-coordinates.json files
Behavior change: only the specified arguments values are set to the CompiledArgument, and not the default value from the schema. This makes the note saying The argument defaultValues are not added to the name in the Kdoc of nameWithArguments() true.

Resolves #5781

…ecated

…ledArgumentDefinition(...)

… usage in operations

…rgument usage in operations

…all its fields

netlify · 2024-04-10T16:40:30Z

✅ Deploy Preview for apollo-android-docs canceled.

Name	Link
🔨 Latest commit	`b0d2dae`
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-android-docs/deploys/6619307566413b0008609f17

.idea/codeStyles/Project.xml

libraries/apollo-api/api/apollo-api.api

libraries/apollo-compiler/src/main/kotlin/com/apollographql/apollo3/compiler/UsedCoordinates.kt

martinbonnin · 2024-04-10T19:37:37Z

libraries/apollo-compiler/src/main/kotlin/com/apollographql/apollo3/compiler/ir/IrOperations.kt

+    val parentType: String,
+    val parentField: String,


can you merge parentType and parentField (and name) in an id field like in IrModel.id that you can later use to resolve the argument definition classname for that id?

I'm trying to avoid GraphQL things in the IR. It's OK to have opaque ids but IR doesn't need to know about GraphQL names.

martinbonnin · 2024-04-10T20:07:29Z

...apollo-compiler/src/main/kotlin/com/apollographql/apollo3/compiler/ir/IrOperationsBuilder.kt

+  private fun UsedCoordinates.putAllArguments(
+      parentType: String,
+      selections: List<GQLSelection>,
+      allFragmentDefinitions: Map<String, GQLFragmentDefinition>,
+  ) {


If you end up keeping this, allFragmentDefinitions is in the context already, I think you can skip it here.

Also side note: yet another thing that would be easier to do with a proper traversal API.

...src/test/graphql/com/example/antlr_tokens/java/operationBased/used-coordinates.json.expected

martinbonnin · 2024-04-10T20:25:55Z

...apollo-compiler/src/main/kotlin/com/apollographql/apollo3/compiler/ir/IrOperationsBuilder.kt

+      // When a field with arguments is selected, its parent type is referenced in the compiled selections
+      if (first.definitionHasArguments) {
+        usedCoordinates.putType(first.parentType)


Could you add all the arguments here instead of calling UsedCoordinates.putAllArguments at the end of the process? You'll probably have to add arguments to the CollectedField but it's colocating everything.

martinbonnin · 2024-04-10T20:34:06Z

libraries/apollo-compiler/src/main/kotlin/com/apollographql/apollo3/compiler/ir/IrSchema.kt

@@ -62,6 +66,10 @@ internal data class IrObject(
    val deprecationReason: String?,
    val embeddedFields: List<String>,
    val mapProperties: List<IrMapProperty>,
+    /**
+     * Only contains fields that have arguments used in operations.


I think it's OK to skip that comment since it's the whole premise of the IR that it matches the target and not the GraphQL schema.

We could add a more general comment how these things are decided in IrOperationBuilder and link from here though.

martinbonnin · 2024-04-10T20:40:22Z

libraries/apollo-api/src/commonMain/kotlin/com/apollographql/apollo3/api/CompiledGraphQL.kt

+    /**
+     * The compile-time value of that argument.
+     *
+     * Can be the defaultValue if no argument is defined in the operation.


Didn't we go for "no need for the schema defaultValue"?

...pollo-compiler/src/main/kotlin/com/apollographql/apollo3/compiler/ir/SelectionSetsBuilder.kt

…on. Instead, store ids to resolve them and necessary info for the codegen.

martinbonnin

👍

martinbonnin · 2024-04-12T15:36:08Z

...aries/apollo-compiler/src/main/kotlin/com/apollographql/apollo3/compiler/codegen/Resolver.kt

@@ -33,6 +33,7 @@ enum class ResolverKeyKind {
  Schema,
  CustomScalarAdapters,
  Pagination,
+  ArgumentDefinition,


For later: we can use the coordinate to lookup the matching kotlin instead of adding a new kind each time.

martinbonnin · 2024-04-12T15:38:30Z

libraries/apollo-compiler/src/main/kotlin/com/apollographql/apollo3/compiler/ir/IrSchema.kt

+    fun id(type: String, field: String, argument: String) = "$type.$field.$argument"
+    fun propertyName(fieldName: String, argumentName: String) = "__${fieldName}_${argumentName}"


For later: we should come up with some rules about symbols. Are __ to escape nameclashes or a signal that it is not meant to be called directly. It's not 100% clear at the moment

BoD added 11 commits April 10, 2024 18:10

Add CompiledArgumentDefinition

d6841b8

Re-introduce CompiledArgument.name and CompiledArgument.isKey as depr…

4f91c7d

…ecated

Have field definitions and their arguments generated inside type classes

cf477f6

Add field parent types to usedFields

9b695ab

Generate top level fields in the type classes: val field__arg = Compi…

0c31cca

…ledArgumentDefinition(...)

Introduce UsedCoordinates to replace usage of Maps and track argument…

c5cf8cc

… usage in operations

Regenerate .expected

7f991af

Update CacheArgumentTest

86b27cf

fixup! Introduce UsedCoordinates to replace usage of Maps and track a…

d6257c2

…rgument usage in operations

If a type is in alwaysGenerateTypesMatching, include all the args of …

1446aa5

…all its fields

Add used types to expected files

e102483

BoD requested a review from martinbonnin as a code owner April 10, 2024 16:40

martinbonnin reviewed Apr 10, 2024

View reviewed changes

BoD added 7 commits April 12, 2024 09:40

Don't store GQL information directly in IrArgument/IrArgumentDefiniti…

9078bf6

…on. Instead, store ids to resolve them and necessary info for the codegen.

Regenerate .expected

6070b5e

Remove superfluous KDoc

1ebd0d7

Indent the used-coordinates.json.expected files

30662ff

Fix comment of CompiledArgument.value

3ececf6

Track argument usage inside merge

25c04da

Regenerate .expected

b0d2dae

martinbonnin approved these changes Apr 12, 2024

View reviewed changes

BoD merged commit fa85469 into main Apr 15, 2024
9 checks passed

BoD deleted the compiled-argument-definition branch April 15, 2024 07:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Codegen] Add CompiledArgumentDefinition #5797

[Codegen] Add CompiledArgumentDefinition #5797

BoD commented Apr 10, 2024

netlify bot commented Apr 10, 2024 •

edited

Loading

martinbonnin Apr 10, 2024

martinbonnin Apr 10, 2024

martinbonnin Apr 10, 2024

martinbonnin Apr 10, 2024

martinbonnin Apr 10, 2024

martinbonnin left a comment

martinbonnin Apr 12, 2024

martinbonnin Apr 12, 2024

		fun id(type: String, field: String, argument: String) = "$type.$field.$argument"
		fun propertyName(fieldName: String, argumentName: String) = "__${fieldName}_${argumentName}"

[Codegen] Add CompiledArgumentDefinition #5797

[Codegen] Add CompiledArgumentDefinition #5797

Conversation

BoD commented Apr 10, 2024

netlify bot commented Apr 10, 2024 • edited Loading

✅ Deploy Preview for apollo-android-docs canceled.

martinbonnin Apr 10, 2024

Choose a reason for hiding this comment

martinbonnin Apr 10, 2024

Choose a reason for hiding this comment

martinbonnin Apr 10, 2024

Choose a reason for hiding this comment

martinbonnin Apr 10, 2024

Choose a reason for hiding this comment

martinbonnin Apr 10, 2024

Choose a reason for hiding this comment

martinbonnin left a comment

Choose a reason for hiding this comment

martinbonnin Apr 12, 2024

Choose a reason for hiding this comment

martinbonnin Apr 12, 2024

Choose a reason for hiding this comment

netlify bot commented Apr 10, 2024 •

edited

Loading