metosin / compojure-api

Sweet web apis with Compojure & Swagger
http://metosin.github.io/compojure-api/doc/
Eclipse Public License 1.0
1.11k stars 149 forks source link

Spec coercion from JSON fails when binding symbol is same as the spec keyword? #396

Closed danielcompton closed 4 years ago

danielcompton commented 5 years ago

Library Version(s)

Problem

We are moving our GET routes to use namespaced keys for the query parameters. The namespaced key in the query param matches the spec we are using. We are finding an inconsistency in coercion behaviour between when the binding value matches the namespaced key and when it is different.

For example:

(s/def :myapp/id uuid?)
(s/def :other/id uuid?)

;; Inside a routes function

    (GET "/different-spec" req
      :coercion :spec
      :query-params [my/id :- :other/id]
      (ok {:id id
           :type (type id)}))

    (GET "/same-spec" req
      :coercion :spec
      :query-params [myapp/id :- :other/id]
      (ok {:id id
           :type (type id)}))

When the binding value is not a specced value:

$ curl --silent -X GET --header 'Accept: application/json' 'http://localhost:3010/different-spec?my%2Fid=91f70ae6-af52-4c51-9ce0-9f12097d7076' | jq
{
  "id": "91f70ae6-af52-4c51-9ce0-9f12097d7076",
  "type": "java.util.UUID"
}

When the binding value has a spec for the same name:

$ curl --silent -X GET --header 'Accept: application/json' 'http://localhost:3010/same-spec?myapp%2Fid=91f70ae6-af52-4c51-9ce0-9f12097d7076' | jq
{
  "spec": "(spec-tools.core/spec {:spec (clojure.spec.alpha/keys :req [:myapp/id]), :type :map, :keys #{:myapp/id}, :keys/req #{:myapp/id}})",
  "problems": [
    {
      "path": [
        "myapp/id"
      ],
      "pred": "clojure.core/uuid?",
      "val": "91f70ae6-af52-4c51-9ce0-9f12097d7076",
      "via": [
        "myapp/id"
      ],
      "in": [
        "myapp/id"
      ]
    }
  ],
  "type": "compojure.api.exception/request-validation",
  "coercion": "spec",
  "value": {
    "myapp/id": "91f70ae6-af52-4c51-9ce0-9f12097d7076"
  },
  "in": [
    "request",
    "query-params"
  ]
}

Is this correct behaviour? Maybe I'm missing something here? I expected the coercion behaviour to only depend on the specs that we are specifying for coercion, not the names we are binding them to.

ikitommi commented 5 years ago

Short

clojure.spec doesn't support of having inlined definitions for qualified map keys - by design.

how to fix:

1) don't try to mix qualified map keys with different values 2) in any case, wrap all leaf specs manually to spec-tools.core/Spec 3) poke clojure.spec team to enable clean coercion, without any wrapping See CLJ-2116 & CLJ-2251.

(require '[spec-tools.spec :as spec])

(s/def :myapp/id spec/uuid?)
(s/def :other/id spec/uuid?)

or:

(require '[spec-tools.core :as st])

(s/def :myapp/id (st/spec uuid?))
(s/def :other/id (st/spec uuid?))

Long Story

For the fnk syntax in compojure-api, data-specs are used to create the actual specs. It needs to do whatever is possible with s/keys, which doesn't allow qualified keys with inline values, from s/keys docs:

  In addition, the values of *all* namespace-qualified keys will be validated
  (and possibly destructured) by any registered specs. Note: there is
  no support for inline value specification, by design.

The problem here is how data-specs are turned into specs:

* Map (keyword) keys
  * can be qualified or non-qualified (a qualified name will be generated for it)
  * are required by default
  * can be wrapped into ds/opt or ds/req for making them optional or required

In the first endpoint: there is no :my/id spec defined in the spec registry => a new one is registered, with the current value of :other/id, automatically wrapped into spec-tools/Spec to support coercion;

(s/get-spec :my/id)
; #Spec{:form clojure.core/uuid?, :type :uuid}

=> it works.

The second one fails as there is already a spec of name :myapp/id, value is reused:

(s/get-spec :myapp/id)
; #object[clojure.spec.alpha$spec_impl$reify__1987 0x4cb13b7f "clojure.spec.alpha$spec_impl$reify__1987@4cb13b7f"]

... here, the leaf spec is a plain clojure.spec.alpha/Spec, unable to coerce itself. to fix it:

(require '[spec-tools.spec :as spec])

(s/def :myapp/id spec/uuid?)
(s/def :other/id spec/uuid?)
danielcompton commented 5 years ago

Thanks heaps for the detailed explanation, much appreciated! I'll take a look at those tickets too, though it doesn't look promising 😢.

ikitommi commented 5 years ago

with the latest alpha, simple specs don't need to be wrapped, so this works too:

(s/def :myapp/id uuid?)
(s/def :other/id uuid?)
danielcompton commented 5 years ago

Do you mean an alpha of Compojure-API or Spec? Lots of alphas going around these days :)

ikitommi commented 5 years ago

Oh, the latest c-api alpha, via latest non-alpha of spec-tools :)

Here's a sample app with different coercion: https://github.com/metosin/c2.

Reitit also supports spec coercion, here's a sample app with specs & data-specs: https://github.com/metosin/reitit/tree/master/examples/http-swagger

miikka commented 4 years ago

The example works nowadays, so I'm closing this.