Marshal methods and multiple fragments using the same structure

[tlevavasseur]

Marshal methods have problems with multiple fragments using (partially) the same structure

Apollo 2.5.9

Combining fragments as a workaround of the lack of inheritance :

// just using partial media structure in Light object 
fragment ProductLightFragment on Product { 
    id, 
    name, 
    media { images } 
}

// using the rest of media structure on complete product
fragment ProductCompleteFragment on Product {
   ...ProductLightFragment,
   media { videos }
}

This is just a small example, my real Product structure is really big and I really want to use fragments combination.

No problem with this as a client, the parsing works and you can reach the images via the "light" fragment and videos via the "complete" fragment in return of the query.

But using the marshal methods in my embedded mock server leads to a merge problem in the final json :

    val writer = SimpleResponseWriter(ScalarTypeAdapters(customAdapters))
    data.marshaller().marshal(writer)
     val json = writer.toJson(null)
     response.setBody(json)

Only one of the partial media structure of the two fragments is serialized, here the light version (only the images are returned in the media node of the final json). Then the client fails to parse media for the "complete" fragment.

Probably one of those 2 situations :

• one marshaller overwrites the "media" node of the other
• one marshaller cannot write in "media" node because the other already did it

[martinbonnin]

Hi 👋 !

Thanks for sending this. This problem happens when using merged fields. This is unfortunately a limitation in the 2.x version and pretty hard to fix without breaking the API.

The good news is that significant work was made in 3.x to address this issue, as well as giving more typed models using fragments as interfaces.
The default operationBased models will correctly merge the merged fields in the output json. Additionally, you can look into responseBased models (doc) to model fragments as interfaces.

Any chance you can try the just released 3.0.0-rc01 and let us know what you think?

[mateuszkwiecinski]

I think I faced exactly the same issue in 3.0.0-rc02. Repro test: https://github.com/mateuszkwiecinski/apollo-playground/blob/979d9f9d7070aaf584a6899b360445b550b25885/src/test/kotlin/com/example/Test.kt#L87

(nested response test passes on 2.x but fails on 3.0.0-rc02 with Object 'viewerId.nested({"limit":1}).0.books.0' has no field named 'id'

and it seems like there is a behavior change between 2.x and 3.x and 3.x 🤔 2.x seems to pick first fragment, 3.x picks last fragment (which in my case results id being dropped which breaks caching heavily.

Apollo 2.x	Apollo 3.x

(☝️ sceenshots generated after dropping custom viewer id generation)

edit: I can also link related issue I reported in the past: #2818 which sounds almost the same)

[mateuszkwiecinski]

Feel free to ignore my comment from above. It seem like I stumbled upon yet another issue where superfluous field in json response was written into the cache, which seems to be fixed 3.x.

Here's proper repro on 3.0.0-rc02 for both this issue and #2818: https://github.com/mateuszkwiecinski/apollo-playground/blob/7c5ae7530511bb872a7b04694a0a9423b8757555/src/test/kotlin/com/example/Test.kt#L15

3.0.0-rc02 tries to read all requested fields and throws CacheMiss in runtime.
2.5.11 seems to drop additional fragment data since the very beginning, so it's easy to spot something's wrong by looking at generated models, but the app remains stable

3.0.0-rc02 doesn't fix merged fragments issue and it seems it introduces regression to how they behave in 2.5.11

[martinbonnin]

Thanks a lot for the reproducer, that helps a ton 💙 !

Turns out there was a bug in BufferedSinkJsonWriter that did not correctly merge nested objects. This should work now with #3691. I took the liberty of adding your test case to the test suite (here) so we don't have this happening again, let me know if it's ok with you.

As a side note, if you have a lot of merged fields like book here, you might also be interested in responseBased codegen (doc here). It's a codegen that "flattens" the fragments and generates models matching 1:1 with the json response (instead of matching the GraphQL query). While it usually generates more code, it also generates models that don't have to rewind at all so the parsing is usually faster (and most importantly, is amortized during IO). Which one to use is a tradeoff so that why we included both but sounds like it could help for this specific use case.

Edit: looks like this also fixes #2818 (test case).

[mateuszkwiecinski]

🎉
It's great to hear all of that! That's the kind of new one wants to read in the morning 🚀

let me know if it's ok with you.

Sure! I probably could have adding tests in this project in the first place to make it even easier. Thanks!

As a side note, if you have a lot of merged fields like book here, you might also be interested in responseBased codegen

Nice, didn't know that 👍 I don't think I have that many merged fields in my default project (I only aimed to try all types of them in the repro), but I'll definitelly try that out and see if there's any profit, thanks again!

[martinbonnin]

This is fixed in 3.0.0-rc03. Closing this issue.

[mateuszkwiecinski]

@martinbonnin 👋 I think I found another edge case where rc-03 doesn't work if the nested fragment references a list of objects 👀

Here's updated repro: https://github.com/mateuszkwiecinski/apollo-playground/tree/fragments_with_lists
The test fails with Object 'library-1.books.0' has no field named 'id' despite the referenced book has an id and if I'm not mistaken it should be properly picked by CacheKeyGenerator
When looking at the cache content, I expected to see a book id in following places:

I tried to modify the functional test you've added, but I'm having trouble to set-up the project on Java17 :/ (btw I think I made a mistake previously when declaring CacheResolver I should have used the one migration guide suggests (it doesn't affect the previous repro, but it seems to be important now, so I updated its definition))

[martinbonnin]

Fixed in 3.0.0, hopefully for good 🤞

[tlevavasseur]

I'm not 100% done migrating to 3.0.0 (rc3) but for my initial problem it's ok

[mateuszkwiecinski]

Hello 👋 It's me again 😬
I think I found one more edge case which seems to be a behavior change when compared with 2.x: https://github.com/mateuszkwiecinski/apollo-playground/tree/cross_cache_typename - (this specific branch)
The only test there throws:

Object 'book-id' has no field named '__typename'

And if I'm not mistaken the repro is to have 2 queries, first writes data into cache, the other one tries to fetch part of the previous object, but it uses fragments as a part of its definition.

If I inline the bookData fragment the test passes which makes me think that might be an unwanted behavior.

Reproducible in 3.0.0 and 3.0.1-SNAPSHOT

[martinbonnin]

Hi @mateuszkwiecinski and apologies for huge Holidays delay. Wish you and everyone in this thread all the best for 2022 🎄 :)

This issue is somewhat different because the underlying cause is the algorithm used to add __typename.

In a nutshell, the 3.x code adds __typename when there are fragment (because this is how you do polymorphism in GraphQL and when we need __typename to parse to the correct concrete type).

This is more restrictive than 2.x that added it on all composite fields by default.

Overall, we're trying to remove the unneeded added __typename because it's less bytes on the network, less work to parse and overall leaner models (see also this issue). So I don't think we want to rollback to 2.x behaviour there.

What we can do is add an optimization that doesn't add typename if we know by construction that the fragment type condition is always true (i.e. the field is monomorphic). This will not fix all issues.

For an example, someone could do this:

interface Product
type Book implements Product

# read from network
query Query1 {
  book {
    id # "1001"
    title
  }
}

# read from cache

query Query2 {
  product(id: "1001")  {
    ... on Book {
      id
      title
    }
  }
}

__typename is not needed to parse Query1 but it is for Query2 so we'd have to store the mapping of id <-> __typename somewhere in the cache. That's doable but not easy.

All in all:

• quick workaround: (caller-side) add __typename to GetAuthor.viewer.author.topBook so that the cache can find it
• mid-term: (apollo-side) optimize and do not add __typename for monomorphic fields
• long-term: (apollo-side) store the id <-> __typename mapping in the cache

[lwasyl]

It's great to have this optimization, always fetching __typenames was a bit of a waste 🎉

I just wanted to point out that the current behavior makes it pretty easy to shoot yourself in the foot, because intuitively fragment extraction like from this:

query Foo {
  foo {
    bar
    baz
  }
}

to this:

query Foo {
  foo { 
    ...fooData 
  }
}

fragment fooData on Foo {
  bar
  baz
}

shouldn't be a functional change. It's really easy to not realize that Apollo will fetch different data in both cases and thus cause potential cache misses. Perhaps it'd be worth documenting this somehow?

[martinbonnin]

@lwasyl indeed. One more argument to remove the __typename in these cases. Let's follow up in #3774