Deprecated arguments are missing in introspection query

[hwielenberg]

Since we upgraded from 2.02 to 2.6.0 introspection queries no longer return arguments marked as deprecated.
This doesn't seem to affect all definitions that are marked as deprecated.

I managed to reproduce the issues with new tests here:
hwielenberg@9a18e0b

Our main problem is the ArgumentAdded error in the IntrospectionClientSpec spec. But the tests show other inconsistencies as well.

Here are the failures:

sbt:caliban-tools> testOnly caliban.tools.RemoteSchemaSpec
+ RemoteSchemaSpec
  - is isomorphic
...
      -  getObject(name: String! @deprecated(reason: "Use nameV2"), nameV2: String!): Object!
      +  getObject(name: String!, nameV2: String!): Object!
...

sbt:caliban-tools> testOnly caliban.tools.IntrospectionClientSpec
+ IntrospectionClientSpec
...
  - is isomorphic
    ✗ List was not empty (size 4)
    res.isEmpty
    res = List(
        ArgumentAdded(
            argName = "name",
            target = Field(name = "getObject", typeName = "Queries"),
            optional = true
        ), 
        DirectiveDefinitionDeleted(
            directiveName = "skip"
        ), 
        DirectiveDefinitionDeleted(
            directiveName = "include"
        ),
        DirectiveDefinitionDeleted(
            directiveName = "specifiedBy"
        )
    )
    at /Users/henning.wielenberg/caliban/tools/src/test/scala/caliban/tools/IntrospectionClientSpec.scala:48 

[kyri-petrou]

Hey @hwielenberg, I think there are a couple of issues you're reporting here so let's try and take it 1-by-1. First:

Since we upgraded from 2.02 to 2.6.0 introspection queries no longer return arguments marked as deprecated.

Can you provide an example of the introspection query you're using? I think, we used to include the deprecated fields by default, but that was against the spec. There is a includeDeprecated argument on different fields in the introspection query that should be defined as true for them to be included. You can take a look at the spec on which fields that argument is supported.

This doesn't seem to affect all definitions that are marked as deprecated

OK this is weird. Can you provide some more info on this one regarding for which definitions are included by default? AFAICT it should default to false for all definitions

But the tests show other inconsistencies as well.

Thanks for providing those. I'll take a look and see where are the inconsistencies coming from

[hwielenberg]

We use SchemaLoader.fromIntrospection in our code. So if it's against the spec to include deprecated fields/arguments by default that method probably should have a parameter to enable it.

One of our clients uses this js library and run into the issue as well.
https://the-guild.dev/graphql/inspector/docs/commands/validate

[hwielenberg]

OK this is weird. Can you provide some more info on this one regarding for which definitions are included by default? AFAICT it should default to false for all definitions

I added some more deprecated fields here - but that doesn't cause any additional errors in my tests.
06b76b1

You can take a look at the spec on which fields that argument is supported.

Looking at the spec it looks like __InputValue doesn't support deprecated in the first place.
But in caliban it does:
caliban.introspection.adt.__Field#isDeprecated part of the spec
caliban.introspection.adt.__InputValue#isDeprecated not part of the spec

[hwielenberg]

Looking at the spec it looks like __InputValue doesn't support deprecated in the first place.
But in caliban it does:
caliban.introspection.adt.__Field#isDeprecated part of the spec
caliban.introspection.adt.__InputValue#isDeprecated not part of the spec

Well it is supported in the latest draft of the spec
https://spec.graphql.org/draft/#sec-Schema-Introspection.Schema-Introspection-Schema

[kyri-petrou]

OK so AFAICT there are a few different things at play here 🙃

So, first thing is that I think the server used to return deprecated fields by default (which was wrong). The server now behaves correctly, and will return the deprecated fields if includeDeprecated = true is used. The following introspection query will make the server return deprecated args

Introspection query with deprecated args included (Click to expand)

query IntrospectionQuery {
        __schema {
          queryType { name }
          mutationType { name }
          subscriptionType { name }
          types {
            ...FullType
          }
          directives {
            name
            description
            locations
            args {
              ...InputValue
            }
          }
        }
      }
      fragment FullType on __Type {
        kind
        name
        description
        fields(includeDeprecated: true) {
          name
          description
          args(includeDeprecated: true) {  # <---- HERE
            ...InputValue
          }
          type {
            ...TypeRef
          }
          isDeprecated
          deprecationReason
        }
        inputFields(includeDeprecated: true) { # <---- AND HERE
          ...InputValue
        }
      
      }
      fragment InputValue on __InputValue {
        name
        description
        type { ...TypeRef }
        defaultValue
      }
      fragment TypeRef on __Type {
        kind
        name
        ofType {
          kind
          name
          ofType {
            kind
            name
            ofType {
              kind
              name
              ofType {
                kind
                name
                ofType {
                  kind
                  name
                  ofType {
                    kind
                    name
                    ofType {
                      kind
                      name
                    }
                  }
                }
              }
            }
          }
        }
      }

Now onto the introspection client (both caliban's and external). My guess is that since the addition of the includeDeprecated argument is fairly new, perhaps it wasn't added to ensure compatibility with legacy servers. For Caliban, we should add it as an option to SchemaLoader.fromIntrospection

[ghostdogpr]

See my comment in the PR description: #1932 (comment)

If we add it we have to do like this to not break targeting servers that don't support it: #1732

[hwielenberg]

@kyri-petrou
Looks good.
Thanks!