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

pedestal-service's default responses are now served as application/octet-stream #89

Closed
avescodes opened this issue Jun 18, 2013 · 5 comments
Assignees
Milestone

Comments

@avescodes
Copy link

Previously, it was the case that running lein new pedestal-service helloworld and making src/helloworld/service.clj match the following would result in the / route serving "Hello world" as normal browser-friendly response.

(ns helloworld.service
    (:require [io.pedestal.service.http :as bootstrap]
              [io.pedestal.service.http.route.definition :refer [defroutes]]
              [ring.util.response :refer [response]]))

(defn home-page
  [request]
  (response "Hello World!"))

(defroutes routes
  [[["/" {:get home-page}]]])

;; Consumed by helloworld.server/create-server
(def service {:env :prod
              ::bootstrap/routes routes
              ::bootstrap/type :jetty
              ::bootstrap/port 8080})

Now, without the addition of an html-body interceptor as below the / route is served as application/octet-stream.

(defroutes routes
 [[["/" {:get home-page} ^:interceptors [(body-params/body-params) bootstrap/html-body]]]])

It should probably be the case that by-default content is not served as an octet-stream.

@cldwalker
Copy link
Member

I thought the default used to be text/plain. Are you suggesting that or text/html as the default? I'm for the latter since most of us render html not plain text

@avescodes
Copy link
Author

I agree it should be text/html, but regardless, this is a weird bug where a default is being squashed.

@subhash
Copy link
Member

subhash commented Jul 30, 2013

This was a bug that bit me early on, so I am partial to it :-)

As far as I can see, ring.util.response/response is returning an empty header whereas io.pedestal.service.http/html-body is using ring.util.response/content-type to explicitly set the content-type to "text/html;charset=UTF-8". Would it make sense to do the same within ring.util.response/response function (pass it through content-type with a suitable default content-type)?

I know this would work because if I change the definition of home-page this way, the bug ceases to exist:

(defn home-page
  [request]
  (resp/content-type (response "Hello World!") "text/html"))

@ghost ghost assigned avescodes Jul 31, 2013
@avescodes
Copy link
Author

I'm going to work on fixing this.

avescodes pushed a commit that referenced this issue Jul 31, 2013
@avescodes
Copy link
Author

Regardless of what the default content type should be, I wrote a test (see mention) that fails like so:

expected: (= "text/plain" (get-in response [:headers "Content-Type"]))
  actual: (not (= "text/plain" "application/octet-stream"))

Talking with @timewald it seems like we may need our own implementation of content-type-response. (I should also write a failing test in the ring middlewares test ns when I do that.)

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

No branches or pull requests

3 participants