I am in a situation where I have `Operation.Data` ...

# apollo-kotlin

[Stylianos Gakis]

I am in a situation where I have Operation.Data and I would like to get their Json String representation. I have kind of by accident stumbled upon this function which is jvm only [1] and I think this almost is what I am looking for.

However just had a question, there isn’t a toJsonString equivalent in there which is what I would like to get instead. And I can’t quite build my own version of it locally since I adapter() is private in that file and I don’t feel like copying that one into our project.
I guess what I can do instead locally is imitate what the Adapter<D>.toJsonString is doing, with something like this right?

fun Operation.Data.toJsonString(
    scalarTypeAdapters: CustomScalarAdapters = CustomScalarAdapters.Empty,
    indent: String? = null,
): String {
    val buffer = Buffer()
    val jsonWriter = BufferedSinkJsonWriter(buffer, indent)
    toJson(jsonWriter, scalarTypeAdapters)
    return buffer.readUtf8()
}

But optimally, I’d like that function to be available to me from the library, where it can use the private adapter() function and not have to deal with buffers etc. in my local code. I am definitely not super confident with using them.

So in short, do you think it’d fit to have a function with this body directly under this one, or should I just do the workaround I posted above (or something else if it is wrong of course)

fun Operation.Data.toJsonString(customScalarAdapters: CustomScalarAdapters = CustomScalarAdapters.Empty, indent: String? = null): String {
  return adapter().toJsonString(this, customScalarAdapters, indent)
}

Random question:
[1] Is this due to the getDeclaredField calls etc? Is that using reflection only available on the JVM or something?

[Stylianos Gakis]

Hey went ahead and made a PR just so I can forget about this for now and give you the time you need to respond when you’re available. As noted in the PR itself, if you don’t like this idea just feel free to close the PR 🙌

[bod]

Hello, World!

First, [1], yes exactly 🙂 Not sure what the state of reflection is on Native and JS but I believe this code would work only on the JVM.

About toJsonString(), could you simplify it like this?

fun Operation.Data.toJsonString(
    scalarTypeAdapters: CustomScalarAdapters = CustomScalarAdapters.Empty,
    indent: String? = null,
): String = buildJsonString(indent) {
  toJson(this, scalarTypeAdapters)
}

[bod]

oh I'll have a look 👀

[mbonnin]

For some background there, we historically had overloads with String and File and I removed them just before going 3.0 because I wasn't sure how it should all work

[mbonnin]

We have this problem in a ton of places and I'd like to address this in a "global" way if possiblie, making sure the APIs are uniform accross the repo

[mbonnin]

For the reflection, kotlinx.serialization has some nice tricks that allows "reflection-like" feature on native and JS: https://github.com/Kotlin/kotlinx.serialization/blob/d43ce106f9f1d37f2b9ba6feb6aa0[…]/core/nativeMain/src/kotlinx/serialization/internal/Platform.kt

[bod]

ooh interesting!

[mbonnin]

Also relevant "overloads" discussion (sorry this thread has 2 sub-threads 😅: https://kotlinlang.slack.com/archives/C5HT9AL7Q/p1637866524180300

[bod]

thinking out loud but could we have a "codegen way" (as opposed to a reflection way) of getting an adapter from a Data class?

[mbonnin]

Yes, that's the other solution but it'd clutter the Data class with some extra bytecode

[mbonnin]

I kind of like the current state where Data is purely ... well... data

[mbonnin]

Easy to construct/serialize, no logic at all

[mbonnin]

We'd need a way to get from MyQuery.Data to just MyQuery

[bod]

we could have some kind of a registry (Data class -> Adapter) maybe - but where?

[bod]

or maybe an extension fun

[Stylianos Gakis]

Benoit, you’re totally right, I for some reason was thinking that buildJsonString was internal API and didn’t use it in my local code, and therefore forgot to use it in the apollo-kotlin code too.
I can update my PR, but it seems like you’d optimally want a different approach anyway right? And I am perfectly happy to use the buildJsonString locally anyway.
So I can instead close the PR and reference this chat here, what do you think?

[bod]

yes that makes sense 👍 I'll create an issue to have this globally as well (unless one exists already) so we can track this

[mbonnin]

I'm happy with the addition as long as it's flagged @ApolloExperimental so that we get a chance to change it if we have other thoughts

[bod]

oh yeah that works too

[Stylianos Gakis]

Oh right, that’s even better, I can fix that up real quick

[mbonnin]

@Stylianos Gakis Question: what's your use case where you have data without having operation around? In the Apollo codebase, these 2 are always found together (see ApolloResponse , ApolloStore.writeOperation , ...)

[Stylianos Gakis]

2 questions:
1. I assumed this comment was for the library function as well, but I just realized maybe in there we can just do the adapter() approach directly without the buildJsonString , which one do you prefer?
2. When I added the @ApolloExperimental annotation, the apiDump produced a different result. This wasn’t something I expected, is that okay?

[mbonnin]

No strong preference for me on 1.

[mbonnin]

2. is expected, we don't track @ApolloExperimental APIs because there's no backward compat garantees

[Stylianos Gakis]

That makes sense, but what confuses me then is that if I remove the funtion completely I get a different dump.
Without function -> nothing
With function and experimental annotation 1 line ->

public static synthetic fun toJsonString$default (Lcom/apollographql/apollo3/api/Operation$Data;Lcom/apollographql/apollo3/api/CustomScalarAdapters;Ljava/lang/String;ILjava/lang/Object;)Ljava/lang/String;

With function and no annotation 2 lines ->

public static final fun toJsonString (Lcom/apollographql/apollo3/api/Operation$Data;Lcom/apollographql/apollo3/api/CustomScalarAdapters;Ljava/lang/String;)Ljava/lang/String;
public static synthetic fun toJsonString$default (Lcom/apollographql/apollo3/api/Operation$Data;Lcom/apollographql/apollo3/api/CustomScalarAdapters;Ljava/lang/String;ILjava/lang/Object;)Ljava/lang/String;

I guess the difference between the two is the “synthetic” keyword in there, but I am not familiar with how to properly read this file format

These may be a bit silly questions, but I haven’t worked with something like this before so I don’t quite understand it all, thanks for bearing with me 🙌

[mbonnin]

Yep, I'm guessing the $default version is a false positive from https://github.com/Kotlin/binary-compatibility-validator/

[mbonnin]

The synthetic keyword means no code can compile against the symbol (but it's still present in the bytecode)

[mbonnin]

<https://github.com/Kotlin/binary-compatibility-validator/|binary-compatibility-validator> reads the symbols from the java bytecode and tries to infer what is "public API". I'd say the $default() method up there isn't public API...

[mbonnin]

Ah, it's this issue: https://github.com/Kotlin/binary-compatibility-validator/issues/58

[Stylianos Gakis]

Aha, so this is something that must have been happening to other places in the codebase too I assume. Is there something we can do to handle this? Do we have to just release the new versions with this false positive in there or is it bad enough to warrant not making such changes that hit this bug?

[mbonnin]

It's ok, we've seen it in other places too. When monitoring the public API we look out for these

[mbonnin]

All of this is informational. At the end of the day, there's no 100% automatic way to enforce API compatibility so it's left to our judgement

[Stylianos Gakis]

I’m just gonna reply to this in a different thread to avoid having this become even bigger than what it already is 😅