apollographql / apollo-kotlin

:rocket:  A strongly-typed, caching GraphQL client for the JVM, Android, and Kotlin multiplatform.
https://www.apollographql.com/docs/kotlin
MIT License
3.71k stars 652 forks source link

ErrorPath meta in data fields #4733

Open ProVir opened 1 year ago

ProVir commented 1 year ago

Use case

Hi!

We need to be able to find graphql errors that are the reason for the absence of a particular field. There is some explicit logic in this binding, but apollo itself provides data only in the form in which they come - this is not enough.

Example: Field data.some.value.subvalue.meta is null, In errors there is a reason for the field data.some.value. Obviously, there will be an obvious reason for the null field in the form of a graph error that can be returned as a result of mapping.

But how can this be done at the code level automatically for all fields? This rule has some non-obvious backend rules that you need to know about (we tested).

I am attaching a clipping from our code to solve this problem as an example. But the main pain is manual work and the danger of typos, as well as strict compliance with implicit rules.

ApolloExampleClient.zip

Start with TestApiClient, GraphQLResponseProvider and GraphQLValueProvider.

Examples mappings and valid rules (from our internal wiki): To correctly search for errors and generate the message for exception, you must explicitly specify the path to the received field in the form of a string. Example: provider.valueOrThrow("some.data") { some?.data }

We are tied to the rules of the backend for the formation of the path of fields - it is on the path that the backend will return in the graph error if the field cannot be obtained. If we make a mistake in at least one character, if necessary, the error search for this field will stop working.

query GetSomeMovies(firstId: Int!, secondId: Int!) {
    some {
        ... someFragment
    }
    # The `firstMovie` label is used instead of `movie`
    firstMovie: movie(id: $firstId) {
        ... movieFragment
    }
    # The `secondMovie` label is used instead of `movie`
    secondMovie: movie(id: $secondId) {
        ... movieFragment
    }
    content(contentId: $firstId) {
        ... on Movie {
            downloadId
        }
    }
}

fragment someFragment on SomeData {
    data {
        value
    }
}

fragment movieFragment on BlockData {
    title
}

In code:

// There is no fragment in the path, i.e. we allegedly make the path without a fragment
val someValue = provider.valueOrThrow("some.data.value") { some?.someFragment?.data?.value }

// There is no type indication in the path, i.e. we allegedly do the path without it
val downloadId = provider.valueOrThrow("content.downloadId") { content?.onMovie?.downloadId }

// Labels are used - they replace the original name of the field/query with our
val firstMovieTitle = provider.valueOrThrow("firstMovie.title") { firstMovie?.movieFragment?.title }
val secondMovieTitle = provider.valueOrThrow("secondMovie.title") { secondMovie?.movieFragment?.title }

Describe the solution you'd like

What I would like from Apollo There is an understanding that at the level of kotlin model generation there is knowledge about how to correctly form these paths automatically.

Is it possible to somehow come up with logic so that we can get these paths from apollo itself? Perhaps there will be an analogue of ValueProvider as we did.

BoD commented 1 year ago

Hi!

There isn't anything like this at the moment. In theory, a mapping between the generated models' fields and their path in the response JSON could be exposed in some way. This would certainly involve changes in the codegen. Did you have an idea of an API for how this could be made visible?

Another thought is that when using the response-based codegen, the shape of the generated models will more closely match the shape of the response JSON, so this may help, but not exactly sure how (also the different codegens have their own pros/cons to be aware of).

ProVir commented 1 year ago

There are a few ideas.

In addition to the main (current) generation approach, to provide a additional wrappers (value providers). Something like what we did - ValueProvider.

Or another option is to provide the correct paths for the fields in some way nearby. This will not remove manual work, but will make their receipt accurate and safe. This option seems to be the easiest to implement, but less convenient to use.

Or it is possible to solve the final problem at all - to be able to explicitly request an error as a reason for each field.

It seems that such functionality is very lacking for Apollo, because working with errors is now separated from the main answer - that according to the idea of the graphql should not be so in the end.

martinbonnin commented 1 year ago

Or it is possible to solve the final problem at all - to be able to explicitly request an error as a reason for each field.

It's actually very possible but it's not the default in GraphQL (because it would make everything extra verbose)

This talk is a very nice illustration of how you could move error handling to your schema: https://www.youtube.com/watch?v=RDNTP66oY2o

val someValue = provider.valueOrThrow("data.some.value") { data?.some?.value }

Out of curiosity, is the issue that data and some are nullable? If you don't want to handle these checks at the callsite, you can have the parsers check the nullability status for you:

query GetValue {
  some @nonnull {
    value
  }
}

Then your usage becomes like so:

val data = try {
  apolloClient.query(myQuery).execute().data!!
} catch (e: Exception) {
  // If an error happens on field data.some.value, it will be caught here
}

if (data != null) {
   data.some.value // no safe call here
} else {
  // handle errors here
}
ProVir commented 1 year ago

Out of curiosity, is the issue that data and some are nullable? If you don't want to handle these checks at the callsite, you can have the parsers check the nullability status for you:

The problem is to find the real GraphQLError from errors, which is the reason for the absence of a value in the field.

In this case, instead of the usual Exception, we can return a more informative one - that's exactly what we need. Moreover, for some types of GraphQLError, we can get your own Exception.

In the example I gave that we have to explicitly duplicate the path in the form of a string - it is along this path that an error will be searched in the list, compared with GraphQLError.path by the value of each of them (see example project).

martinbonnin commented 1 year ago

I see. Thanks for the details! 🙏

instead of the usual Exception, we can return a more informative one

I know this is a bigger change but if it's something that needs to be surfaced to the user, modeling the users in your schema would give you all the type safety. Quoting @sachee's blog post (https://sachee.medium.com/200-ok-error-handling-in-graphql-7ec869aec9bc):

type User {
  id: ID!
  name: String
}
type Suspended {
  reason: String
}
type IsBlocked {
  message: String
  blockedByUser: User
}

And then:

"User, or reason we can't get one normally"
union UserResult = User | IsBlocked | Suspended

That gives you a nice type safe error explaining the reason you would not be able to get a given User (but that requires a server change)

(follow up comment incoming with additional more complex client side brainstorming...)

martinbonnin commented 1 year ago

Potential client-side only solutions

1. Every field is now wrapped under some kind of Result class

class GraphQLResult<T> {
  fun successOrNull(): T
  fun successOrThrow(): T
  fun errorOrNull(): GraphQLError
}

class Data(val user: GraphQLResult<User>) 

which would then be used like this:

apolloClient.query(query).execute().data.successOrThrow().user.successOrThrow()

The thrown exception would contain the GraphQL error if any so that you could customize the error message as needed

2. Make the parser throw on null fields

query {
  user @nonull(onError: THROW) {
    email
  }
}

If the parser sees a null field in a position where null is not expected, it stops the parsing. Since it knows the path, it can lookup the GraphQLError and throw it alongside a regular Kotlin exception for error message customization.

3. Another solution?

ProVir commented 1 year ago

I know this is a bigger change but if it's something that needs to be surfaced to the user, modeling the users in your schema would give you all the type safety.

This is one of the methods that we already use for other cases. But in our case, this does not fit - our backend is arranged differently and it actively uses graphql errors to provide information about what exactly went wrong. There are also a lot of error options, doing this for each case at the data level will further complicate our already huge and complex backend.

  1. Every field is now wrapped under some kind of Result class

It seems to be a closer option to us, but it seems cumbersome to use

  1. Another solution?

It may be difficult to conveniently refuse to manually fill in the path, but doing it safely from the Apollo side is also a good solution.

Example Instead of:

val someValue = provider.valueOrThrow("some.data.value") { some?.someFragment?.data?.value }

Use:

val someValue = provider.valueOrThrow(
    path = { somePathChain.someFragment.data.value.path },
    value = { some?.someFragment?.data?.value } 
)

That is, in each class, in addition to nested fields, there will be an analogue for them - a global path from the root with the ability to receive it in a chain.

public data class Data(
    public val movie: Movie?,

    public val moviePath: GraphQLValuePath,
    public val moviePathChain: GraphQLValuePath.Chain,
  ) : Query.Data

This is just an example, the names and approach are discussed.

martinbonnin commented 1 year ago

There are also a lot of error options, doing this for each case at the data level will further complicate our already huge and complex backend.

Well, it could be argued that handling complexity in the backend is better since it saves handling the complexity on each client. It's doing the work once instead of doing it for web/iOS/Android. Plus you get a type system for the different error options and you don't have to rely on strings.

Back to your proposal:

public data class Data(
    public val movie: Movie?,

    public val moviePath: GraphQLValuePath,
    public val moviePathChain: GraphQLValuePath.Chain,
  ) : Query.Data

Do we need both moviePath and moviePathChain? Sounds like what's needed here is another class that mirrors Data except that it will only contain paths.

Given this GraphQL query:

{
  node {
    ... on Movie {
      duration
    }
  }
}

Generate this Kotlin code:

// data
class Data(val node: Node) {
  class Node(onMovie: OnMovie)
  class OnMovie(duration: Double)
}

// paths
class Path(val node: Node, val __path: String) { // '__' to avoid name clashes with GraphQL
  class Node(onMovie: OnMovie, __path: String) 
  class OnMovie(duration: Duration) // no __path here because onMovie is a synthetic field
  class Duration(__path: String) // leaf 
}

// response
class ApolloResponse(val data: Data, val path: Path)

Usage:

// getting data:
response.data?.node?.onMovie?.duration // 1.56

// getting path
response.path.node.onMovie.duration // "node.duration"

This more or less double the amount of generated code, even for fields where you don't need any special handling.

Make the parser throw on null fields

Going back to my proposal 2. above, any reason why it would not work? Looking at your code:

// There is no fragment in the path, i.e. we allegedly make the path without a fragment
val someValue = provider.valueOrThrow("some.data.value") { some?.someFragment?.data?.value }

// There is no type indication in the path, i.e. we allegedly do the path without it
val downloadId = provider.valueOrThrow("content.downloadId") { content?.onMovie?.downloadId }

// Labels are used - they replace the original name of the field/query with our
val firstMovieTitle = provider.valueOrThrow("firstMovie.title") { firstMovie?.movieFragment?.title }
val secondMovieTitle = provider.valueOrThrow("secondMovie.title") { secondMovie?.movieFragment?.title }

Looks like it's all or nothing. Either you display something or you throw an error. So throwing a global exception on the first encountered error would work and simplify this code a lot. You wouldn't have to do any error handling during usage, it's all handled while parsing:

val response: ApolloResponse?
try {
  response = apolloClient.query(myQuery).execute()
  exception = null
} catch (e: ApolloException) {
  response = null
  exception = e
}

if (response != null) {
  // display the data
} else {
  if (exception is ApolloGraphQLException) {
    // handle GraphQL errors
    val graphqlError = exception.error
    println(graphqlError.message)
  } else {
    // handle network errors
  }
}

Or do you need to maybe display "content.downloadId", show an error for "firstMovie.title" and display seconMovie.title?

ProVir commented 1 year ago

Well, it could be argued that handling complexity in the backend is better since it saves handling the complexity on each client. It's doing the work once instead of doing it for web/iOS/Android. Plus you get a type system for the different error options and you don't have to rely on strings.

Alas, we cannot influence this, the backend has long fixed such error handling as correct - the error is part of the response.

Back to your proposal:

Overall it looks good, but:

  1. I want there to be an Interface by which it was possible to get __path inside our ValueProvider (we have a generic, we don't know the specific type inside).

  2. onMovie is a synthetic field - then if we stop at this value, it will not be possible to automatically get a path from it through the Interface, it is better to let the field in it duplicate its parent value or there will be another way to explicitly report it.

  3. I would place Paths in the root Data itself and fix it at the Operation.Data (in order to depend only on the Operation.Data heir at the generic level)

  4. It turns out that in our ValueProvider<T, P> there will now be two generics - for the data itself and for the duplicate path. If we make it possible to get such an object from any nested type, then we will not be able to complicate this part, and the real reading of the transmitted path will be only at the time of the error. But the big minus is not just doubled generation, but increased at times. (((

Make the parser throw on null fields, proposal 2

There is a question about the query scheme - is it on the client side and will there be no backend when sending it?

It looks tentatively like a solution that covers most cases. It seems that such an approach would not be superfluous to implement only because it really suits the majority.

But it is not suitable for us from some complex and frequent cases:

query GetSomeMovies(firstId: Int!, secondId: Int!) {
    firstMovie: movie(id: $firstId) {
        ... movieFragment
    }
    secondMovie: movie(id: $secondId) {
        ... movieFragment
    }
}

fragment movieFragment on BlockData {
    title @nonull(onError: RESULT)
}

And result after mapping:

data class SomeMovies(
    val firstMovie: Result<Movie>,
    val secondMovie: Result<Movie>,
)

We often need to receive an error not for the entire request at once, but only for a specific scope. Also, when we receive some fragments in some Query, we need to respond to an error, and in another Query ignore and simply ignore in final list (used mapNotNull).

But you can come up with a solution using this approach, where the fields marked in this way will have a value wrapper in the form of a Result with an value or an graphql error.

Then, together with mapping, it will be possible to decide how to read such a field - with an error or as null. If the field just came null without a corresponding error - also be able to give information with the path (it is necessary for correct logging).

There are also cases when it is important for us to mark a nested field as required, but the zeroing occurred in the hierarchy above.

Most likely, we will mark @nonnull, perhaps not even at the zero field - what to do here, you will need to mark all nullable filelds in chain? In Kotlin, we use an optional chain and the passed path takes into account that the error can to be searched only for part of the path.

It is not always convenient to think about the whole chain, it is easier to request the need for some kind of final field.

Example:

// Throwed GraphQLError(path = "some") - field some is null, so some?.someFragment?.data?.value == null
val someValue = provider.valueOrThrow("some.data.value") { some?.someFragment?.data?.value }
martinbonnin commented 1 year ago

Thanks for the feedback. Comments below

Build a separate path models hierarchy

  1. onMovie is a synthetic field - then if we stop at this value, it will not be possible to automatically get a path from it through the Interface, it is better to let the field in it duplicate its parent value or there will be another way to explicitly report it.

It makes no sense to check for an error on fragment synthetic fields because the backend is not aware of them and will never send an error for them.

I would place Paths in the root Data itself and fix it at the Operation.Data (in order to depend only on the Operation.Data heir at the generic level)

Agreed, much more simple. Could work like this:

// data
class Data(val node: Node, val __nodePath: NodePath) {
  class Node(onMovie: OnMovie)
  class OnMovie(duration: Double)

  class NodePath(onMovie: OnMoviePath, __path: String) 
  class OnMoviePath(duration: DurationPath) // no __path here because onMovie is a synthetic field
  class DurationPath(__path: String) // leaf   
}

// response
class ApolloResponse(val data: Data)
  1. I want there to be an Interface by which it was possible to get __path inside our ValueProvider (we have a generic, we don't know the specific type inside).

Not sure what you mean. Your current valueOrThrow is like this:

 fun <T> valueOrThrow(pathName: String, value: D.() -> T?): T

You would change it to:

 fun <T> valueOrThrow(path: D.() -> String, value: D.() -> T?): T

The call site would change from

// current
val someValue = provider.valueOrThrow("node.duration") { node?.onMovie?.duration }

// proposed
val someValue = provider.valueOrThrow(
     path = { nodePath.onMovie.duration.__path },
     value =  { node?.onMovie?.duration }
)     
  1. It turns out that in our ValueProvider<T, P> there will now be two generics - for the data itself and for the duplicate path. If we make it possible to get such an object from any nested type, then we will not be able to complicate this part, and the real reading of the transmitted path will be only at the time of the error.

Can you ellaborate a bit more? I don't think you'd need 2 generics?

But the big minus is not just doubled generation, but increased at times. (((

Yea, that's the part that gets me quite worried...

Make the parser throw on null fields, proposal 2

There is a question about the query scheme - is it on the client side and will there be no backend when sending it?

The client doesn't have any schema. But we could use client directives to control codegen

In your example:

query GetSomeMovies(firstId: Int!, secondId: Int!) {
    firstMovie: movie(id: $firstId) {
        ... movieFragment
    }
}

fragment movieFragment on BlockData {
    title @nonull(onError: RESULT)
}

If the directive is on title, it's the title field that will be a Result, not firstMovie. Also the directive should probably be a simple @result because the generated code is the same whether there is an error or not.

All in all, I'd say it'd look like this:

query GetSomeMovies(firstId: Int!, secondId: Int!) {
    firstMovie: movie(id: $firstId) @result {
        ... movieFragment
    }
}

fragment movieFragment on BlockData {
    title
}
class Data(val firstMovie: Result<FirstMovie>) {
  class FirstMovie(val movieFragment: MovieFragment?) 
  class MovieFragment(val title: String)
}

We often need to receive an error not for the entire request at once, but only for a specific scope.

By scope, you mean per-field, right? If that's the case, that means parsing needs to read errors (they may be at the beginning or at the end) and then populate the models. It's all doable but a significant departure from our current parsers that try to stream as much as possible.

There are also cases when it is important for us to mark a nested field as required, but the zeroing occurred in the hierarchy above.

So if you're trying to read, say data.movie.duration and movie is null you would need to get the data.movie error (and not the inexisting data.movie.duration one?). In that case, you could walk the path and stop on the first error?

ProVir commented 1 year ago

We need to clarify a couple of important points about our project in order to understand the context.

1. We have two providers - ResponseProvider (used for the root field) and ValueProvider (this is nested and used for mapping further).

For example, ResponseProvider to start mapping, and also somewhere for a fragment, ValueProvider is reused for nested values.

That is why it is important to be able to work with the path not only from the root, but also from the middle - from any field stored in ValueProvider.

class GraphQLResponseProvider<D : Operation.Data>(
    private val data: D?,
    private val errorsProvider: GraphQLResponseErrorsProvider,
) {
    ...
    fun <T> valueOrThrow(pathName: String, value: D.() -> T?): T
    fun <T : Any> valueProviderOrThrow(pathName: String, value: D.() -> T?): GraphQLValueProvider
}

class GraphQLValueProvider<V : Any>(
    val value: V,
    val path: GraphQLValuePath,
    internal val errorsProvider: GraphQLResponseErrorsProvider,
) {
....
}

2. At the output, after processing the data from the graphql, we get the final business model. This is our own type, it is mapped from the api model generated by apollo.

Work through ResponseProvider and ValueProvider occurs only at the mapping stage - obtaining the final business models from the apollo api models, outside of our repositories, the project does not even know about the existence of apollo.

For example, I cited this, this is exactly the final business model:

data class SomeMovies(
    val firstMovie: Result<Movie>,
    val secondMovie: Result<Movie>,
)

And the generated apollo model will of course be different. But I wanted to show by my example that if one of the important fields does not appear in the fragment, then it will not fail the entire request, but only one scope - only one field in the business model will be with an error, the 2nd scope will be have data.

3. We have a very large and highly loaded backend, different clients and everyone has their own data processing needs.

Therefore, we cannot use models and backend logic 1:1, we sometimes need to prepare it for our clients. We also have a lot of reused fragments, a large nesting of data, all this is divided into modules.

For example, the largest whitelist (the query and all the fragments used in it) is almost 1000 lines, and the number of queries is already about 100 (not all large, there are many small queries), there will be even more.

Our mapping does not always look simple, sometimes it contains some business logic for the correct formation of a state that is correct for our applications.

For all this, we use (Kotlin Multiplatform Mobile) KMM - to use the general logic of receiving data from the backend and give it to 4 platforms - iOS, tvOS, Android and AndroidTV. The implementation of KMM helped us a lot with GraphQL, thank you so much for supporting this opportunity! (I started implementing KMM and Apollo into the project at the time of the appearance of alpha 1 for apollo 3)

Also, in the given Example of the project, I added only the smallest part of our GraphQLClient - to show how we work with mapping (api models of apollo -> business models of the project), because the problem is in this part.

Therefore, it is not always possible to anticipate all the needs, even in the error handling discussed here.

Below are the answers to your comments

Consider this example:


// current
val movieProvider = provider.valueProviderOrThrow("node") { node?.onMovie }
val durationValue = movieProvider.valueOrThrow("duration") { duration }

// proposed

// provider: ResponseProvider<SomeQuery.Data>
// ValueProvider<SomeQuery.Data.OnMovie, SomeQuery.Data.OnMoviePath>
val movieProvider = provider.valueProviderOrThrow(
     path = { nodePath.onMovie },
     value =  { node?.onMovie }
)

val durationValue = movieProvider.valueOrThrow(
     path = { duration },
     value =  { duration }
)    

The only way to do this is with two generics in ValueProvider. Only now we are tied to the path type of the query itself and will not be able to write universal mapping for the reused fragment:

fun toMovie(provider: ValueProvider<SomeMovieFragment>) -> Movie { ... }

It is precisely because of the presence of a ValueProvider that takes into account what can be reused in different queries and there is a need to be able to get a path from any field.

Proposal 2

The client doesn't have any schema. But we could use client directives to control codegen

Maybe that's not how I asked the question.

We write queries in graphql files, the contents of this file and its fragments are sent to the backend, such content is stored in each query in OPERATION_DOCUMENT, right?. I want to understand - will such directive as @nonull be sent to the backend as part of a general json request? If yes, will it not turn out that the backend will not be able to ignore it on his side?

If the directive is on title, it's the title field that will be a Result, not firstMovie.

That's right, I described above that the model shown in the example is not the generated apollo, but the project business model.

Also the directive should probably be a simple @result because the generated code is the same whether there is an error or not. I agree, this is a shorter and simpler option.

By the way, within the issue of this, I would have thought of a directive that would:

  1. Stored Result with a value or a specific graphql error
  2. Would store explicitly __path for other cases: then it seems that it will be able to cover any of our cases (not only work with errors) and will not create a massive extra code generation.

For example, we can decide that the value from the backend in the field is incorrect and we again need a path to create our own Exception.

If there is such a directive, then this proposal turns out to be the most convenient and universal option for everyone.

By scope, you mean per-field, right?

No, it means a group of fields with different nesting.

For example, firstMovie and secondMovie are independent scopes that exist by themselves, but some of the required fields inside them with any nesting in depth may not be and only the one scope turns out to be an error, but not the rest of the query. Under the scope, it can be represented as an independent query, just the final of the graphql query consists of several such and is requested at once.

So if you're trying to read, say data.movie.duration and movie is null you would need to get the data.movie error (and not the inexisting data.movie.duration one?). In that case, you could walk the path and stop on the first error?

True, if we could not get the duration, then we do not delve into the mapping exactly because of which part of the chain this happened. But the automatic error search will find exactly the one that was exactly for the really empty value in the chain.

Our logic is as follows:

fun findError(path: GraphQLValuePath) = errors.firstOrNull { error ->
    path.startsWith(error.path)
}

data class GraphQLValuePath(val raw: List<String>) {
    fun startsWith(subPath: GraphQLValuePath): Boolean =
        if (raw.isEmpty() || subPath.raw.isEmpty() || subPath.raw.size > raw.size) {
            false
        } else if (raw.size == subPath.raw.size) {
            subPath.raw == raw
        } else {
            subPath.raw == raw.subList(0, subPath.raw.size)
        }
}

If there really was an error on one of the fields, then it means that only one error will be found and only on that field, which turned out to be null. There can be no error on the field before it - the data came, on the field after it too - the error happened higher in the hierarchy.

martinbonnin commented 1 year ago

Thanks for writing this! Comments inline:

That is why it is important to be able to work with the path not only from the root, but also from the middle - from any field stored in ValueProvider.

I see, thanks for clarifying this 👍

But I wanted to show by my example that if one of the important fields does not appear in the fragment, then it will not fail the entire request, but only one scope - only one field in the business model will be with an error, the 2nd scope will be have data.

At some point we'll have to clarify what a "scope“ is. In your example, I think firstMovie and secondMovie are both scope and fields. From now on, I'll think of "scope" as something in the app domain and "field" as something in the GraphQL domain

I started implementing KMM and Apollo into the project at the time of the appearance of alpha 1 for apollo 3

Oh wow that's quite the ride! Thanks for all the feedback along the way by the way💙 . You're commenter #.9 at the moment in the issue leaderboard 🙂 .

in the given Example of the project, I added only the smallest part of our GraphQLClient - to show how we work with mapping (api models of apollo -> business models of the project), because the problem is in this part. Therefore, it is not always possible to anticipate all the needs, even in the error handling discussed here.

It would be really handful to have a more complete example with the usages and everything. If we ever get to implement this, we should add that as tests. I don't want to get your hopes too high though but having a complete example would help a ton crafting good APIs.

I want to understand - will such directive as @nonull be sent to the backend as part of a general json request? If yes, will it not turn out that the backend will not be able to ignore it on his side?

Apollo-specific client directives are stripped before being sent to the server. This is done here.

For example, we can decide that the value from the backend in the field is incorrect and we again need a path to create our own Exception. If there is such a directive, then this proposal turns out to be the most convenient and universal option for everyone.

I'm not sure I'm following there. Do you have a specific API in mind?

All in all this is a complex topic. Any code/project you have that demonstrate the use case and feature would help a ton. Maybe just skip Apollo completely and write the models manually (including the one with the paths). And then we can see how to generate these?

ProVir commented 1 year ago

Apollo-specific client directives are stripped before being sent to the server.

Great!

It would be really handful to have a more complete example with the usages and everything. If we ever get to implement this, we should add that as tests. I don't want to get your hopes too high though but having a complete example would help a ton crafting good APIs.

Our client is built to take into account the specifics of our project, depends on our types and is unlikely to be suitable for everyone. It's just a layer of business logic that hides the nuances of working with the API, in this case through Apollo. At the same time, Apollo itself is at the center of this implementation and greatly simplifies working with GraphQL.

I am not sure that I will be able to provide the entire part of the project (this is a big piece tied to our other parts of the project), but I will try to come up with something. Also, our api is private and I definitely will not be able to provide a solution that will fully work.

Most likely, it will be an issue format in github with a feature suggestion like this one.

I'm not sure I'm following there. Do you have a specific API in mind?

This means when the data from the backend came in string format and we have to additionally process them. Or another case is when the data is in several fields and we realized that their datasets are not compatible with each other. I.e. it is part of the mapping business logic that checks the correctness of not only the format of the graphql query itself, but also the data itself that came.

Proposal

Thank you for your time and discussion, it seems that I started to have ideas to solve the problem after our discussions. It seems to combine both proposal into one.

You can create a directive @meta (or with a different name) that generates additional code in the model api in the specified fields.

query GetSomeMovies(firstId: Int!, secondId: Int!) {
    firstMovie: movie(id: $firstId) {
        ... movieFragment @meta // non-optional value
    }
}

fragment movieFragment on BlockData {
    title @meta // optional value
}
class Data(val firstMovie: FirstMovie) {
  class FirstMovie(val movieFragment: FieldWithMeta<MovieFragment>) 
  class MovieFragment(val title: FieldWithMeta<String?>)
}

What is additionally generated for the field:

  1. val [field]: FieldWithMeta<FieldType> - a wrapper with a meta instead of the field itself.
  2. FieldWithMeta<T>.value: Value - the value itself.
  3. FieldWithMeta<T>.error: GraphQLError? - an error field that will have a value if the field is null and an error is found.
  4. FieldWithMeta<T>.path: String - the path to this field, even if it is a synthetic field. You can add an additional flag to this in meta - whether the field is a synthetic field. Perhaps even specifying the type - fragment or another type.
  5. Sets of methods for getting data from the type FieldWithMeta, for example getValueOrThrow().

It is required to be able to specify such a directive not only for optional fields, but also for regular ones (errors cannot come for them). This is necessary to know the path even to such a field when necessary.

We are discussing the naming of the type and the directive, but we would like the directive for optional and non-optional fields to be the same - otherwise it will force us to think more at the stage of placing the directive in the query. Also, I would like the class with the meta to be one common for both optional fields and not. Perhaps the option for an optional value will be an inheritor of the class, or they will implement a common interface.

Then we will decide for which fields it is important for us to know the correct path to use it. And also immediately know the correct error on this field if there is.

in general, as a result, this will cover most of the cases and, perhaps, will allow us to get rid of using ValueProvider altogether in our project.

And most importantly, there will be no double generation - data will not be generated for all fields, but only for those explicitly specified.

ProVir commented 1 year ago

It would be really handful to have a more complete example with the usages and everything. If we ever get to implement this, we should add that as tests. I don't want to get your hopes too high though but having a complete example would help a ton crafting good APIs.

As an example, I can attach a file with the logic of mapping some queries.

Example Mappings.zip

Also it seems to me that the idea with @nonull(onError: THROW) also seems good for simple solutions (not for us). It can also be implemented in addition to the proposed solution.

martinbonnin commented 1 year ago

You can create a directive @meta (or with a different name) that generates additional code in the model api in the specified fields.

I see 👍 . Ideally we'll want to make that customizable. I.e. add APIs similar to Kotlin Hooks so that different user can customize the metadata to their needs. That's still a very long way though...

FieldWithMeta<T>.path: String - the path to this field

Do you still need the path if you have the error? I thought you were only needing it to lookup errors?

even if it is a synthetic field.

So for synthetic fields, what do you expect the path to be, the one of the field on which the fragment is?

{
  animal {
    ... on Lion { # data.animal.onLion synthetic field, "data.animal" path?
    }
  }
}

We are discussing the naming of the type and the directive, but we would like the directive for optional and non-optional fields to be the same - otherwise it will force us to think more at the stage of placing the directive in the query.

Agreed.

As an example, I can attach a file with the logic of mapping some queries.

Thanks, I'll look into this!

And most importantly, there will be no double generation - data will not be generated for all fields, but only for those explicitly specified.

👍

Also it seems to me that the idea with @nonull(onError: THROW) also seems good for simple solutions (not for us). It can also be implemented in addition to the proposed solution.

For the record, some prior art:

ProVir commented 1 year ago

Ideally we'll want to make that customizable. I.e. add APIs similar to Kotlin Hooks so that different user can customize the metadata to their needs. That's still a very long way though...

It seems that the path and error are generally sufficient basic meta, in this form, allowing you to configure a set of fields will not give almost any gain. Even if you leave only an error, you will need to know the correct path in this place to look for it, so I don't see much gain in hiding the path.

Do you still need the path if you have the error? I thought you were only needing it to lookup errors?

I have already mentioned that we need a path not only for errors from the graphql backend, but also for creating our own errors based on incoming data. The data came - and we, based on our business logic, considered them incorrect.

There may also be other logging cases (there are plans) that are not related to the return of errors and it will also be more convenient for us to pledge the correct path. When reusing fragment mapping, the final path is unknown and we want to know it in such places.

data.animal.onLion synthetic field, "data.animal" path?

Yes

ProVir commented 1 year ago

Hello!

Is there a chance that the proposal will get into release 4.0.0? At least for us it is very important.

martinbonnin commented 1 year ago

Hi 👋

Sorry I can't give estimates at this point. It's not being worked on actively by the Apollo team right now but if you or the community would like to help, it is very welcome.

There's a lot of discussion in this thread already so before someone starts working on this, I would suggest creating a document in design-docs summarizing the latest state of the proposal including directive definitions for the proposed directives (if any) and samples of GraphQL operations and matching Kotlin models. That will make sure everyone is aligned with the implementation.

martinbonnin commented 8 months ago

Hi @ProVir 👋 it's been a while!

If you're still around, I'm happy to share that there has been a lot of discussion in the GraphQL community over the last few months about nullability and error handling and we're starting to implement it in Apollo 🎉 And I think this might help for your use case.

You can read the current pull request here and associated documentation here.

The gist of it is to use @catch to model errors in the generated code as FieldResult<D> classes:

query {
  user @catch {
    email
  }
}

In Kotlin:

class Data(val user: FieldResult<User>)

sealed interface FieldResult<T> {
  class Success(val value: T): FieldResult<T>
  class Error(val error: Error): FieldResult<Nothing>
}

Would that help?

ProVir commented 8 months ago

Hi @ProVir 👋 it's been a while!

If you're still around, I'm happy to share that there has been a lot of discussion in the GraphQL community over the last few months about nullability and error handling and we're starting to implement it in Apollo 🎉 And I think this might help for your use case.

You can read the current pull request here and associated documentation here.

Would that help?

Hi @martinbonnin !

The solution looks super, but it's not quite suitable for us.

Often, it suits us if the field is null due to an error, we just ignore it. That is, we don't care if the field is null, due to a lack of data, or due to an error.

With error aware parsing, the errors are detected at parsing time, so you don't have to look them up in the errors array. Any GraphQL error stops the parsing and you are guaranteed that response.data is actual semantic data and does not contain any error.

By enabling this mode, we will greatly complicate our development.

We will have to explicitly ask all developers to set @catch on all fields, so that the whole request will not fail. And only for the fields that are requested without which we cannot work, do not set @catch (there are fewer such fields as the first ones).

We have a lot of reused fragments in which the attitude to errors is different depending on the query. We also have the concept of "SubResponse": part of the response might fail with an error, and this will be logged and send alert for monitorings, but the entire request won't be completed with error.

Which solution is convenient for us.

We work according to the recommendation of GraphQL, and, by default, any error is not considered as an error of the whole query - this way, it is convenient for us to do so.

Using @catch seems very convenient, but you could leave the current default behaviour: without @catch in case of an error, the entire request does not fail with an error, and the field comes null and the error is also in the error list.

That is, @catch just adds a wrapper so that you can get an error. But if there is none, things are as before.

@nullOnlyOnError

It looks convenient and logical. But there are worries in the case of modularity - when we write queries and work with types in some domain, and it is required to form a list of requirements for fields in the root module with base types.

Perhaps specifying in the queries themselves in @catch would just be more convenient and sufficient. My suggestion is to give both possibilities, both to indicate in the general list, and to explicitly indicate in @catch.

This is especially true for the specified fields without @catch - for them, according to our logic, null will come anyway.

martinbonnin commented 8 months ago

Thanks for the super quick feedback!

We will have to explicitly ask all developers to set @catch on all fields, so that the whole request will not fail.

There is a @ignoreErrors directive on operations that bypasses error aware parsing and @catch altogether. I just realized I forgot to add it to the docs, will do that in a bit.

We also have the concept of "SubResponse": part of the response might fail with an error

What is the SubResponse granularity? field? fragment? something else?

That is, @catch just adds a wrapper so that you can get an error. But if there is none, things are as before.

Actually I had that in a previous version of the PR where you could specify the default @catch for the whole schema. Let me think about this a bit more, I'll circle back here.

Perhaps specifying in the queries themselves in @catch would just be more convenient and sufficient.

I think this is what Client Controlled Nullability aimed for. Allowing to override the nullability of a field from the operation itself.

Ultimately, we realized that what was really missing was the ability to express "null but only for errors" or "true nullability" in the schema (see https://github.com/graphql/nullability-wg/discussions/22)

This is especially true for the specified fields without @catch - for them, according to our logic, null will come anyway

If there is a way to specify @catch(to: NULL) at the schema level, then @nullOnlyOnError would be overriden and the types would be generated as nullable anyways so I think that'd work?

martinbonnin commented 8 months ago

Actually I had that in a previous version of the PR where you could specify the default @catch for the whole schema. Let me think about this a bit more, I'll circle back here.

I remember now why I went away from schema-wide @catch. The issue was it did not allow migrating progressively to the fail by default mode.

Because fragments are shared between operations, there cannot really be a per-operation @catch(to:) because of possible conflicts, which is why I went with a runtime ignoreErrors flag. ignoreErrors can be set per-operation.

This is obviously not your use case so we'd need to distinguish between 2 kinds of users:

ProVir commented 8 months ago

What is the SubResponse granularity? field? fragment? something else?

This is solution in Kotlin code, this is kotlin.runCatching { }.getOrNull(), but additionaly logging all exception. Example:

query GetSomeData() {
    watch {
        ... watchMoviesFragment
    }
    favorite {
       ... favoriteMoviesFragment
    }
    note {
        ... noteMoviesFragment
    }
}

A simplified example, but true in meaning:

val watchModel = watch.runCatching {
    it.toModelOrThrow()
}.getOrNull()

....

return SuccessResult(watchModel, favoriteModel, noteModel)
ProVir commented 8 months ago

If there is a way to specify @catch(to: NULL) at the schema level, then @nullOnlyOnError would be overriden and the types would be generated as nullable anyways so I think that'd work?

I didn't understand the idea a bit. I mean, to give the opportunity in @catch to indicate that the field is null only in case of an error and in no other way - @catch(to: RESULT_NOT_NULL) as example.

It seems convenient to me to indicate the need to wrap the field in Result and the optionality field for Result.Success in one place at once.

This is obviously not your use case so we'd need to distinguish between 2 kinds of users:

We are users who want to "null by default".

We want to manage errors more from the backend and instead of an automatic error for the entire request to be able to process it at the point of occurrence, it is possible to additionally process it with our own logic.

There are also a variety of errors from the backend that we know about and we need to treat them differently and process them in the mapping of the request, depending on the domain where it is located.

Therefore, the generated Apollo models are only the first stage, after which we have a lot of our own mapping logic. And only in this logic we get the final business model, which is often not 1 in 1 as in the backend.