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

Fix Improper 406 Responses #1540

Merged
merged 1 commit into from Jul 24, 2018

Conversation

Projects
None yet
2 participants
@StevenXL
Copy link
Member

StevenXL commented Jul 13, 2018

Before submitting your PR, check that you've:

  • Bumped the version number

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)
@StevenXL

This comment has been minimized.

Copy link
Member Author

StevenXL commented Jul 13, 2018

Folks,

I wanted to present this PR as a discussion on fixing the issue with the defaultErrorHandler function identified by #1402.

There is a lot to clean up in the code. For example, if we keep selectRepE, then selectRep can be implemented in terms of selectRepE, and I'd also style the code so that it is more in keeping with conventions - specifically, no code going all the way to column 158.

However, is the general solution to the problem adequate? The original problem that the tests surface is that we have an error handler (defaultErrorHandler) that can itself fail, because selectRep can fail. The way I see it, the problem is that the error handling behavior ofselectRep is baked in. The new function selectRepE is selectRep, but with explicit failure so that the client of selectRepE can decide what to do on failure. Then, each defaultErrorHandler uses selectRepE, and if selectRepE failed, we don't care why it failed, we just "raise the original error", to speak in loose terms.

If this gets a thumbs up I can clean up this code and, hopefully, get all the other tests to pass.

P.S. I forgot to mention that this only fixes the caseDviInvalidArgs test, but shows the general pattern I'm going for.

@StevenXL StevenXL force-pushed the StevenXL:error-406 branch from e40847e to 5ceafb5 Jul 14, 2018

@snoyberg

This comment has been minimized.

Copy link
Member

snoyberg commented Jul 15, 2018

Playing devil's advocate: what about instead including a fall-back representation for when there is no matching content type? This could be handled exclusively in the errorHandler method without other changes.

@StevenXL StevenXL force-pushed the StevenXL:error-406 branch from 5ceafb5 to f2db587 Jul 15, 2018

@StevenXL StevenXL changed the title WIP - No Merge - Fix Improper 406 Responses Fix Improper 406 Responses Jul 15, 2018

@StevenXL

This comment has been minimized.

Copy link
Member Author

StevenXL commented Jul 15, 2018

@snoyberg Please take a look at the current solution.

It seems to me that the problem is that selectRep does not select the first provided representation if no representations match. That's a necessary but insufficient condition - the Content-Type header of the request must also be empty.

It seems to me that this behavior was added in this commit.

The reason I felt comfortable changing that work is:

  1. The relevant RFC seems to allow the server to choose a default representation, so we're not violating the standard as I interpret it.
  2. Based on your first comment - paraphrasing as "add a default representation" - it seems to me that you also thought that the selectRep function would choose a representation as long as we provided one.

Tagging @gregwebs to get his thoughts on this as well.

Happy to take directions on this from the team, but this seemed like a correct, clean change.

@snoyberg

This comment has been minimized.

Copy link
Member

snoyberg commented Jul 16, 2018

I'm OK with heading in this direction. It seems like perhaps we should have a separate function like selectRepNoDefault (bad name) that provides the current behavior, but personally I've never had that need. And it seems like a client can always simply check the content-type header and decide it doesn't like what's been delivered.

But a change like this certainly needs to be called out clearly in the ChangeLog.

`selectRep` chooses first rep if no matches found.
The `selectRep` documentation indicates that it choose the first
representation provided if no representation matches.

This was only partially correct, as `selectRep` required that no
representation matched **and** that the `Content-Type` header of the
response was empty.

This led to a problem because `defaultErrorhandler` relies on
`selectRep`, and when `selectRep` was unable to find a suitable
representation, it would "swallow" the original error that resulted in
`defaultErrorhandler` being called, and set a status 406 for all cases.

@StevenXL StevenXL force-pushed the StevenXL:error-406 branch from f2db587 to 266c436 Jul 20, 2018

@StevenXL

This comment has been minimized.

Copy link
Member Author

StevenXL commented Jul 20, 2018

@snoyberg take a look now please. Feedback welcomed.

@snoyberg
Copy link
Member

snoyberg left a comment

LGTM, merge when ready 👍

@StevenXL StevenXL merged commit db1ff95 into yesodweb:master Jul 24, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@StevenXL StevenXL deleted the StevenXL:error-406 branch Jul 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.