Directives (i.e., @include) not working

[plflanagan]

I have a fragment

fragment QLOffer on Offer {
    id
    type
    name @include(if: $show)
}

that is then queried by

query Offer($offerId: Int, $show: Boolean = false) {
    offer(id: $offerId) {
        ...QLOffer
    }
}

but the name is always returned.

Some of the generated API code is

public struct QlOffer: GraphQLFragment {
  public static let fragmentString =
    "fragment QLOffer on Offer {" +
    "  __typename" +
    "  id" +
    "  type" +
    "  name @include(if: $show)" +
    "}"

  public static let possibleTypes = ["Offer"]

  public static let selections: [GraphQLSelection] = [
    GraphQLField("__typename", type: .nonNull(.scalar(String.self))),
    GraphQLField("id", type: .nonNull(.scalar(GraphQLID.self))),
    GraphQLField("type", type: .nonNull(.scalar(String.self))),
    GraphQLField("name", type: .nonNull(.scalar(String.self))),
  ]

Any suggestions, or is this a confirmed bug currently?

[martijnwalraven]

Hmmm, it is a confirmed bug that @include doesn't work on fragments right now (see #74). But it should work on fields.

Are you relying on the default value ($show: Boolean = false) here, or setting show to false explicitly? That reminds me we don't currently have support for default values.

[plflanagan]

The fields I put @include on were in a fragment; but you're saying that that should work since the @include is on a field specifically?

I was primarily using the default values to check it was working. There was a point where I was passing in a value. If I passed in false then the entire fragment would not be returned.

[martijnwalraven]

Yes, I think @include on a field should work, but there could very well be issues with this as well. I'd like to understand better what is going on so we can fix all of this for once and for all.

It's surprising that passing in false would result in the whole fragment not being returned. I would at least expect an error message there, as the fragment doesn't seem conditional (the type condition matches the parent type). Can you try again and describe the behavior you're seeing in more detail?

[martijnwalraven]

I've been able to confirm there are bugs related to the way codegen handles @skip/@include that is likely to be the cause of your issue as well. I hope to have a fix for this out next week.

[plflanagan]

Whoops! Just seeing your request. Would it still be helpful for you to describe the behavior in more detail, or do you think you have it all figured out at this point?

[martijnwalraven]

@plflanagan: I'm in the process of rewriting the way we deal with conditions, so I don't necessarily need to know more about the current behavior. This should fix all of these issues, I'll keep you updated!

[MaximusMcCann]

@martijnwalraven Also, seeing issues when trying to return an optional object, ie:

query SomeQuery($hasAccount: Boolean!, $accountId: ID!) {
  Account: node(id: $accountId) @include(if: $hasAccount) {
    ... on Account {
      status
    }
  }
  OtherThingNotNeedingAccountId: viewer {
    id
  }
}

Apollo is returning:

Error at path \"Account\": missingValue

I can see graphql return something reasonable:

{
  "data": {
    "OtherThingNotNeedingAccountId": {
      "id": "VldSOkFOT04="
    }
  }
}

The Android code handles this correctly, fwiw.

[MaximusMcCann]

Initial poc on this:
GraphQLOutputType.swift, add .skippable, which could handle the @skip & @include Directives (https://github.com/mugli/learning-graphql/blob/master/4.%20Querying%20with%20Directives.md)

public indirect enum GraphQLOutputType {
  case scalar(JSONDecodable.Type)
  case object(GraphQLSelectionSet.Type)
  case nonNull(GraphQLOutputType)
  case list(GraphQLOutputType)
  case skippable(GraphQLOutputType)
  
  var namedType: GraphQLOutputType {
    switch self {
    case .nonNull(let innerType), .list(let innerType), .skippable(let innerType):
      return innerType.namedType
    case .scalar, .object:
      return self
    }
  }
}

in GraphQLExecutor.swift, starting line 195

  /// Each field requested in the grouped field set that is defined on the selected objectType will result in an entry in the response map. Field execution first coerces any provided argument values, then resolves a value for the field, and finally completes that value either by recursively executing another selection set or coercing a scalar value.
  private func execute<Accumulator: GraphQLResultAccumulator>(fields: [GraphQLField], on object: JSONObject, info: GraphQLResolveInfo, accumulator: Accumulator) throws -> ResultOrPromise<Accumulator.FieldEntry> {
    // GraphQL validation makes sure all fields sharing the same response key have the same arguments and are of the same type, so we only need to resolve one field.
    let firstField = fields[0]
    
    var info = info
    
    let responseKey = firstField.responseKey
    info.responseKeyForField = responseKey
    info.responsePath.append(responseKey)
    
    if shouldComputeCachePath {
      let cacheKey = try firstField.cacheKey(with: info.variables)
      info.cacheKeyForField = cacheKey
      info.cachePath.append(cacheKey)
    }
    
    // We still need all fields to complete the value, because they may have different selection sets.
    info.fields = fields
    
    let resultOrPromise = resolver(object, info)
    
    return resultOrPromise.on(queue: queue).flatMap { value in
// ------Start changes------
      switch firstField.type {
      case .skippable:
        if value == nil {
          return try self.complete(value: NSNull(), ofType: firstField.type, info: info, accumulator: accumulator)
        }
      default:
        guard let value = value else {
          throw JSONDecodingError.missingValue
        }
      }
// ------End changes------   

      return try self.complete(value: value, ofType: firstField.type, info: info, accumulator: accumulator)
    }.map {
      try accumulator.accept(fieldEntry: $0, info: info)
    }.catch { error in
      if !(error is GraphQLResultError) {
        throw GraphQLResultError(path: info.responsePath, underlying: error)
      }
    }
  }

Generated code would look like:

GraphQLField("node", alias: "Account", arguments: ["id": Variable("accountId")], type: .skippable(.object(Account.self))),

[MaximusMcCann]

Something better would probably be this route:

public enum GraphQLDirective {
  case skip, include
}

public indirect enum GraphQLOutputType {
  case scalar(JSONDecodable.Type, Set<GraphQLDirective>)
  case object(GraphQLSelectionSet.Type, Set<GraphQLDirective>)
  case nonNull(GraphQLOutputType, Set<GraphQLDirective>)
  case list(GraphQLOutputType, Set<GraphQLDirective>)
  
  var namedType: GraphQLOutputType {
    switch self {
    case .nonNull(let innerType, _), .list(let innerType, _):
      return innerType.namedType
    case .scalar, .object:
      return self
    }
  }
}

[martijnwalraven]

@MaximusMcCann: Thanks for looking into this! I have an almost working implementation ready, but it turned out to be a little more complicated than I thought, and required a rewrite of the codegen compiler! I'm shooting for a beta after the weekend.

One of the complications is that it isn't enough to know whether you can skip a field, because we also need to know when we can fulfill a query from the cache, and differentiate between missing values and null values there. So we need to evaluate the conditions as part of execution. The code I'm now generating introduces an explicit GraphQLBooleanCondition node:

GraphQLBooleanCondition(variableName: "includeName", inverted: false, selections: [
  GraphQLField("name", type: .nonNull(.scalar(String.self))),
]),

There are more complications, because @skip/@include can also be specified on inline fragments or fragment spreads, and they also interact with type conditions. So you get situations like:

query HeroAndFriendsNames($includeName: Boolean!, $includeFriendsName: Boolean!) {
  hero {
    id
    name @include(if: $includeName)
    friends {
      id
    }
    friends @include(if: $includeFriendsName) {
      name
    }
    ... on Droid {
      friends {
        name
      }
    }
  }
}

Where you always fetch the names of friends of droids, but let that depend on the $includeFriendsName variable otherwise...

I believe the new compiler handles this correctly, but we'll have to do some careful testing because some of these situations get pretty hairy!

[MaximusMcCann]

@martijnwalraven excellent, cheers!!

[MaximusMcCann]

Also, believe this should go without saying, I would happily help test! ha. Just give me a heads up in this channel. Cheers!

[martijnwalraven]

I just released Apollo iOS 0.7.0-alpha.1, which uses the updated codegen and should support @skip/@include. There have been quite a few changes to the codegen, so there might be slight differences in the generated code (especially for type conditions). That's one of the reasons I've labeled it alpha for now. Would be great if people could try it out and provide feedback!

[MaximusMcCann]

@martijnwalraven Cheers!
First main issue: Anonymous closure argument not contained in closure.

mutation DisableTwoFactorAuth ($input: DisableTwoFactorAuthInput!) {
  disableTwoFactorAuth(input: $input) {
    result {
      didSucceed
      inputError
    }
    user {
      id
      isTwoFactorAuthEnabled
    }
  }
}

[martijnwalraven]

@MaximusMcCann: Thanks for the report! This should be fixed in Apollo iOS 0.7.0-alpha.2.

[MaximusMcCann]

Cheers @martijnwalraven Next up, this query uses the @include directive, but posting the entire query is not reasonable here.

query SecurityDetail($hasAccount: Boolean!, $accountId: ID!, $securityId: ID!) {
  Account: node(id: $accountId) @include(if: $hasAccount) {
    ... on Account {
 ----more code----

[MaximusMcCann]

Thanks again for all the hard work!!

[martijnwalraven]

@MaximusMcCann: I've made some changes in Apollo iOS 0.7.0-alpha.3 that will hopefully fix this. Let me know how it goes!

[MaximusMcCann]

@martijnwalraven Seeing duplicate generated code on alpha.3. We have a fragment on a fragment. Cheers!!

sudo query:

query GetChartData ($id: ID!) {
  Thing: node(id: $id) {
    ...ThingWithChartData
  }
}
fragment ThingWithChartData on Thing {
  oneYearData: ChartData {
  ...ChartData
  }
  twoYearData: ChartData {
  ...ChartData
  }
  threeYearData: ChartData {
  ...ChartData
  }
}
fragment ChartData on ChartData {
  xValue
  yValue
}

          public struct Fragments {
            public var snapshot: Snapshot

            public var chartData: ChartData {
              get {
                return ChartData(snapshot: snapshot)
              }
              set {
                snapshot += newValue.snapshot
              }
            }

            public var chartData: ChartData {
              get {
                return ChartData(snapshot: snapshot)
              }
              set {
                snapshot += newValue.snapshot
              }
            }
          }
        }

[martijnwalraven]

@MaximusMcCann: Thanks for the report, but I'm having some trouble reproducing this. Nested fragments doesn't seem to be the issue, and neither seems including the same fragment multiple times in different aliased subselections. Any chance you could reproduce this with the Star Wars schema or get a better sense of what causes it?

[MaximusMcCann]

@martijnwalraven Will try to have something to you by today, else it will be Thursday 👍

[MaximusMcCann]

@martijnwalraven K, here's some more detail on our query.

query ThingDetail($hasAccount: Boolean!, $accountId: ID!, $thingId: ID!) {
  Account: node(id: $accountId) @include(if: $hasAccount) {
    ... on Account {
      // stuff
    }
  }
  Thing: node(id: $thingId) {
    ...DetailedThing
  }
}

fragment DetailedThing on Thing {
  id
  oneYearQuote: ChartData(range: {length: {amount: 1, unit: YEARS}}, interval: WEEK) {
    ...ChartData
  }
  threeYearQuote: ChartData(range: {length: {amount: 3, unit: YEARS}}, interval: MONTH) {
    ...ChartData
  }
  fiveYearQuote: ChartData(range: {length: {amount: 5, unit: YEARS}}, interval: MONTH) {
    ...ChartData
  }
}

// Separate file
fragment ChartData on ChartData {
  xValue: date
  yValue: price
}


// Part of another query file
fragment ChartDataMoreData on ChartData {
  date
  value: price
  dividends {
    amount
  }
  split {
    ratio
  }
}

Issues:

          public struct Fragments {
            public var snapshot: Snapshot

            public var chartData: ChartData {
              get {
                return ChartData(snapshot: snapshot)
              }
              set {
                snapshot += newValue.snapshot
              }
            }

            public var chartData: ChartData {
              get {
                return ChartData(snapshot: snapshot)
              }
              set {
                snapshot += newValue.snapshot
              }
            }
          }
        }

        public struct ThreeYearQuote: GraphQLSelectionSet {
          public static let possibleTypes = ["ChartData"]

          public static let selections: [GraphQLSelection] = [
            GraphQLField("__typename", type: .nonNull(.scalar(String.self))),
            GraphQLField("__typename", type: .nonNull(.scalar(String.self))),
            GraphQLField("date", alias: "xValue", type: .nonNull(.scalar(String.self))),
            GraphQLField("price", alias: "yValue", type: .nonNull(.scalar(Double.self))),
            GraphQLField("__typename", type: .nonNull(.scalar(String.self))),
            GraphQLField("__typename", type: .nonNull(.scalar(String.self))),
            GraphQLField("date", alias: "xValue", type: .nonNull(.scalar(String.self))),
            GraphQLField("price", alias: "yValue", type: .nonNull(.scalar(Double.self))),
          ]

Cheers! Hope this helps.

Side note, anyway to not have everthing dumped into a massive single file. Xcode gets real slow opening the file/linking to definitions.

[martijnwalraven]

@MaximusMcCann: I'm sorry, but I still have trouble reproducing this. What is Thing here? An object type or an interface?

[martijnwalraven]

@MaximusMcCann: I managed to reproduce this with a query like:

query HeroAndFriendsNamesWithFragmentTwice {
  hero {
    friends {
      ...CharacterName
    }
    ... on Droid {
      friends {
        ...CharacterName
      }
    }
  }
}

Not sure how that relates to the query above, but I fixed the issue in Apollo iOS 0.7.0-alpha.4. Could you check if that fixes your issue too?

[MaximusMcCann]

@martijnwalraven It compiles now, but still having issues when the Account object is not pulled back. Apollo hits fatal error: unexpectedly found nil while unwrapping an Optional value where account is. I see the code generated for account is:

    public var account: Account? {
      get {
        return (snapshot["Account"] as! Snapshot?).flatMap { Account(snapshot: $0) }
      }

I believe the as! is the culprit (as Xcode becomes unresponsive and won't hit the break point).

Cheers!

[martijnwalraven]

@MaximusMcCann: I thought I changed the force cast. Is this using the latest v0.17.0-alpha.12?

[MaximusMcCann]

I see, I was on v0.7.0-alpha.8. Will try again this afternoon. cheers!

[MaximusMcCann]

@martijnwalraven The highest is see for alpha is 8, fyi.

[martijnwalraven]

@MaximusMcCann: Oops sorry, I was confusing the Apollo iOS with the apollo-codegen version. Let me see what could be going on here. Is the account field optional in the schema, or only conditionally included?

[MaximusMcCann]

@martijnwalraven In this case the node can return an optional.

query ThingDetail($hasAccount: Boolean!, $accountId: ID!, $thingId: ID!) {
  Account: node(id: $accountId) @include(if: $hasAccount) { // node(id: $accountId) can be optional regardless of the include directev
    ... on Account {
      // stuff
    }
  }
  Thing: node(id: $thingId) {
    ...DetailedThing
  }
}

[martijnwalraven]

@MaximusMcCann: I missed one of the cases when making those changes. Can you try again with v0.7.0-alpha.9?

[MaximusMcCann]

@martijnwalraven Cheers, alpha.9 doesn't have the nil issue!

[fruitcoder]

@MaximusMcCann is this issue fixed?

[MaximusMcCann]

@fruitcoder close this, cheers

[asaadreh-meister]

Hello!

I'm having some trouble using the @include directive with mocks. It works correctly in the actual code but in my when I mock the models, every time I use include with either a fragment or a field, it returns nil, regardless of whether the passed in condition is true or false. Is this a known issue? is there a work around (apart from not using includes at all 😅 )?