Skip to content
This repository was archived by the owner on Aug 18, 2020. It is now read-only.

[CO-417] Add more friendly 415 Unsupported Media Type error #3727

Merged
merged 1 commit into from
Oct 11, 2018

Conversation

paweljakubas
Copy link
Contributor

@paweljakubas paweljakubas commented Oct 8, 2018

Description

Implement more friendly 415 Unsupported Media Type error.

BEFORE

$ curl -X POST -vk -H "Content-Type: patate" http://localhost:8090/api/v1/wallets 

> POST /api/v1/wallets HTTP/1.1
> Host: localhost:8090
> User-Agent: curl/7.60.0
> Accept: */*
> Content-Type: patate
> 
< HTTP/1.1 415 Unsupported Media Type
< Transfer-Encoding: chunked
< Date: Fri, 05 Oct 2018 08:57:20 GMT
< Server: Warp/3.2.22
< 
* Connection #0 to host localhost left intact

AFTER

$ curl -X POST -vk -H "Content-Type: patate" http://localhost:8090/api/v1/wallets

> POST /api/v1/wallets HTTP/1.1
> Host: localhost:8090
> User-Agent: curl/7.60.0
> Accept: */*
> Content-Type: patate
> 
< HTTP/1.1 415 Unsupported Media Type
< Transfer-Encoding: chunked
< Date: Mon, 08 Oct 2018 13:14:42 GMT
< Server: Warp/3.2.22
< Content-Type: application/json
< 
* Connection #0 to host localhost left intact
{"status":"error","diagnostic":{"mimeContentTypeError":"The API expects the Content-Type's main MIME-type to be 'application/json'"},"message":"UnsupportedMimeTypePresent"}

Linked issue

https://iohk.myjetbrains.com/youtrack/issue/CO-417

Type of change

  • 🐞 Bug fix (non-breaking change which fixes an issue)
  • 🛠 New feature (non-breaking change which adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
  • 🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
  • 🔨 New or improved tests for existing code
  • ⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

  • I have read the style guide document, and my code follows the code style of this project.
  • If my code deals with exceptions, it follows the guidelines.
  • I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.
  • CHANGELOG entry has been added and is linked to the correct PR on GitHub.

Testing checklist

  • I have added tests to cover my changes.
  • All new and existing tests passed.

QA Steps

  1. Run cluster and
curl -X POST -vk -H "Content-Type: patate" http://localhost:8090/api/v1/wallets

Screenshots (if available)

@paweljakubas paweljakubas requested a review from KtorZ October 8, 2018 17:28
@paweljakubas paweljakubas force-pushed the paweljakubas/CO-417/fix-unsupported-media-type branch from 35fee0c to 0dddf95 Compare October 8, 2018 17:57
@paweljakubas
Copy link
Contributor Author

continuous-integration/appveyor/pr — AppVeyor build failed because of timeout

Copy link
Contributor

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

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

Sorry for the late review!

, "diagnostic" .= diagnostic
, "message" .= msg
]
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's have it as a middleware instead, don't you think? And, let's re-use code actually serializing jsend errors toJSON

unsupportedMimeTypeMiddleware :: Middleware
unsupportedMimeTypeMiddleware = ifResponse ((== status415) . responseStatus) $ responseBuilder status415
    [ ("Content-Type", "application/json") ]
    (fromByteString . toStrict . encode UnsupportedMimeTypeError

Something along those lines, with UnsupportedMimeTypeError defined somewhere as a plain type, like other errors, not forgetting the example documentation in Swagger.hs ❤️

@paweljakubas paweljakubas force-pushed the paweljakubas/CO-417/fix-unsupported-media-type branch 2 times, most recently from da86a5a to 2d86d1c Compare October 10, 2018 20:59
@paweljakubas
Copy link
Contributor Author

paweljakubas commented Oct 10, 2018

Made changes as requested:

  1. Introduced UnsupportedMimeTypeError
  2. Encapsulated the stuff inside middleware
  3. Added golden test, swagger entry and corresponding aesonRoundtripProp

Now it outputs:

{"status":"error","diagnostic":{"mimeContentTypeError":"The API expects the Content-Type's main MIME-type to be 'application/json'"},"message":"UnsupportedMimeTypePresent"}

Code rebased, and squashed

@paweljakubas
Copy link
Contributor Author

Both AppVeyor failures are due to timeouts

Copy link
Contributor

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

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

A minor comment, but looks good now 👍

| otherwise = r

unsupportedMimeTypeMiddleware :: Middleware
unsupportedMimeTypeMiddleware = modifyResponse responseModifier
Copy link
Contributor

Choose a reason for hiding this comment

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

Since responseModifier is solely related to the above function, it would make sense to have it under a where clause. It doesn't make sense to use responseModifier as it is in any other context 👍

@KtorZ
Copy link
Contributor

KtorZ commented Oct 11, 2018

May you add an entry in the CHANGELOG, explaining that we now have more developer-friendly errors for wrong content-type ?

Incidentally, that is a breaking change 🙃 ...


instance Buildable UnsupportedMimeTypeError where
build _ =
bprint "The API expects the Content-Type's main MIME-type to be 'application/json'"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should display what get passed as Text to the UnsupportedMimeTypePresent constructor ? Don't you think?

golden_UnsupportedMimeTypeError_UnsupportedMimeTypePresent =
goldenTestJSON
(UnsupportedMimeTypePresent "test")
"test/golden/UnsupportedMimeTypeError_UnsupportedMimeTypePresent"
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@paweljakubas paweljakubas force-pushed the paweljakubas/CO-417/fix-unsupported-media-type branch from 2d86d1c to f71f0b4 Compare October 11, 2018 10:41
[CO-417] stack2nix patch

[CO-417] Introducing UnsupportedMimeTypeError and delegating code to middleware

[CO-417] Small improvements and CHANGELOG entry
@paweljakubas
Copy link
Contributor Author

paweljakubas commented Oct 11, 2018

Changes:

  1. Added entry to CHANGELOG.md
  2. Refactored instance Buildable UnsupportedMimeTypeError
  3. Moved responseModifier into where

Commits squashed, was up to date against develop

Copy link
Contributor

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@KtorZ KtorZ merged commit 70edf77 into develop Oct 11, 2018
@KtorZ KtorZ deleted the paweljakubas/CO-417/fix-unsupported-media-type branch October 11, 2018 11:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants