Request contains nil :body and :query-string

[conan]

I've tried using both jetty and httpkit, in both cases my request maps contain nil values for the optional keys :body and :query-string; this is not compliant with the :ring/request spec. Is it possible to remove the keys from the request map instead of setting them to nil in order to remain spec compliant?

Call to #'hanabi.web/initialise did not conform to spec:
web.clj:181

-- Spec failed --------------------

Function arguments

  ({:reitit.core/match ...,
    :reitit.core/router ...,
    :remote-addr ...,
    :params ...,
    :headers ...,
    :async-channel ...,
    :server-port ...,
    :content-length ...,
    :form-params ...,
    :websocket? ...,
    :web/session ...,
    :query-params ...,
    :content-type ...,
    :character-encoding ...,
    :uri ...,
    :server-name ...,
    :query-string ...,
    :path-params ...,
    :body nil,
          ^^^
    :scheme ...,
    :request-method ...})

should satisfy

  #object[clojure.spec.alpha$regex_spec_impl$reify__2436 0x765709fc "clojure.spec.alpha$regex_spec_impl$reify__2436@765709fc"]

-- Relevant specs -------

:ring.request/body:
  :clojure.spec.alpha/unknown
:ring/request:
  (clojure.spec.alpha/keys
   :req-un
   [:ring.request/server-port
    :ring.request/server-name
    :ring.request/remote-addr
    :ring.request/uri
    :ring.request/scheme
    :ring.request/protocol
    :ring.request/headers
    :ring.request/request-method]
   :opt-un
   [:ring.request/query-string :ring.request/body])
:web.ring/request:
  (clojure.spec.alpha/merge
   :ring/request
   (clojure.spec.alpha/keys
    :req
    [:web/session]
    :req-un
    [:web.ring.request/edn-params]))

-- Spec failed --------------------

Function arguments

  ({:reitit.core/match ...,
    :reitit.core/router ...,
    :remote-addr ...,
    :params ...,
    :headers ...,
    :async-channel ...,
    :server-port ...,
    :content-length ...,
    :form-params ...,
    :websocket? ...,
    :web/session ...,
    :query-params ...,
    :content-type ...,
    :character-encoding ...,
    :uri ...,
    :server-name ...,
    :query-string nil,
                  ^^^
    :path-params ...,
    :body ...,
    :scheme ...,
    :request-method ...})

should satisfy

  #object[clojure.spec.alpha$regex_spec_impl$reify__2436 0x765709fc "clojure.spec.alpha$regex_spec_impl$reify__2436@765709fc"]

-- Relevant specs -------

:ring.request/query-string:
  :clojure.spec.alpha/unknown
:ring/request:
  (clojure.spec.alpha/keys
   :req-un
   [:ring.request/server-port
    :ring.request/server-name
    :ring.request/remote-addr
    :ring.request/uri
    :ring.request/scheme
    :ring.request/protocol
    :ring.request/headers
    :ring.request/request-method]
   :opt-un
   [:ring.request/query-string :ring.request/body])
:web.ring/request:
  (clojure.spec.alpha/merge
   :ring/request
   (clojure.spec.alpha/keys
    :req
    [:web/session]
    :req-un
    [:web.ring.request/edn-params]))

-------------------------
Detected 2 errors

[ikitommi]

I think at least :query-string should be s/nillable. Both Immutant and Aleph use custom Map implementations with predefined keys but the implementations read the values directly from underlaying implementations, which can return nil.

[weavejester]

If the Jetty adapter is doing the wrong thing it should be fixed.

I'm not sure whether the best thing to do is to make the keys nillable. I currently lean toward not, as that gives more relevance to contains?, e.g. (contains? request :query-string). I'm also reluctant to treat nil and a missing key as being the same, if Clojure spec itself treats them differently.

If Immutant and Aleph use a custom map implementation, I don't see any reason why they also can't dictate a key as being missing.

[ikitommi]

Both Aleph & Immutant use zero-copy requests where the shape of the request needs to be defined forehand and the values are lazily fetched on-demand from the underlaying impementation (Netty or Undertow). This is a big win in performance, but can't strip away keys (in a performant way).

• Aleph: https://github.com/ztellman/aleph/blob/master/src/aleph/http/core.clj#L184-L206
• Immutant: https://github.com/ikitommi/immutant/blob/0777ebda97391ee2a49871837f1794458fc5348e/web/src/immutant/web/internal/undertow.clj#L226-L244

[weavejester]

If get is calculated on the fly, then contains? can be as well. The only performance hit would be in keys, but arguably it would return better information as a result.

In terms of pros and cons, here's how I see it:

Pros for adding nils

• Currently incorrect adapters wouldn't have to fix their implementations
• Adapters would be a little easier to write
• Request maps could be records

Cons

• We'd have to update the spec
• There would be two ways of denoting optional keys instead of one
• Less idiomatic?
• contains? and keys become much less useful
• As we make more keys optional (e.g. :headers), more things would become nillable as well

I'm not quite sure what the best option is. My inclination is that allowing nils would make life a little easier for adapter writers, but might make the job of end developers a little more complex. My current inclination is to side with the latter, but I'm open to convincing.

[conan]

I don't have a strong argument either way. My feeling from an idiomatic perspective is that nil should not be allowed, but my feeling from a pragmatic perspective is that this issue can be fixed more easily if they are. Though my inner aesthete complains, I think I'd also side with the latter because I haven't been able to find any complaints about the current behaviour that allows nils, and I'd rather the issue was fixed than not. Not very helpful, sorry.