Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include deprecated arguments and directives in introspection #4702

Merged
merged 26 commits into from
Feb 27, 2023

Conversation

BoD
Copy link
Contributor

@BoD BoD commented Feb 22, 2023

See #4699

@BoD BoD requested a review from martinbonnin as a code owner February 22, 2023 16:45
@netlify
Copy link

netlify bot commented Feb 22, 2023

Deploy Preview for apollo-android-docs canceled.

Name Link
🔨 Latest commit e13d4aa
🔍 Latest deploy log https://app.netlify.com/sites/apollo-android-docs/deploys/63fcb481b76808000701c92d

Copy link
Contributor

@martinbonnin martinbonnin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

Copy link
Contributor

@martinbonnin martinbonnin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As hinted by @calvincestari (Thanks!), we'll need to make includeDeprecated on args and inputFields optional as they're only supported in the draft version of the spec and some servers might not support it.

@martinbonnin
Copy link
Contributor

🐶 🥫 suggestion: could we recursively use apollo-kotlin to build a typesafe introspection query to be used by apollo-kotlin? Maybe have 2 versions of the schema, one for the current spec and one for the draft?

@BoD
Copy link
Contributor Author

BoD commented Feb 23, 2023

I tried to use apollo-kotlin but couldn't use the 'draft spec' introspection types with it 😿.

Instead I updated the built-ins so it will possible in the future after this is published in a version. Also removed the restriction preventing from overriding the introspection types in a project's schema, so if this happens again in the future we'll be able to make it work.

Also, removed the flag and instead try the 'draft spec' query first, and fallback to the 'October2021 spec' query if it failed.

@martinbonnin
Copy link
Contributor

  • When converting from introspection Json -> SDL, we should keep the introspection type definitions, directive definitions and scalar definitions
  • Providing a schema with type definitions should override the builtins.graphql one
  • For backward compat reason, we're still allowing users to pass a schema without introspection type definitions
  • To do in separate PR:
    • remove data classes
  • investigate if we can remove CompilesSchemaType and others

@@ -1512,7 +1512,8 @@
},
"defaultValue": null
}
]
],
"isRepeatable": false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is (yet another) question: I think we'll want to keep being able to read "old" json schemas. Without this, we're taking the risk of having codegen fail for a lot of users that are using current (or older) spec without "isRepeatable"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you revert that part? I'd like to keep our test suite unchanged unless we make some behaviour change that we need to document either in the CHANGELOG or migration guide (but I don't think this is the case here?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 makes sense

import kotlin.Unit
import kotlin.collections.List

public class Introspection() : Query<Introspection.Data> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are these files coming from?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woops added by mistake, good catch!

Copy link
Contributor

@martinbonnin martinbonnin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same way we need to support querying both versions from introspection, we'll need to be able to read both versions of Jsons. Maybe it's "just" a matter of setting default values for isRepeatable and others potentially missing fields.

Comment on lines +208 to +219
fun copy(
queryType: QueryType = this.queryType,
mutationType: MutationType? = this.mutationType,
subscriptionType: SubscriptionType? = this.subscriptionType,
types: List<Type> = this.types,
) = copy(
queryType = queryType,
mutationType = mutationType,
subscriptionType = subscriptionType,
types = types,
directives = emptyList(),
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing this is a binary compatibility method? If yes, the problem is that it shadows the default .copy and gets resolved from normalize() for an example.

Maybe make it hidden?

Suggested change
fun copy(
queryType: QueryType = this.queryType,
mutationType: MutationType? = this.mutationType,
subscriptionType: SubscriptionType? = this.subscriptionType,
types: List<Type> = this.types,
) = copy(
queryType = queryType,
mutationType = mutationType,
subscriptionType = subscriptionType,
types = types,
directives = emptyList(),
)
@Deprecated("For binary compatibility only.", level = DeprecationLevel.HIDDEN)
fun copy(
queryType: QueryType = this.queryType,
mutationType: MutationType? = this.mutationType,
subscriptionType: SubscriptionType? = this.subscriptionType,
types: List<Type> = this.types,
) = copy(
queryType = queryType,
mutationType = mutationType,
subscriptionType = subscriptionType,
types = types,
directives = emptyList(),
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly - good catch! 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm looks like that failed in apiCheck because the method is marked as 'synthetic'. I think this is ok though cause it's still binary compatible.

BoD and others added 5 commits February 27, 2023 14:24
…/ast/introspection/IntrospectionSchema.kt

Co-authored-by: Martin Bonnin <[email protected]>
…/ast/introspection/IntrospectionSchema.kt

Co-authored-by: Martin Bonnin <[email protected]>
…ql/apollo3/gradle/test/ConvertSchemaTests.kt

Co-authored-by: Martin Bonnin <[email protected]>
…ql/apollo3/gradle/test/ConvertSchemaTests.kt

Co-authored-by: Martin Bonnin <[email protected]>
@BoD BoD merged commit f5b68a4 into main Feb 27, 2023
@BoD BoD deleted the introspection-deprecated-arguments-and-directives branch February 27, 2023 17:58
BoD added a commit that referenced this pull request Mar 14, 2023
Co-authored-by: Martin Bonnin <[email protected]>
(cherry picked from commit f5b68a4)
@martinbonnin martinbonnin mentioned this pull request Mar 15, 2023
This was referenced Apr 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants