Fork me on GitHub
#pedestal
<
2018-06-04
>
martinklepsch11:06:09

Having some issues with io.pedestal.test: For some reason (pdt/response-for (service-fn s) :get "/") returns 404 but using curl to load the page returns 200 + proper body (same server instance/port) — any ideas why that could be?

martinklepsch11:06:17

Ok. Found the source of this: expand-routes takes this additional map with :host and :port keys. If I pass it I see the aforementioned issue. If it’s not passed everything works as expected.

martinklepsch11:06:50

Specifying the host and port as part of the URL does not seem to fix this

martinklepsch12:06:32

(pdt/parse-url “localhost:8888/“)
;; {:scheme nil, :host “localhost:8888”, :path “”, :query-string nil}

martinklepsch12:06:36

So the hostname in test-servlet-response includes the port if provided. And the port is always set to -1: https://github.com/pedestal/pedestal/blob/6b2faef808c66573e1fde564017ab1dbde1018c8/service/src/io/pedestal/test.clj#L109-L110

martinklepsch12:06:28

I don’t know if this should be added but it works. I’ll probably just not pass the {:host ,,, :port ,,, :scheme ,,,} map in testing environments

martinklepsch12:06:30

(because otherwise I’d also have to specify host/port for test requests and I don’t want to bother for now)

martinklepsch14:06:08

Opened an issue here: https://github.com/pedestal/pedestal/issues/570 included a link to a patch but not sure if it’s the right thing to do.

souenzzo11:06:38

@martinklepsch reload "all" your namespaces

martinklepsch13:06:00

Another thing I stumbled upon: The default terminator is ring-response/response which checks for a :status key and a :headers key. Is there a particular reason setting headers is required for termination?

martinklepsch14:06:40

Is it intended that url-for doesn’t throw if the supplied params are lacking some?

(url-for :artifact/version :path-params {:group-id “x”})
; “/d/x/:artifact-id/:version”

martinklepsch15:06:59

I’m not sure in what situation you’d want this behavior. Very confused that this is the default.

martinklepsch14:06:01

@U0FL657GR Do you know if this is intentional? Do you think a PR to either be strict by default or at least make that configurable would be welome?

ddeaguiar16:06:21

@martinklepsch I can’t say if that’s intentional or not but I can’t see why we’d want that. Can you open an issue? I’d wait for further feedback before opening a PR. In general, when there are doubts I’d recommend opening an issue before starting a PR.

ddeaguiar16:06:57

@martinklepsch I bet this is by design

ddeaguiar16:06:46

and : is a valid character in a path segment (refer to https://tools.ietf.org/html/rfc3986#section-3.3)

ddeaguiar16:06:12

so there’s no way for Pedestal to know if that is a param or part of the path segment

martinklepsch16:06:04

but path-params are passed as a map so there is a way to tell if a given param has been supplied or not.

martinklepsch16:06:27

you can pass a value with a colon and it will be regarded as a path param.

martinklepsch16:06:46

What’s done now is just using the key as the value which just seems wrong to me :thinking_face:

martinklepsch16:06:29

and yes that code you found is the right one, the (get path-params % %) being the culprit of the whole thing

martinklepsch16:06:15

consider this:

((route/url-for-routes (route/expand-routes #{["/thing/:id" :get nop :route-name :thing :constraints {:id #"[0-9]+"} ]}))
   :thing :path-params {:foo :bar})
the constraint says digits only but since it’s just using (str :id) it returns a url as if everything worked as intended

ddeaguiar23:06:51

@martinklepsch while using the key as a value may seem wrong to you it’s consistent with what’s contained in the :path-params collection once the route is expanded. Your example does contain additional information which could be used by Pedestal to infer that :id is a path param and is numeric. This information could be used during url generation to let the user know information was missing. Outside of such information I think the current implementation makes a reasonable compromise. I think you should open an issue so that we can solicit additional feedback.

willier14:06:49

Hi, the documentation for request-map (http://pedestal.io/reference/request-map) says that :params is “Always Present”

willier14:06:15

However, (:params request) returns nil

willier14:06:50

I am using the body-params interceptor and passing application/json and can see (:json-params request) just fine

willier14:06:16

How can I work with params in a content-type agnostic way?

martinklepsch14:06:39

I’m not sure I fully understand the docs for :params but if you want to work with params in content type agnostic way you have a few options: 1) define a simple get-params function that gets all parameter maps you care about and merges them 2) define an interceptor that does the same and stores the result somewhere in the context

willier14:06:29

yea i was hoping that the :params key would provide 1)

willier14:06:23

Merged map of path, query, and request parameters.
seems to suggest so

martinklepsch14:06:38

Yeah, understand your confusion, the docs could use some love there I guess

willier14:06:05

i will play around with it a bit more and hopefully not have to resort to writing my own function…

martinklepsch15:06:40

writing your own function shouldn’t be so bad 🙂

martinklepsch19:06:00

Was a little bit bugged by coupling my routes to the server namespace as that quickly led to cyclic deps. Ended up with this, which I’m liking so far: https://github.com/martinklepsch/cljdoc/commit/089b5fc20065607327b36d2dc570ee4702bb90a3#diff-2b617a32cb2ee893338f526bde351be6 🙂