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

Support Error marshalling using application/problem+json content type #134

Closed
dahankzter opened this issue Mar 5, 2018 · 14 comments
Closed
Assignees

Comments

@dahankzter
Copy link

I just found out that Mailchimp uses this header which is unfortunate and unusual afaik but there is a spec: https://tools.ietf.org/html/rfc7807 that seems to define this.

It would be nice to be able to use the SetError() functionality.

@jeevatkm
Copy link
Member

jeevatkm commented Mar 5, 2018

@dahankzter I have read the Spec. It seems resty needs knowledge to understand it.

Let me add a support, once its ready I will ping you.

@jeevatkm jeevatkm self-assigned this Mar 5, 2018
@jeevatkm jeevatkm added this to the v1.4 Milestone milestone Mar 5, 2018
@jeevatkm
Copy link
Member

jeevatkm commented Mar 6, 2018

@dahankzter Just thought to clarify. I'm planning to add support for application/problem+json and application/problem+xml per RFC7807

However resty only considers response status code > 399 as an error.

@jeevatkm
Copy link
Member

jeevatkm commented Mar 6, 2018

@dahankzter I have implemented it, available on branch add-support-for-rfc7807

Please give it try and let me know.

@dahankzter
Copy link
Author

I will test it as soon as possible. Having some dep problems now that go.uber.org is gone.

@jeevatkm
Copy link
Member

jeevatkm commented Mar 6, 2018

No problem, take your time. What happened to go.uber.org?

@dahankzter
Copy link
Author

Some DNS issue over at Cloudflare is the current bet.

@dahankzter
Copy link
Author

The regex doesn't' seem to match application/problem+json afaik. I tried here https://regex101.com/ as well a little test.

Your test case seems to only assert that the Error is set which it is but not actually parsed and used but maybe I am wrong?

@dahankzter
Copy link
Author

Making the "problem+" a group seems to work (?i:[application|text]/(problem\+)?json)

@jeevatkm
Copy link
Member

jeevatkm commented Mar 6, 2018

@dahankzter Thank you, I have updated the regex and test case. Now it should be good.

@dahankzter
Copy link
Author

Excellent! What is your release policy? When can this go out? I use it in a real project and we decided to avoid specific commits and go with proper tags as given by the respective dependency.

@jeevatkm
Copy link
Member

jeevatkm commented Mar 7, 2018

@dahankzter Typically bug fixes gets immediately released. Feature and enhancements are monthly.

jeevatkm added a commit that referenced this issue Mar 7, 2018
@dahankzter
Copy link
Author

Ok great, I look forward to the next release! :)

@jeevatkm
Copy link
Member

jeevatkm commented Mar 8, 2018

Merged to master 😄

@jeevatkm jeevatkm closed this as completed Mar 8, 2018
@jeevatkm
Copy link
Member

jeevatkm commented Apr 5, 2018

@dahankzter FYI resty v1.4 released.

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

No branches or pull requests

2 participants