Fork me on GitHub
#ring-swagger
<
2018-09-10
>
mgrbyte15:09:21

are there any known issues with using spec coercion with a spec that's comprised of s/merge, s/keys and s/map-of? Having an issue where the spec functions as I'd expect at the repl, but not via compojure-api. In the repl, I simulate the data that's being sent via form, and get :clojure.spec.alpha/invalid (expected), but via the app I'm getting no errors, and the key/val pairs that would cause the error appear to be stripped from the data received by the handler :thinking_face:

ikitommi15:09:58

@mgrbyte I think c-api strips non-defined keys by default. Is that good or bad?

mgrbyte15:09:41

bad in my case, as I don't know how to do what I need atm 😆

mgrbyte15:09:42

I have a fairly complex spec, can paste/share the code somewhere. Essentially: a map of attributes A. 2 x specs X and Y that define combinations of A that are permitted by a request. I'm trying to make the app return a 4xx response if neither variant of A is satisfied.

mgrbyte15:09:39

thanks @ikitommi! 🙂 I'll see if I can figure out how to override those transformers at the app level and see if that works... :thinking_face:

ikitommi15:09:05

have you tried st/merge?

ikitommi15:09:22

It's like`s/merge` but works.

mgrbyte15:09:34

no... using vanila s/merge atm

ikitommi15:09:22

you should try that too,

mgrbyte15:09:26

I've just tried st/merge instead of s/merge, no noticable difference there - pretty sure it's the stripping of data that's the issue

ikitommi15:09:22

ok. If you can isolate a sample repo/code, I can try to figure out too.

ikitommi17:09:52

default-transformer-stripping-data-issue.handler=> (-> ::variant-a s/spec :keys)
#{:g/cn :p/how :g/bt :g/sp :p/who :p/when :p/why :g/sn}
default-transformer-stripping-data-issue.handler=> (-> ::variant-b s/spec :keys)
#{:g/cn :p/how :g/sp :p/who :p/when :p/why}
default-transformer-stripping-data-issue.handler=>

ikitommi17:09:23

… with extra-keys strpping:

default-transformer-stripping-data-issue.handler=> (st/conform ::update {:g/sp "s1", :g/cn "cn1", :g/bt "XXXX"} st/strip-extra-keys-transformer)
[:default-transformer-stripping-data-issue.handler/variant-a #:g{:cn "cn1", :sp "s1"}]

ikitommi17:09:09

so you are right: c-api first strips out keys that are not defined and then checks the keys => the invalid keys are removed and it succeeds.

ikitommi17:09:21

Normally, this would be good, here - I guess not.

mgrbyte08:09:54

thanks for looking, really appreciate your input and help :thumbsup:

mgrbyte09:09:33

just been reading spec-tools sources, looks like this has already been considered, as there's fail-on-extra-keys-type-decoders in addition to strip-extra-keys-decoders. Although there's a TODO that makes e wary of directly using that: https://github.com/metosin/spec-tools/blob/master/src/spec_tools/transform.cljc#L113 Out of curiosity, what's the rationale why strip variant was chosen as default for c-api? I think it's great to be able to take request data, convert to EDN/clojure and run it through the spec at the repl; I've been assuming that giving a parameter spec as the metadata for :body-params or :parameters would codify the validity of the request/response, the stripping of extra-keys seems to break that invariant? thanks!

mgrbyte09:09:01

erk, my bad (not= fail-on-extra-keys anything-to-do-with-stripping)

mgrbyte10:09:46

fwiw, I've "fixed" this by: https://github.com/mgrbyte/capi-spec-transforms/commit/47736063b20a1c373140cbaea98acdc6d59a1e54 Hopefully that is "idiomatic" (for some value of idiomatic! :thinking_face:)

ikitommi13:09:39

@mgrbyte The fix looks good.

ikitommi13:09:18

about the default of dropping keys… there was a discussion about the default long time ago. Can’t recall why that was chosen. I think at least the option to pass-through all values easy to do. Would you like to do a PR where the spec-coercion could be easily modified to do any of the: strip, fail or nothing?

ikitommi13:09:43

still alpha, so we can re-visit the decision too.

mgrbyte13:09:32

I will look into doing a PR, possibly this evening. think if it's easy to change, that'd be good. I think the default should be "less surprise" - guessing for folks who are using spec that'd mean not dropping keys, but 🤷

ikitommi13:09:20

the stripping… sadly there is no extra info emitted if the spec fails on extra keys, just ::s/invalid. there is https://dev.clojure.org/jira/browse/CLJ-2115 and https://dev.clojure.org/jira/browse/CLJ-2251, but both 1.5y old without any progress.

mgrbyte15:09:01

thanks for the links there.

mgrbyte15:09:17

So it looks like my custom coercion wasn't good enough/the complete story

mgrbyte15:09:36

Now values are not being transformed 😞

mgrbyte15:09:22

keywords within map spec specifically

mgrbyte15:09:57

I'm guessing I have to (re)implement the entire Coercion protocol?

ikitommi15:09:08

keyword leaf values?

ikitommi15:09:12

the leaf specs need to be "decodable", which means Spec Records fow now. there are versions in soec-tools.spec for most/all predicates

mgrbyte15:09:14

this works when I use :coercion :spec in api

ikitommi15:09:30

e.g. spec-tools.spec/keyword?

mgrbyte15:09:20

I can switch between the new custom coercion and the default (`:spec`) and observe the transform works in the later, but not the former

mgrbyte15:09:40

restarting the app in between of course

ikitommi15:09:46

thats weird.

ikitommi15:09:30

Could you pr the curent status into the repo?

mgrbyte15:09:49

the example repo or my "real" app?

mgrbyte15:09:19

will try and dup in the example repo, less to look at

mgrbyte16:09:02

have to get a 🚌 home now, but will be looking again later tonight to see what I can do. thanks again for taking interest! 😍

mgrbyte10:09:26

thanks @ikitommi - i merged and tested, still doesn't work for me though thinking_face I'm an idiot, made a typo that stopped it working

mgrbyte13:09:32

fwiw, I've just pushed the final working version to master: https://github.com/mgrbyte/capi-spec-transforms/commit/432ed6b3011d2eda2e193a550e6e8062fe932a80 There was one additional issue, in that the (repeat json-transformer) code I had in options passed to create-coercion wasn't working, but fixed that easily enough.

vuuvi21:09:37

hey @ikitommi I’ve updated my swagger scheme in my routing file and for some reason it’s not rendering the swagger.json, even though one has been returned

vuuvi21:09:09

the swagger.json gets returned by the API

vuuvi21:09:13

but there’s an error

vuuvi21:09:24

and I’ve included the screen where stuff isn’t rendering

ikitommi13:09:20

the stripping… sadly there is no extra info emitted if the spec fails on extra keys, just ::s/invalid. there is https://dev.clojure.org/jira/browse/CLJ-2115 and https://dev.clojure.org/jira/browse/CLJ-2251, but both 1.5y old without any progress.