https://kotlinlang.org logo
Title
s

Stylianos Gakis

05/19/2022, 7:50 AM
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?
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 🙌
b

bod

05/19/2022, 8:28 AM
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)
}
oh I'll have a look 👀
m

mbonnin

05/19/2022, 8:30 AM
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
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
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
b

bod

05/19/2022, 8:35 AM
ooh interesting!
m

mbonnin

05/19/2022, 8:37 AM
Also relevant "overloads" discussion (sorry this thread has 2 sub-threads 😅: https://kotlinlang.slack.com/archives/C5HT9AL7Q/p1637866524180300
🧠 1
👀 1
b

bod

05/19/2022, 8:38 AM
thinking out loud but could we have a "codegen way" (as opposed to a reflection way) of getting an adapter from a Data class?
m

mbonnin

05/19/2022, 8:39 AM
Yes, that's the other solution but it'd clutter the
Data
class with some extra bytecode
I kind of like the current state where
Data
is purely ... well... data
👍 1
Easy to construct/serialize, no logic at all
We'd need a way to get from
MyQuery.Data
to just
MyQuery
b

bod

05/19/2022, 8:42 AM
we could have some kind of a registry (Data class -> Adapter) maybe - but where?
or maybe an extension fun
s

Stylianos Gakis

05/19/2022, 8:47 AM
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?
b

bod

05/19/2022, 8:48 AM
yes that makes sense 👍 I'll create an issue to have this globally as well (unless one exists already) so we can track this
m

mbonnin

05/19/2022, 8:49 AM
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
b

bod

05/19/2022, 8:50 AM
oh yeah that works too
s

Stylianos Gakis

05/19/2022, 8:51 AM
Oh right, that’s even better, I can fix that up real quick
🙏 1
m

mbonnin

05/19/2022, 8:59 AM
@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
, ...)
s

Stylianos Gakis

05/19/2022, 8:59 AM
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?
m

mbonnin

05/19/2022, 9:00 AM
No strong preference for me on 1.
2. is expected, we don't track
@ApolloExperimental
APIs because there's no backward compat garantees
s

Stylianos Gakis

05/19/2022, 9:06 AM
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 🙌
m

mbonnin

05/19/2022, 9:08 AM
Yep, I'm guessing the
$default
version is a false positive from https://github.com/Kotlin/binary-compatibility-validator/
The
synthetic
keyword means no code can compile against the symbol (but it's still present in the bytecode)
:thank-you: 1
<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...
s

Stylianos Gakis

05/19/2022, 9:13 AM
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?
m

mbonnin

05/19/2022, 9:15 AM
It's ok, we've seen it in other places too. When monitoring the public API we look out for these
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
:today-i-learned: 1
s

Stylianos Gakis

05/19/2022, 9:39 AM
I’m just gonna reply to this in a different thread to avoid having this become even bigger than what it already is 😅