handle-malformed raises exception when returning a map

[JuneKelly]

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 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"}}))

[JuneKelly]

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]

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]

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]

Any news on this?

[ricardojmendez]

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]

@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]

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]

@ShaneKilkelly thanks for the workaround!

[mveytsman]

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]

@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]

@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]

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 [email protected] 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]

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]

@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]

@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 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]

@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]

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."}`