Apollo-ios: Crash when gets new interface type from server

[camp191]

Version

• ApolloClient 0.14.0

Problem

when I have the following types:

interface Notification {
  id: ID!
}

type NotificationArticle implements Notification {
  id: ID!
  article: Article!
}

type NotificationComment implements Notification {
  id: ID!
  comment: Comment!
}

and my app use query from this fragment.

fragment NotificationFragment on Notification {
  id
  ... on NotificationArticle {
    ... ArticleFragment
  }
  ... on NotificationComment{
    ... CommentFragment
  }
}

Later, I add another implementation of Notification below

type NotificationShare implements Notification {
  id: ID!
  share: Share!
}

Without updating app with the new schema, my app will crash when I tried to read id from NotificationShare. The crash happens in generated API.swift below

public var id: GraphQLID {
 get {
  return resultMap["id"]! as! GraphQLID
 }
 set {
   resultMap.updateValue(newValue, forKey: "id")
 }
}

It seems that Apollo Core doesn't read any field from response when __typename is not exist in fragment's possible types below (from GraphQLExecutor.swift)

if let runtimeType = runtimeType, fragment.possibleTypes.contains(runtimeType) {
  try collectFields(selections: fragment.selections, forRuntimeType: runtimeType, into: &groupedFields, info: info)
}

Expected Behavior

The app should not crash because id is a field in Notification interface.

[designatednerd]

Woof, yeah, that's unfortunate. For now you'll need to regenerate your code off the new schema, but you're correct that shouldn't happen in theory.

If you'd like to take a stab at fixing it in typescript, please take a look at our tooling repo and the apollo-swift-codegen package , but honestly, given that we're going to move to Swift codegen soonish, simply regenerating your code off the new schema is probably going to be much faster.

I'm adding this ticket to the Swift Codegen project to make sure it's addressed in updated codegen. Once we switch to using Codable for this we should be in a much better place to get this to make sure something like this still parses the identifier correctly but doesn't crash.

[koenpunt]

Ah, so it still happens; this is essentially a duplicate of #400 (closed).

[chalermpong]

@designatednerd
This crash still exist in apollo 0.20.0.

• Is the new Swift Codegen ready to use yet?
• Could you put link to the ticket you added in Swift Codegen project here

[designatednerd]

@chalermpong Quick answers: No, and this is the ticket in the Swift Codegen Project.

[pm-dev]

Wow, just ran into this. This means you should never ever query fields on an interface using a fragment. Yikes.
@designatednerd If I took a stab at fixing this would you create a release for it?

[designatednerd]

@pm-dev You'd need to fix it in the typescript repo, I'm not sure what your appetite is for that type of hurt.

If you take a look at #1242 you can see some of what i'm going to be doing in order to prevent this from happening again in the new Swift codegen, but it's been s l o o o o w going so I don't have an ETA.

[pm-dev]

You'd need to fix it in the typescript repo

@designatednerd I think the best fix would involve changes to both the typescript repo and this repo. Currently this library skips populating the resultMap for any concrete types it's not aware of (in the possibleTypes array). To simply prevent the crash, we would only need to update the tooling repo.
However, I think the correct behavior would be to not skip an unknown concrete type. In the OP's example this would mean a NotificationShare concrete type returned from gql would still be returned from this library populated with the fields queried on the interface - id in the example. This would require updates to this repo as well.

I'm curious, is this the behavior that we'll see after the swift codegen re-write is complete?

[designatednerd]

Yes, if you take a look at how an unknown variant of an interface or union type is handled in that PR, an unknown type will still parse, and so it will successfully parse along with all other returned data in the future for fragments (that's my next biiiiiig thing to tackle once I'm unblocked on the union types).

I'm not 100% sure of what the consequence of not skipping things in the possibleTypes array would be with the current codegen, but if you're interested in diving into this rabbit hole, please feel free!

[pm-dev]

Yes, if you take a look at how an unknown variant of an interface or union type is handled in that PR, an unknown type will still parse, and so it will successfully parse along with all other returned data in the future for fragments (that's my next biiiiiig thing to tackle once I'm unblocked on the union types).

I'm not 100% sure of what the consequence of not skipping things in the possibleTypes array would be with the current codegen, but if you're interested in diving into this rabbit hole, please feel free!

That's great.
My current plan is to remove all fragments on interfaces in my project, however if I have spare time I'll come back and see if I can create a PR to fix this. Thanks for your help.

[designatednerd]

OK thanks for the heads up - if you can think of a way we can do it from the Swift side alone, I'd love to hear it too.

[pm-dev]

@designatednerd I came back to this today in an attempt to fix the bug but ran into a couple questions:

After cloning this repo and running the ApolloTests target, about 81 of the 238 enabled tests were failing. Is this expected?

I believe this issue would be fixed if we remove this check https://github.com/apollographql/apollo-ios/blob/main/Sources/Apollo/Decoding.swift#L60 but I'd like to have tests verify this doesn't cause any additional issues. Unfortunately, API.swift (codegen file) used in tests was created with the option --mergeInFieldsFromFragmentSpreads and therefore doesn't use the type GraphQLFragmentSpread which is required to expose this bug, meaning we can't really test this change. Any suggestions on how to proceed?

[designatednerd]

There's a local server that needs to be running with the Star Wars schema to run the tests. If that's not running, that would cause a lot of failures. You can check it out with install-or-update-starwars-server.sh in scripts and then running npm start.

In terms of dealing with the fragment spread, let me think about that - is there a particular reason why you don't use the option to merge in fields from fragment spreads?