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

handle-malformed raises exception when returning a map #94

Open
ShaneKilkelly opened this issue Dec 19, 2013 · 19 comments
Open

handle-malformed raises exception when returning a map #94

ShaneKilkelly opened this issue Dec 19, 2013 · 19 comments
Labels
bug

Comments

@ShaneKilkelly
Copy link

@ShaneKilkelly ShaneKilkelly commented Dec 19, 2013

When I try to return a map (json) from handle-malformed, the server crashes with the following exception:

>> java.lang.IllegalArgumentException:
>> No method in multimethod 'render-map-generic' for dispatch value: null
>> MultiFn.java:160 clojure.lang.MultiFn.getFn
>> MultiFn.java:231 clojure.lang.MultiFn.invoke
>> representation.clj:210 liberator.representation/fn
>> representation.clj:23 liberator.representation/fn[fn]
>>                        core.clj:176 liberator.core/run-handler
>> core.clj:485 liberator.core/handle-malformed
>> core.clj:93 liberator.core/decide
...

The following code is an example:

(defresource something
...
  :malformed?
  (fn [context]
    (let [params (get-in context [:request :params])]
      (empty? (params :imageData))))

  :handle-malformed
  (fn [_]
    {:error "Error: imageData required"})
...

If the handle-malformed function is replaced with a string, the code works and a plain text string is returned to the client, however this is not a workable solution as the api needs to consistently deal with json rather than sometimes returning plain text. :)

Seeing as other handle-xyz branches can return map data, I presume that handle-malformed should be able to do the same?

Tested on Ubuntu 13.10 , liberator 0.10.0 .

@ordnungswidrig

This comment has been minimized.

Copy link
Member

@ordnungswidrig ordnungswidrig commented Dec 20, 2013

This is a nasty gap in liberator. The context will not have a negotiated media type available until the decision point media-type-available? is executed. But there is a simple workaround: assoc the desired media-type in the context:

(defresource someting
:malformed? (fn[ctx](when-not %28get-in ctx [:request :params :imageDate]%29 {:representation {:media-type "application/json"}}))

@ShaneKilkelly

This comment has been minimized.

Copy link
Author

@ShaneKilkelly ShaneKilkelly commented Dec 20, 2013

Thanks for that.

However, is there a way to do this assoc while still returning a boolean from :malformed? ?
I've tried the code you gave as an example and it always evaluates :malformed? to true, (i guess because a map is a truthy value). It's possible that I'm just using the example wrong though.

EDIT: I figured it out, If I return a vector, the first entry being the boolean result of the decision and the second entry being the :representation map, then it all appears to work. My code as it stands now:

  :malformed?
  (fn [context]
    (let [params (get-in context [:request :params])]
      [(empty? (params :imageData))
       {:representation {:media-type "application/json"}}]))

  :handle-malformed
  (fn [_]
    {:error "imageData required"})

That's a pretty neat way of handling things, but not obvious until I tried it by chance. Thanks for the help :)

@graue

This comment has been minimized.

Copy link
Contributor

@graue graue commented Jan 24, 2014

I just hit this, and it applies to handle-forbidden as well. Trying to make my API behave nicely and return JSON error responses. Glad to hear a fix is in the works.

@conan

This comment has been minimized.

Copy link

@conan conan commented Aug 21, 2014

I'm having to include the :representation key in all my :malformed? decisions too, it'd be nice to have a cleaner way of expressing this.

@lmatoso

This comment has been minimized.

Copy link

@lmatoso lmatoso commented Sep 24, 2014

Any news on this?

@ricardojmendez

This comment has been minimized.

Copy link

@ricardojmendez ricardojmendez commented Oct 26, 2014

I figured it out, If I return a vector, the first entry being the boolean result of the decision and the second entry being the :representation map, then it all appears to work.

Is this supposed to be the expected behavior? Or should we expect this to be fixed in the near future?

@graue

This comment has been minimized.

Copy link
Contributor

@graue graue commented Oct 26, 2014

@ricardojmendez: It's always legal to provide a return value of this form, so you don't have to worry about this workaround being broken by a fix, if that's what you're asking. (The fix would be to negotiate an appropriate content-type for you in this case, making it optional to specify one, but still allowed.)

@ricardojmendez

This comment has been minimized.

Copy link

@ricardojmendez ricardojmendez commented Oct 26, 2014

Thanks @graue, I was indeed wondering if a future release would end up breaking this workaround. I can live with returning the content-type.

@ghedamat

This comment has been minimized.

Copy link

@ghedamat ghedamat commented Feb 9, 2015

@ShaneKilkelly thanks for the workaround!

@mveytsman

This comment has been minimized.

Copy link

@mveytsman mveytsman commented Feb 21, 2015

I found a slightly more "elegant" fix.

Since I know my API will always return JSON, I can add the media type to :service-available? which is the first node on the decision graph, i.e.

(def base-resource 
  {:service-available? {:representation {:media-type "application/json"}
   :handle-malformed {:error "malformed"}
   :available-media-types ["application/json"]}

(defresource myresource
  baseresource
  :handle-ok {:status "OK"})

I'm not shooting myself in the face by doing this, right?

@xpe

This comment has been minimized.

Copy link

@xpe xpe commented Mar 7, 2015

@mveytsman Work-arounds are, by definition, sub-optimal. :) Some thoughts:

  • It seems to me that whatever you do in :service-available? should only semantically impact the part of your API directly having to do with service availability. Merging something into the context is quite non-intuitive. 503 Service Unavailable means (per Wikipedia): "The server is currently unavailable (because it is overloaded or down for maintenance). Generally, this is a temporary state."
  • I would have less of a concern if you attached the :representation to the initial context if you could do so in a clear fashion, such as with an initial value. I don't know if that is possible (yet).
  • But relying on the knowledge that :service-available comes first and then merging :representation into the context seems brittle, coupled, and semantically confusing.

So, yes, I think you are shooting yourself in the foot, as well as anyone who reads the code. But I can't blame you for exploring work-arounds.

@ordnungswidrig

This comment has been minimized.

Copy link
Member

@ordnungswidrig ordnungswidrig commented Mar 9, 2015

@xpe you have good points here. Besides the need for a good solution for the overal problem, I was tinkering with the idea of having an :initial-context callback. That could be used to setup defaults like a default representation in this case. I, personally, found myself abusing the:service-available? decision for such hacks.

What do you think?

@mveytsman

This comment has been minimized.

Copy link

@mveytsman mveytsman commented Mar 9, 2015

I think that's a great idea!

@ordnungswidrig I take it you would accept a PR to that effect, or are these kinds of changes something you don't want to leave to contributors?

Max

On Mar 9, 2015, at 7:06 AM, Philipp Meier notifications@github.com wrote:

@xpe you have good points here. Besides the need for a good solution for the overal problem, I was tinkering with the idea of having an :initial-context callback. That could be used to setup defaults like a default representation in this case. I, personally, found myself abusing the:service-available? decision for such hacks.

What do you think?


Reply to this email directly or view it on GitHub.

@zamaterian

This comment has been minimized.

Copy link

@zamaterian zamaterian commented Jun 11, 2015

Would liberator.conneg/best-allowed-content-type be considered fairly stable to use as a workaround to set a representation in service-available (or (liberator.conneg/best-allowed-content-type ACCEPT-HEADER ["application/json" "application/transit+json;verbose"] ) "application/json" ) ?

Cheers
Thomas

@ordnungswidrig

This comment has been minimized.

Copy link
Member

@ordnungswidrig ordnungswidrig commented Jun 12, 2015

@zamaterian yes that's a possible workaround. I'm not sure, however, if the Vary header is set correctly if done naively. I was looking into something like that while working adding conneg support for "error" status codes but's it's not trivial if you want to support all the negotiable parameters.

@ordnungswidrig

This comment has been minimized.

Copy link
Member

@ordnungswidrig ordnungswidrig commented Jun 12, 2015

@zamaterian you can take the code in negotiate-media-type and adjust for your needs in service-available?:

(defn negotiate-media-type [context]
  (try-header "Accept"
              (when-let [type (conneg/best-allowed-content-type 
                               (get-in context [:request :headers "accept"]) 
                               ((get-in context [:resource :available-media-types] (constantly "text/html")) context))]
                {:representation {:media-type (conneg/stringify type)}})))

Liberator adds the Vary header according to the values at the key :representation in the context.

@wesleyhall

This comment has been minimized.

Copy link

@wesleyhall wesleyhall commented Jun 18, 2015

This is a pretty nasty bug, in an otherwise fantastic library.

It seems to me that 'malformed?' simply comes too early in the decision graph.

The malformed decision handler encourages the user to start poking around in the request body before the client and server have finished negotiating headers.

Would it not make more sense for the server to tell the client either, "I do not know how to handle that incoming content type", or "I cannot produce any of these output types" before it starts inspecting the incoming body to see if it is a wellformed request?!

I am currently using 'wrap-json-body' in my middleware which, sensibly, only touches the body if the content-type is application/json.

However, because of this strange ordering in the decision graph, this means that my malformed handler has to be prepared to receive either a clojure map (when content-type is application/json) or a plain input stream (when it's not). I don't understand why this needs to be the case. Liberator should reject the latter requests upstream of my malformed decision function right?

@ordnungswidrig

This comment has been minimized.

Copy link
Member

@ordnungswidrig ordnungswidrig commented Jun 23, 2015

@wesleyhall http status 400 Bad Request indicates a request that is malformed on the HTTP syntax level. You won't encounter them in ring too often because the server adapter and ring were able to process the request and thus the request cannot be "too malformed".

If you want to indicate that the request body ("entity" in http speak) is malformed you better use 422 unprocessable entity. The drawback is that that status code is not in the http spec (RFC 7231) but from WebDAV (RFC 4918).

This explains why the malformed? decision come early in the graph. On the other hand, processable? come after content negotiation and should address your problem. If you absolutely need to return status code 400, you can use ring-response in handle-unprocessable-entity:

(defresource foo ;; untested
  :processable? (fn [_] {::msg "invalid json body"})
  :handle-unprocessable-entity (fn [{msg ::msg}] (ring-response {:status 400} msg)))
@danielsz

This comment has been minimized.

Copy link

@danielsz danielsz commented Oct 11, 2016

I agree with @xpe about not hijacking the :service-available? decision point, and more generally, with the principle to respect the semantics of an API and not hack it to bypass shortcomings.
I'm writing this in fact to share the solution I have adopted. For all decision points prior to media-type-available?, that is to say before a negotiated media type is available, I return a vector.
As per the docs, "If the value is a vector, then the first element is used as the boolean decision and the second element is used to update the context."
So, for example:

:allowed? [true/false {:representation {:media-type "application/edn"}}]
:handle-forbidden {:message "Not allowed."}`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.