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

[GRAPH-1099] Allows config error to return a map #13

Merged
merged 2 commits into from
Sep 27, 2024

Conversation

bryanjos
Copy link
Contributor

@bryanjos bryanjos commented Sep 26, 2024

This PR makes it so that if config/2 returns an error tuple where the second element is a map, then that map is treated as the error response.

Subscription config errors currently expect a tuple of {:error, msg }. If message is an map, when returned to the user, it looks like:

"errors": [
    {"message": {"extensions": {"code": "BAD_REQUEST"}, "message": "Invalid argument"}}
]

This causes trouble for us as we return spec compliant errors and they get wrapped in the message.

This PR makes it so returning the following from config/2

{:error, %{message: "Invalid argument", extensions: %{code: "BAD_REQUEST"}}

Will return

"errors": [
    {"extensions": {"code": "BAD_REQUEST"}, "message": "Invalid argument"}
]

@bryanjos bryanjos requested review from cschiewek, michaelcaterisano and GeoffreyPS and removed request for cschiewek and michaelcaterisano September 26, 2024 16:06
Copy link
Contributor

@michaelcaterisano michaelcaterisano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this api works, but I was hoping we could make it match a resolver's api, which supports returning either {:error, message} or {:error, spec_compliant_error_map}, i.e.
{:error, %{message: message, extensions: %{code: bad_request_code(), retry: false}}}

Was there a reason you decided against that api?

@bryanjos
Copy link
Contributor Author

Yes because of what I explained about the spec_compliant_errors option. I figure it's the best way to still allow for the result phase to work how it does now with the least amount of unexpected consequences. I can do it differently and make changes in the result phase, but that feels weird to me.

And I figured it's the best way to get it into absinthe. I'm making an issue now and can change things depending on the feedback

@michaelcaterisano
Copy link
Contributor

michaelcaterisano commented Sep 26, 2024

Ah I see Maarten's PR that added this, addressing this issue: absinthe-graphql/absinthe#925

Personally I think it's more straightforward to just return a map in the correct shape rather than have Absinthe format the errors for you. That's what we do now. IMO, the issue above isn't actually an issue, just a misunderstanding.

That said, if Absinthe is going to move forward with Maarten's fix (it's opt-in now, and to my knowledge we don't use it), then we should make sure it's well documented too.

@bryanjos
Copy link
Contributor Author

Yeah I guess knowing the history of it now, it seems like it would make more sense to allow it as is

@bryanjos bryanjos changed the title [GRAPH-1099] Allows a 3-tuple to be return for config errors [GRAPH-1099] Allows config error to return a map Sep 27, 2024
@bryanjos bryanjos merged commit 6881fa5 into main Sep 27, 2024
10 checks passed
@bryanjos bryanjos deleted the bryanj/GRAPH-1099/allow_extras_to_be_defined branch September 27, 2024 16:44
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

Successfully merging this pull request may close these issues.

3 participants