If I am hitting the error “<Expected END_OBJECT bu...

# apollo-kotlin

[Stylianos Gakis]

If I am hitting the error “Expected END_OBJECT but was ${peek()} at path ${getPathAsString()}” in code where I am not doing anything fancy (Just executing a query) what would you suggest I start looking into first? Is there anything obvious I could be messing up 🤔 Having a hard time figuring it out.

[mbonnin]

Could it be that your server sends extra fields?

[mbonnin]

What does peek() say?

[Stylianos Gakis]

Got a failing test here to be able to debug this, with the response as it comes from the backend in the string right below.
Exact crash is:
“Expected END_OBJECT but was NAME at path data.embarkStory.passages.5.response.__typename”
Got the response from here with variables:

{
  "name": "Web Onboarding SE - Quote Cart Needer?quoteCartId=65db82c7-0475-4793-84b8-57a1562cde5b",
  "locale": "en_SE"
}

And Query:

query EmbarkStory($name: String!, $locale: String!) { embarkStory(name: $name, locale: $locale) { startPassage computedStoreValues { key value } passages { name id externalRedirect { data { location } } offerRedirect { data { keys } } messages { __typename ...MessageFragment } response { __typename ...MessageFragment ...ResponseExpressionFragment ... on EmbarkGroupedResponse { title { __typename ...ResponseExpressionFragment } items { __typename ...MessageFragment } each { key content { __typename ...MessageFragment } } } } redirects { __typename ... on EmbarkRedirectUnaryExpression { unaryType: type to passedExpressionKey passedExpressionValue } ... on EmbarkRedirectBinaryExpression { binaryType: type to key value passedExpressionKey passedExpressionValue } ... on EmbarkRedirectMultipleExpressions { multipleExpressionType: type to passedExpressionKey passedExpressionValue subExpressions { __typename ...ExpressionFragment } } } action { __typename ... on EmbarkSelectAction { selectData: data { options { link { __typename ...EmbarkLinkFragment } keys values badge api { __typename ...ApiFragment } } } } ... on EmbarkTextAction { textData: data { key placeholder mask subtitle link { __typename ...EmbarkLinkFragment } api { __typename ...ApiFragment } } } ... on EmbarkTextActionSet { textSetData: data { link { __typename ...EmbarkLinkFragment } textActions { data { key mask placeholder title } } api { __typename ...ApiFragment } } } ... on EmbarkPreviousInsuranceProviderAction { previousInsurerData: data { next { __typename ...EmbarkLinkFragment } skip { __typename ...EmbarkLinkFragment } storeKey insuranceProviders { id name logo { variants { __typename ...IconVariantsFragment } } } } } ... on EmbarkExternalInsuranceProviderAction { externalInsurerData: data { next { __typename ...EmbarkLinkFragment } skip { __typename ...EmbarkLinkFragment } storeKey } } ... on EmbarkNumberAction { numberActionData: data { __typename ...EmbarkNumberActionFragment } } ... on EmbarkNumberActionSet { numberActionSetData: data { numberActions { data { key placeholder unit label maxValue minValue title } } link { __typename ...EmbarkLinkFragment } } } ... on EmbarkDatePickerAction { storeKey label next { __typename ...EmbarkLinkFragment } } ... on EmbarkMultiAction { multiActionData: data { key maxAmount addLabel link { __typename ...EmbarkLinkFragment } components { __typename ... on EmbarkMultiActionNumberAction { numberActionData: data { key placeholder label unit } } ... on EmbarkDropdownAction { dropDownActionData: data { label key options { value text } } } ... on EmbarkSwitchAction { switchActionData: data { label key defaultValue } } } } } ... on EmbarkAudioRecorderAction { audioRecorderActionData: data { storeKey label next { __typename ...EmbarkLinkFragment } } } ... on EmbarkAddressAutocompleteAction { component addressAutocompleteActionData: data { placeholder key link { __typename ...EmbarkLinkFragment } api { __typename ...ApiFragment } } } } api { __typename ...ApiFragment } tooltips { title description } allLinks { __typename ...EmbarkLinkFragment } tracks { eventName eventKeys includeAllKeys customData } quoteCartOfferRedirects { data { id expression { __typename ...ExpressionFragment } selectedInsuranceTypes } } variantedOfferRedirects { data { selectedKeys allKeys expression { __typename ...ExpressionFragment } } } } } }  fragment BasicExpressionFragment on EmbarkExpression { __typename ... on EmbarkExpressionUnary { unaryType: type text } ... on EmbarkExpressionBinary { binaryType: type key value text } }  fragment ExpressionFragment on EmbarkExpression { __typename ...BasicExpressionFragment ... on EmbarkExpressionMultiple { multipleType: type text subExpressions { __typename ...BasicExpressionFragment ... on EmbarkExpressionMultiple { multipleType: type text subExpressions { __typename ...BasicExpressionFragment ... on EmbarkExpressionMultiple { multipleType: type text subExpressions { __typename ...BasicExpressionFragment } } } } } } }  fragment MessageFragment on EmbarkMessage { expressions { __typename ...ExpressionFragment } text }  fragment ResponseExpressionFragment on EmbarkResponseExpression { text expressions { __typename ...ExpressionFragment } }  fragment EmbarkLinkFragment on EmbarkLink { name label hidden }  fragment GraphQLResultsFragment on EmbarkAPIGraphQLResult { key as }  fragment GraphQLErrorsFragment on EmbarkAPIGraphQLError { contains next { __typename ...EmbarkLinkFragment } }  fragment GraphQLVariablesFragment on EmbarkAPIGraphQLVariable { __typename ... on EmbarkAPIGraphQLSingleVariable { key from as } ... on EmbarkAPIGraphQLGeneratedVariable { key storeAs type } ... on EmbarkAPIGraphQLMultiActionVariable { key from variables { __typename ... on EmbarkAPIGraphQLSingleVariable { key from as } ... on EmbarkAPIGraphQLGeneratedVariable { key storeAs type } } } ... on EmbarkAPIGraphQLConstantVariable { key value as } }  fragment ApiFragment on EmbarkApi { __typename ... on EmbarkApiGraphQLQuery { queryData: data { query results { __typename ...GraphQLResultsFragment } errors { __typename ...GraphQLErrorsFragment } variables { __typename ...GraphQLVariablesFragment } next { __typename ...EmbarkLinkFragment } } } ... on EmbarkApiGraphQLMutation { mutationData: data { mutation results { __typename ...GraphQLResultsFragment } variables { __typename ...GraphQLVariablesFragment } errors { __typename ...GraphQLErrorsFragment } next { __typename ...EmbarkLinkFragment } } } }  fragment IconVariantsFragment on IconVariants { dark { svgUrl } light { svgUrl } }  fragment EmbarkNumberActionFragment on EmbarkNumberActionData { key placeholder unit label maxValue minValue link { __typename ...EmbarkLinkFragment } }

[Stylianos Gakis]

Sorry just wrote the above for myself too, to easily reproduce if we don’t figure it out 😄

[Stylianos Gakis]

NAME is what I get back doing .peek() on the actualReader right before the crash

[Stylianos Gakis]

[mbonnin]

I get this error trying to run the test:

java.io.FileNotFoundException: /Users/mbonnin/git/hedvig-android/apollo/src/main/graphql/com/hedvig/android/owldroid/schema.graphqls (No such file or directory)

[mbonnin]

I'll try with the one from playground

[Stylianos Gakis]

And no I don’t see any extra fields that the backend is returning. The one which is failed to be parsed at position 5 seems to be this:

"response": {
  "__typename": "EmbarkGroupedResponse",
  "title": {
    "__typename": "EmbarkResponseExpression",
    "text": "Extra buildings",
    "expressions": []
  },
  "items": [],
  "each": {
    "key": "extraBuildings",
    "content": {
      "__typename": "EmbarkMessage",
      "expressions": [],
      "text": "{type.Label}, {area} square meters"
    }
  }
}

Which seems to be the correct ones according to the schema, since the response is EmbarkGroupedResponse

... on EmbarkGroupedResponse {
  title {
    ...ResponseExpressionFragment
  }
  items {
    ...MessageFragment
  }
  each {
    key
    content {
      ...MessageFragment
    }
  }
}

[Stylianos Gakis]

Did you try running first
./gradlew apollo:downloadGiraffeApolloSchemaFromIntrospection
Our schema isn’t checked out in the repo

[mbonnin]

I didn't, I just ran the test

[mbonnin]

Damn, looks like I need the lokalize strings to build the app. Let me see if I can move the test to a non-android module

[Stylianos Gakis]

Ah right there’s that too. Benoit last time went around this by providing some fake values to make the build happy, since that step is not part of the build really, but a gradle task to download the strings, you don’t need that there afaik

[mbonnin]

maybe I can regex my way out of this

[Stylianos Gakis]

I am sorry there’s this friction, I need to try and go through this process myself some time to make this setup as effortless as I can for people trying to check the repo out

[Stylianos Gakis]

What exactly is the issue maybe I can help you with by sending you some stuff over on a DM

[mbonnin]

Execution failed for task ':app:processDebugResources'.
> A failure occurred while executing com.android.build.gradle.internal.res.LinkApplicationAndroidResourcesTask$TaskAction
   > Android resource linking failed
     com.hedvig.app-mergeDebugResources-111:/values/values.xml:24: error: resource string/SETTINGS_LANGUAGE_SWEDISH (aka <http://com.hedvig.dev.app:string/SETTINGS_LANGUAGE_SWEDISH|com.hedvig.dev.app:string/SETTINGS_LANGUAGE_SWEDISH>) not found.

[bod]

ooh yeah last time you simply sent me the strings.xml by dm 🙂

[Stylianos Gakis]

Yeap just sent them in a DM to Martin as well

[mbonnin]

So there are no extra fields sent by the server but since the json reader was rewinded to parse the fragments, it still sees some 😕

[Stylianos Gakis]

Interesting! Is it due to this combination of inline fragments and real fragments at the same time? Seems like a very complicated combination to get everything right.
I am glad I could give you a repro of this bug though, at least we know what’s happening now.

[mbonnin]

Yea, I think this only happens if there are both inline and named fragment

[Stylianos Gakis]

Okay let me try replacing the inline fragment with a real one and see if it crashes

[Stylianos Gakis]

https://github.com/HedvigInsurance/android/commit/fd1da5712f569751b20e96e9ab98f18d235046ce This does in fact make the test pass 🥳

[mbonnin]

🥳

[Stylianos Gakis]

Do you want me to make a proper error in the apollo-kotlin repository to make it easier for you?

[Stylianos Gakis]

Also thanks so much for taking the time to help me once again! I appreciate it a ton!

[mbonnin]

Thank you for the precious feedback!

[mbonnin]

I can make a small reproducer in the repo

[mbonnin]

I think it shouldn't be too hard.

[mbonnin]

Maybe the good fix is to actually ignore the error....

[Stylianos Gakis]

Okay so no need for a new GitHub issue I guess. I’d like to be able to follow it to see updates on it. I am now a bit paranoid that we may be doing inline and normal fragments in more places in the app, and it might be a bit easier for me to just wait for a fix 😄 We’ve not merged the 3.x changes in our app yet, but I am actively trying to make that happen atm.

[Stylianos Gakis]

yeah it might be, why was the error there in the first place? Was it preventing some wrong behavior or more that it was a state that you thought never should have happened but it does in fact happen in this very specific scenario?

[mbonnin]

Oh yes, please open an issue

[mbonnin]

Issues are always good!

[mbonnin]

We might make a hotfix release for that actually, not sure, still trying to wrap my head around what this implies

[mbonnin]

Was it preventing some wrong behavior or more that it was a state that you thought never should have happened but it does in fact happen in this very specific scenario?

It is required to make https://github.com/apollographql/apollo-kotlin/issues/3344 work

[mbonnin]

(although we have not added the strict mode yet... but it feels like an interesting thing to add)

[mbonnin]

Making that work with fragments and operationBased models sounds.... interesting...

[Stylianos Gakis]

Yeap I see. Haha yeah it all sounds very interesting, and hard 🤣

[mbonnin]

With responseBased models, it's not an issue because we read all the fields all the time. But with operationBased models and fragments only having "local" knowledge of the selection set, it feels a lot more challenging...

[mbonnin]

Maybe the tradeoff is "no strict mode for operationBased models"

[mbonnin]

Or we need to add more state to MapJsonReader for the strict mode 🤔

[Stylianos Gakis]

Added this issue and linked to this discussion too! I figured since I posted a lot of the repro steps etc in here it’s enough to add a rough description and then link here for the rest 😄

[mbonnin]

That's perfect, thanks!

[Stylianos Gakis]

As far as your messages above, I am afraid I can’t quite follow all your thoughts, I am not that well versed in the library internals unfortunately to confidently give ideas regarding this. If anything, the “no strict mode for operationBased” sounds like a very reasonable workaround at least as a first step.

[mbonnin]

Sure thing! Sorry for my ramblings!

[mbonnin]

I think we'll do "no strict mode for operationBased" at first and then add an "opt-in strict mode for operationBased" if we find out it's required

[Stylianos Gakis]

Thinking out loud is very valuable, I wouldn't call it a rambling. I actually appreciate it when you do that, sometimes I learn stuff that way.
And yeah that idea sounds very reasonable to me!

[mbonnin]

Fix is there: https://github.com/apollographql/apollo-kotlin/pull/4214

[mbonnin]

I think it hasn't been a bigger issue because it only affects compat models but we should certainly make a release about this.

[Stylianos Gakis]

Right, there’s that which I have to take care of sometime as well I suppose. I dropped this because I was looking into responseBased back in the day but hit this problem and simply put it to the side ever since.
I am thinking though maybe I should go into operationBased from compat at some point though. I didn’t see any mention of how one would do this move in the migration to v3 docs. I can see you say compat is “similar” to operationBased but is there a specification somewhere that can explain the differences to me if there are any?

[mbonnin]

operationBased drops the .fragments intermediate field and renames asFoo to onFoo . That's mostly it

[mbonnin]

(and doesn't collect inline fragments parent fields, hence the renaming)

[Stylianos Gakis]

Aha I see!
I’ll probably do this eventually then. Will also be another of these cases where I have to manually go and change a lot of code due to how we use these models 😅

[Stylianos Gakis]

I am sorry I don’t think I understand what “doesn’t collect inline fragments parent fields” means 🧐

[mbonnin]

{
   name
   ... on Droid {
      primaryFunction
   }
}

compat does:

Hero(val name: String, val asDroid: AsDroid?)
AsDroid(val name: String, val primaryFunction: String)

operationBased does:

Hero(val name: String, val onDroid: OnDroid?)
OnDroid(val primaryFunction: String)

[Stylianos Gakis]

Aha gets rid of this duplication of data, nice this makes sense, thank you for explaining!

[mbonnin]

We should certainly put this somewhere but this is hard TBH

[Stylianos Gakis]

Somewhere in the docs you mean? A section to give a hint to the migrators how to get off of the compat model generation? Or?

[mbonnin]

Yes, document it somewhere

[mbonnin]

Maybe there https://www.apollographql.com/docs/kotlin/advanced/response-based-codegen

[mbonnin]

For migrations, this would ideally be a intelliJ refactoring. But we're nowhere near that unfortunately...

[Stylianos Gakis]

I just saw the design doc does explain these differences. The super simple way would be in the “Migrating to Apollo Kotlin 3.0” doc to have a section about why codegenModels.set(MODELS_COMPAT) exists and link to the design doc.
And yeah an IntelliJ refactoring would be optimal, but it sounds quite complicated to do it.

[mbonnin]

Great idea. I'll do this 👍

[mbonnin]

https://github.com/apollographql/apollo-kotlin/pull/4215

[Stylianos Gakis]

Yep that’s perfect! Great job 🥳

[mbonnin]

Thanks for rising this 🙂