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

.asThing returns nil within fragments #2980

Closed
dafurman opened this issue Apr 25, 2023 · 11 comments
Closed

.asThing returns nil within fragments #2980

dafurman opened this issue Apr 25, 2023 · 11 comments
Assignees
Labels
bug Generally incorrect behavior needs investigation planned-next Slated to be included in the next release

Comments

@dafurman
Copy link

dafurman commented Apr 25, 2023

Summary

While migrating from Apollo 0.53.0's makeThing() functions, I've found that .asThing properties return nil within fragments, which is a blocker for my migration to Apollo 1.x:

Narrowing it down, this seems to happen right here, with this returning false:

return __fulfilledFragments.isSuperset(of: typeIds)

This seems to happen due to a mismatch of types between _data["__fulfilled"] and __mergedSources in selection sets.
Here's an example of such a mismatch (see repro steps for more context on these types):

public static var __mergedSources: [any ApolloAPI.SelectionSet.Type] { [
  CharacterFragment.self,
  CustomHeroBadQuery.Data.Hero.self,
  CharacterFragment.AsHuman.self // Changing this to `Self.self` gets StarWarsServerTests.testCustomHeroBad() working. (See repro steps)
}

"__fulfilled": Set([
  ObjectIdentifier(Self.self), // This is missing from `__mergedSources`, incorrectly replaced with `CharacterFragment.AsHuman.self`
  ObjectIdentifier(Hero.self),
  ObjectIdentifier(CharacterFragment.self)
])

If it doesn't break anything else, it seems like the fragment-based type in __mergedSources could just be generated as Self.self instead to resolve this.

Version

1.1.2

Steps to reproduce the behavior

git clone https://github.com/dafurman/apollo-ios.git && git checkout David/myThingCasting and explore the behavior of StarWarsServerTests.testCustomHeroGood() and StarWarsServerTests.testCustomHeroBad()

Or

  1. Clone this repo
  2. Add the following to a new .graphql file or any existing one within https://github.com/apollographql/apollo-ios/tree/main/Sources/StarWarsAPI/starwars-graphql
fragment CharacterFragment on Character {
  id
  name
  ... on Human {
    homePlanet
  }
}

query CustomHeroBad {
  hero {
    ...CharacterFragment
  }
}

query CustomHeroGood {
  hero {
    id
    name
    ... on Human {
      homePlanet
    }
  }
}
  1. Run https://github.com/apollographql/apollo-ios/blob/main/scripts/run-codegen.sh
  2. Add the following generated files to the Xcode project
  • Sources/StarWarsAPI/StarWarsAPI/Sources/Operations/Queries/CustomHeroBadQuery.graphql.swift
  • Sources/StarWarsAPI/StarWarsAPI/Sources/Operations/Queries/CustomHeroGoodQuery.graphql.swift
  • Sources/StarWarsAPI/StarWarsAPI/Sources/Fragments/CharacterFragment.graphql.swift
  1. Create test functions for the following:
  /// This behaves as expected
  func testCustomHeroGood() {
    let hero = CustomHeroGoodQuery.Data.Hero.AsHuman(id: "123", name: "Luke Skywalker")
    XCTAssertEqual(hero.name, "Luke Skywalker")
    XCTAssertEqual(hero.asRootEntityType.name, "Luke Skywalker")
    XCTAssertNotNil(hero.asRootEntityType.asHuman)
    XCTAssertEqual(hero.asRootEntityType.asHuman?.name, "Luke Skywalker")
  }

  /// Casting .asHuman has trouble here
  func testCustomHeroBad() {
    let hero = CustomHeroBadQuery.Data.Hero.AsHuman(id: "123", name: "Luke Skywalker")
    XCTAssertEqual(hero.name, "Luke Skywalker")
    XCTAssertEqual(hero.asRootEntityType.name, "Luke Skywalker")
    XCTAssertNotNil(hero.asRootEntityType.asHuman) // ❌ Fails here
    XCTAssertEqual(hero.asRootEntityType.asHuman?.name, "Luke Skywalker")
  }

Logs

No response

Anything else?

No response

@dafurman dafurman added bug Generally incorrect behavior needs investigation labels Apr 25, 2023
@AnthonyMDev
Copy link
Contributor

Thanks so so much for the detailed bug report, especially the specific reproduction case and unit test. We'll look into this as soon as we can!

@AnthonyMDev AnthonyMDev self-assigned this Apr 25, 2023
@AnthonyMDev AnthonyMDev added this to the Patch Releases (1.1.x) milestone Apr 26, 2023
@AnthonyMDev AnthonyMDev added the planned-next Slated to be included in the next release label May 18, 2023
@AnthonyMDev
Copy link
Contributor

Hey @dafurman! I think this should also be fixed by the changes made in #3067. This is merged into main now. Would you be able to point your project at main and confirm if this is working for you now. If it's not, I'd like to dig deeper and get a fix out before I push the next patch version!

@dafurman
Copy link
Author

Thanks for the work on this @AnthonyMDev! The test I gave now passes, so good signs there, but I can't verify for sure if this solves my issue in my real project, as I'm now hitting this crash in codegen:
The stack trace here mentions merged selections, so I can't help but wonder if this crash is related (I haven't retried codegen from main since creating this issue, so I don't know how new it is)
Screenshot 2023-06-13 at 9 58 08 AM

@AnthonyMDev
Copy link
Contributor

Thanks for checking! Just to confirm, the codegen has been rebuilt with main and is now causing this crash in your project?

@dafurman
Copy link
Author

Yeah, seems like it could be the same issue described here actually, as I'm doing the same

For reference I'm calling try ApolloCodegen.build(with: config) from a swift tools package myself by importing ApolloCodegenLib.

#3071

@AnthonyMDev
Copy link
Contributor

I'm not able to reproduce this bug in any of our example projects yet. Would you be able to see which one of your operations is causing this and try to isolate a subsection of your schema and operation to send me that reproduces this? Depending on the complexity of your schema, that may easy or a lot of trouble.

If that's not viable, even just sharing the operation might give me a hint as to what edge case circumstances are causing this crash to trigger.

@AnthonyMDev
Copy link
Contributor

Ah that helps! Thanks!

@dafurman
Copy link
Author

One difference to note is that I'm getting this just in regular code generation though - not mocks. If you are having trouble reproducing this, I can see what I can do to help, but that's probably a conversation best left in the other issue 😄

@AnthonyMDev
Copy link
Contributor

Yeah, I noticed that! A reproduction case would be so helpful! If we can't get one together I'll keep digging though.

@AnthonyMDev
Copy link
Contributor

Okay @dafurman, main should now have the fix for the crash you were experiencing, so you can run codegen and verify everything is working properly! Please let me know ASAP. I plan to push the patch release out tomorrow morning/afternoon!

@dafurman
Copy link
Author

Nice! Looks like everything's good on my end so far with codegen from the other issue's fix, and casting in this when branching from main! Thanks a bunch for the fixes. I think this issue can be closed once the patch release goes out 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Generally incorrect behavior needs investigation planned-next Slated to be included in the next release
Projects
None yet
Development

No branches or pull requests

2 participants