-
Notifications
You must be signed in to change notification settings - Fork 660
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ErrorPath meta in data fields #4733
Comments
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). |
There are a few ideas. In addition to the main (current) generation approach, to provide a additional wrappers (value providers). Or another option is to provide the correct paths for the fields in some way nearby. 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. |
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
Out of curiosity, is the issue that 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
}
|
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. 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). |
I see. Thanks for the details! 🙏
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:
That gives you a nice type safe error explaining the reason you would not be able to get a given (follow up comment incoming with additional more complex client side brainstorming...) |
Potential client-side only solutions1. Every field is now wrapped under some kind of
|
This is one of the methods that we already use for other cases.
It seems to be a closer option to us, but it seems cumbersome to use
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:
Use:
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.
This is just an example, the names and approach are discussed. |
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 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 fieldsGoing 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 |
Alas, we cannot influence this, the backend has long fixed such error handling as correct - the error is part of the response.
Overall it looks good, but:
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. 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. 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. 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? 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 } |
Thanks for the feedback. Comments below Build a separate path models hierarchy
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.
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)
Not sure what you mean. Your current 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 }
)
Can you ellaborate a bit more? I don't think you'd need 2 generics?
Yea, that's the part that gets me quite worried... Make the parser throw on null fields, proposal 2
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 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)
}
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.
So if you're trying to read, say |
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<SomeQuery.Data> 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. 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. 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. 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 commentsConsider 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. 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
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
That's right, I described above that the model shown in the example is not the generated apollo, but the project business model.
By the way, within the issue of this, I would have thought of a directive that would:
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.
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.
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. 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. |
Thanks for writing this! Comments inline:
I see, thanks for clarifying this 👍
At some point we'll have to clarify what a "scope“ is. In your example, I think
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 🙂 .
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.
Apollo-specific client directives are stripped before being sent to the server. This is done here.
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? |
Great!
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. 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. Most likely, it will be an issue format in github with a feature suggestion like this one.
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. ProposalThank you for your time and discussion, it seems that I started to have ideas to solve the problem after our discussions. You can create a directive 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:
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. Then we will decide for which fields it is important for us to know the correct path to use it. 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. |
As an example, I can attach a file with the logic of mapping some queries. Also it seems to me that the idea with |
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...
Do you still need the path if you have the error? I thought you were only needing it to lookup errors?
So for synthetic fields, what do you expect the path to be, the one of the field on which the fragment is?
Agreed.
Thanks, I'll look into this!
👍
For the record, some prior art: |
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.
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.
Yes |
Hello! Is there a chance that the proposal will get into release 4.0.0? |
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. |
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 🎉 You can read the current pull request here and associated documentation here. The gist of it is to use 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? |
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.
By enabling this mode, we will greatly complicate our development. We will have to explicitly ask all developers to set We have a lot of reused fragments in which the attitude to errors is different depending on the query. 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 That is, @nullOnlyOnErrorIt looks convenient and logical. Perhaps specifying in the queries themselves in This is especially true for the specified fields without |
Thanks for the super quick feedback!
There is a
What is the SubResponse granularity? field? fragment? something else?
Actually I had that in a previous version of the PR where you could specify the default
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 graphql/nullability-wg#22)
If there is a way to specify |
I remember now why I went away from schema-wide Because fragments are shared between operations, there cannot really be a per-operation This is obviously not your use case so we'd need to distinguish between 2 kinds of users:
|
This is solution in Kotlin code, this is 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) |
I didn't understand the idea a bit. I mean, to give the opportunity in It seems convenient to me to indicate the need to wrap the field in
We are 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. |
Hi @ProVir 👋 Apologies for the delay. With v4 out, I'm hoping we can get to the bottom of this issue. As a preamble, there is now a dedicated documentation page about extend schema @catchByDefault(to: NULL) I think it solves the initial case. I've taken your initial code and added tests here: // field error with GraphQLResponseProvider:
assertFailsWith<MyException> {
val duration =
responseProvider.valueOrThrow("user.content.0.duration") { user?.content?.get(0)?.movieFragment?.duration }
}
// field error with @catch
assertFailsWith<DefaultApolloException> {
val duration =
response.data?.user?.content?.get(0)?.movieFragment?.duration?.getOrThrow()
} It's not 1:1 the same exception thrown but I'm assuming it should be reasonnably doable to map to another exception if needed. It sounded like your use cases were more complex though. Can you update the tests with more use cases and I'll look into how to translate them. Thanks! |
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 fielddata.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
andGraphQLValueProvider
.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.
In code:
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.
The text was updated successfully, but these errors were encountered: