Skip to content
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

ErrorOr does not support null value and ignorable errors #2050

Closed
ntkoopman opened this issue Mar 4, 2024 · 10 comments
Closed

ErrorOr does not support null value and ignorable errors #2050

ntkoopman opened this issue Mar 4, 2024 · 10 comments

Comments

@ntkoopman
Copy link

I'm using the type-safe API and trying to use ErrorOr. In my case, the query can return a null value (if not found). Sometimes it also returns an error on some random (nested) field, which I would like to log.

When using ErrorOr I get an exception value must not be null, because ErrorOr uses requireNonNull. It also does not allow access to the value if there are any errors.

Since this class really assumes there is either a value or an error, I'm hesitant to patch it to allow null values. It would probably be better to create a new type that allows access to both without making assumptions.

@jmartisk
Copy link
Member

jmartisk commented Mar 4, 2024

When using ErrorOr I get an exception value must not be null, because ErrorOr uses requireNonNull

I think that receiving a null value without an error should be allowed, I'm not sure why we disallow it. @t1 wdyt?

It also does not allow access to the value if there are any errors.

This is correct I think because of how GraphQL works. If there's an error on that path, there can be no (non-null) value AFAIK.

@t1
Copy link
Collaborator

t1 commented Mar 4, 2024

I agree that we should patch ErrorOr: it's actually feasible that there is no error and no value (I hadn't been thinking about this before). But in GraphQL there can never be an error and a value.

@t1
Copy link
Collaborator

t1 commented Mar 4, 2024

... actually you should read it as "there is no error but the value null"

@ntkoopman
Copy link
Author

ntkoopman commented Mar 4, 2024

Yes, and since null is not a valid value for Optional/Stream it's a bit unclear on what the solution should be.

It's possible to have both a value and an error, especially in the case of field errors: https://spec.graphql.org/draft/#sel-HAPHRPNBDAAADdABjoF

{
  hero(episode: $episode) {
    name
    heroFriends: friends {
      id
      name
    }
  }
}
{
  "errors": [
    {
      "message": "Name for character with ID 1002 could not be fetched.",
      "locations": [{ "line": 6, "column": 7 }],
      "path": ["hero", "heroFriends", 1, "name"]
    }
  ],
  "data": {
    "hero": {
      "name": "R2-D2",
      "heroFriends": [
        {
          "id": "1000",
          "name": "Luke Skywalker"
        },
        {
          "id": "1002",
          "name": null
        },
        {
          "id": "1003",
          "name": "Leia Organa"
        }
      ]
    }
  }
}
@GraphQLClientApi
public interface StarWarsApi {
  ErrorOr<Hero> hero(@Name("episodeId") String episodeId);
}

In this case I would expect ErrorOr to give me like access to both data and errors. (Preferably not from a class with Or in the name though)

@t1
Copy link
Collaborator

t1 commented Mar 4, 2024

In your example, you should place your ErrorOr on the name field. You do expect to get heroes and their friends, but maybe not the name. If the error can happen everywhere, you might be better off by catching the exception.

@ntkoopman
Copy link
Author

But I don't want an exception, I want the partial response and the errors.

@t1
Copy link
Collaborator

t1 commented Mar 6, 2024

But I don't want an exception, I want the partial response and the errors.

You're right. That won't help in this case.

We've discussed about providing the partial result in the GraphQLClientException in #167, and split it out into #1885.

@t1
Copy link
Collaborator

t1 commented Mar 9, 2024

In this case I would like access to both data and errors. (Preferably not from a class with Or in the name though)

The ErrorOr javadoc states that this is theoretically possible in GraphQL, but I'm not sure if this supported, yet. And the name ErrorOr is okay, if you don't read it as an exclusive or 😁

t1 added a commit to t1/smallrye-graphql that referenced this issue Mar 9, 2024
@t1
Copy link
Collaborator

t1 commented Mar 9, 2024

I've created a PR, but I don't see the GitHub Action running. Is this new?

@t1
Copy link
Collaborator

t1 commented Mar 11, 2024

It builds when I create a correct PR 😳 sorry

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants