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

Addressing a problem with 404 errors for EDN requests #429

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

quoll
Copy link

@quoll quoll commented Feb 8, 2018

Regardless of the requested content type, if an error occurs (such as 404) then the server will often return HTML or plain text to indicate the error. If EDN is requested and status code exceptions turned off, then the returned data is parsed as EDN, leading to a parse exception, and the status is lost.

This patch allows successful EDN requests to go through standard parsing with no change. Unsuccessful requests will attempt to parse EDN (just in case the server really is trying to return a data structure... something I've yet to encounter), but if an error is encountered, it returns the body as raw text.

@danielcompton
Copy link

Is this how other HTTP clients handle this kind of error? It seems like a bug in a server if it doesn't return the correct content-type. There's certainly a good argument that errors should be caught and thrown in such a way that the developer can recover the response, but I'm not so sure about just returning a plain string when the developer has asked for and is expecting EDN.

@quoll
Copy link
Author

quoll commented Feb 12, 2018

It's only a bug in the server if the status code is in the 2xx range. If it's in the 4xx range then the body is undefined. It isn't nice to get a content-type with a 404 though.

Thinking on it further, returning the body as a string was incorrect here, since it could be anything. I'm seeing an HTML page saying "Not found" which is why I handled it as a string, but there are no guarantees, so I realize that this was not correct. Returning the byte array would better, allowing the user to work with whatever their server is giving them.

@danielcompton
Copy link

I think we were talking about different content-types: you were talking about the one that the client sends, and I was talking about the one returned by the server?

The issue seems to be that if you request an EDN content type, but the server returns an error with the content type of text/plain or HTML then clj-http still tries to parse it as Clojure? Is that the case?

@dakrone
Copy link
Owner

dakrone commented Feb 13, 2018

Hmm... I think rather than the "try and then fall back" approach, I'd rather have a boolean value that allows you to specify whether to decode only on success, or all the time. Then it can be used for all the coercions (not just the EDN one).

What do you think @quoll & @danielcompton ?

@danielcompton
Copy link

danielcompton commented Feb 13, 2018

@quoll I’m still not clear on the exact flow that is leading to your error, can you post the headers and body of the HTTP request and response?

@dakrone
Copy link
Owner

dakrone commented Feb 27, 2018

@quoll ping again on this, any thoughts on my proposed solution?

@dakrone
Copy link
Owner

dakrone commented Apr 11, 2018

@quoll ping again, had a chance to take a look at this?

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

Successfully merging this pull request may close these issues.

3 participants