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

How do you set metadata on HttpError? #644

Closed
adamchalmers opened this issue Apr 13, 2023 · 4 comments
Closed

How do you set metadata on HttpError? #644

adamchalmers opened this issue Apr 13, 2023 · 4 comments

Comments

@adamchalmers
Copy link
Contributor

adamchalmers commented Apr 13, 2023

Hi! The docs for HttpError say it includes "optionally: additional metadata describing the issue". But I don't see any method to get/set metadata, nor is there a field for it.

The HttpError docs should describe how this feature works and provide an example. Putting structured data into your errors, for clients to consume, is pretty important to a lot of servers :) Happy to write the docs if y'all can point me to how this works.

@davepacheco
Copy link
Collaborator

Sorry. It looks like that quote is from the very early days and was probably more aspirational and directional than real. I don't think there's currently a way to do this. It hasn't been a big pain point for us yet (which is not to say it's not important -- I totally get the motivation).

See also #41, #39. These are related, but not the same thing. One of those solutions might help here. Another thought is that instead of returning HttpError, we could allow consumers to return something that impls a trait that allows us to extract a status code and JSON schema (based solely on the type) and an actual response body (based on the value), similar to HttpCodedResponse. We'd have to think about this.

@ahl thoughts?

@adamchalmers
Copy link
Contributor Author

adamchalmers commented Apr 13, 2023

No worries! In KittyCAD we're building a lot of stuff on Dropshot, so I'd definitely like to make that feature more than aspirational and actually implement it :) I'm happy to open a PR if we can get consensus on a good direction.

The two mutually-exclusive approaches I can see easily are:

  1. Your solution: generalize the HttpResponse trait so it's generic over E, which defaults to E = HttpError (for backwards-compatibility, and as a reasonable default). E is bounded to that trait you describe (get status code, JSON schema, serialize)
  2. Make HttpError accept a metadata field. It could either be serde_json::Value or could be generic over M: Serialize, default to () and get serialized to JSON by Dropshot.

@epompeii
Copy link
Contributor

I third your idea @davepacheco (@adamchalmers 's number 1).
It would be great to also include a headers method in that trait so that customers headers can be added to error messages. A default impl could also be provided that simply returns None or an empty map, etc.

This would solve #801 for me and make it possible to display these more informative error messages in a web UI, satisfying CORS.

@davepacheco
Copy link
Collaborator

Closing as a dup of #39.

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