apollographql / apollo-ios

📱  A strongly-typed, caching GraphQL client for iOS, written in Swift.
https://www.apollographql.com/docs/ios/
MIT License
3.89k stars 728 forks source link

`asXXXXXXX` property on a union never returning `nil` if selection set empty #3326

Closed vincentisambart closed 9 months ago

vincentisambart commented 9 months ago

Summary

On a union, when you have an inline fragment just getting __typename and no real field, the asMyObject property does not return nil when the type is not MyObject.

Version

1.8.0

Steps to reproduce the behavior

As an example, take this GraphQL schema:

type Query {
  main: Main!
}

type Main {
  thing: Thing!
}

union Thing = MyObject | MyError

type MyObject {
  id: Int!
}

type MyError {
  code: Int!
}

And this query:

query {
  main {
    thing {
      ... on MyObject {
        __typename
      }
    }
  }
}

If main.thing is a MyError, I would expect main.thing.asMyObject to return nil but it does not.

Note that if you add a real field to the inline fragment's selection set like the example below, you will correctly get nil if main.thing is a MyError

query {
  main {
    thing {
      ... on MyObject {
        __typename
        id
      }
    }
  }
}

Logs

No response

Anything else?

Looking at the difference of behavior, if the inline fragment's selection set is empty (other that __typename), the AsMyObject struct implements ApolloAPI.CompositeInlineFragment, but does not if the selection set has other fields in it. That changes the version of _asInlineFragment called (in the empty case func _asInlineFragment<T: CompositeInlineFragment>() -> T? gets called, in the non empty case func _asInlineFragment<T: SelectionSet>() -> T? gets called).

It looks like to me that an empty inline fragment should not implement CompositeInlineFragment, especially at is has no __mergedSources, meaning that _asInlineFragment will never return nil, but I'm not sure of the implications of this.

calvincestari commented 9 months ago

Hi @vincentisambart, thanks for raising this issue. It seems funky to have two different behaviours so we'll certainly take a look into it and try get a reproduction case.

calvincestari commented 9 months ago

@vincentisambart - I debugged this today and I think you're correct that ApolloAPI.CompositeInlineFragment is the incorrect protocol to be conforming to. FWIW, the reason _asInlineFragment is giving the incorrect result is because the generated model incorrectly defines an empty __mergedSources list which is used to match against the returned types.

Going back a few releases it looks like this bug has existed for a while, but I've got a reproduction case now and we'll get it resolved.

calvincestari commented 9 months ago

@vincentisambart - this has been fixed and will go out in the next release.

vincentisambart commented 9 months ago

@calvincestari Thanks a lot!