ring-swagger

ring-swagger & compojure-api
mgrbyte 2018-09-10T15:11:21.000100Z

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:

ikitommi 2018-09-10T15:13:58.000100Z

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

mgrbyte 2018-09-10T15:14:41.000100Z

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

mgrbyte 2018-09-10T15:17:42.000100Z

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.

mgrbyte 2018-09-10T15:18:39.000100Z

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:

ikitommi 2018-09-10T15:19:05.000100Z

have you tried st/merge?

ikitommi 2018-09-10T15:19:22.000100Z

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

mgrbyte 2018-09-10T15:19:34.000100Z

no... using vanila s/merge atm

ikitommi 2018-09-10T15:20:22.000100Z

you should try that too,

mgrbyte 2018-09-10T15:21:26.000100Z

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

ikitommi 2018-09-10T15:23:22.000100Z

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

mgrbyte 2018-09-11T08:50:54.000100Z

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

mgrbyte 2018-09-11T09:43:33.000100Z

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!

mgrbyte 2018-09-11T09:57:01.000100Z

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

mgrbyte 2018-09-11T10:10:46.000100Z

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:)

ikitommi 2018-09-11T13:04:39.000100Z

@mgrbyte The fix looks good.

ikitommi 2018-09-11T13:06:18.000100Z

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?

ikitommi 2018-09-11T13:06:43.000100Z

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

mgrbyte 2018-09-11T13:09:32.000100Z

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 🤷

mgrbyte 2018-09-11T15:52:01.000100Z

thanks for the links there.

mgrbyte 2018-09-11T15:52:17.000100Z

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

mgrbyte 2018-09-11T15:52:36.000100Z

Now values are not being transformed 😞

mgrbyte 2018-09-11T15:53:22.000100Z

keywords within map spec specifically

mgrbyte 2018-09-11T15:53:57.000100Z

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

ikitommi 2018-09-11T15:54:08.000100Z

keyword leaf values?

mgrbyte 2018-09-11T15:55:41.000100Z

yeah

ikitommi 2018-09-11T15:56:12.000100Z

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

mgrbyte 2018-09-11T15:56:14.000100Z

this works when I use :coercion :spec in api

ikitommi 2018-09-11T15:56:30.000100Z

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

ikitommi 2018-09-11T15:56:43.000100Z

Oh, hmm.

mgrbyte 2018-09-11T15:57:20.000100Z

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

mgrbyte 2018-09-11T15:57:40.000100Z

restarting the app in between of course

ikitommi 2018-09-11T15:57:46.000100Z

thats weird.

ikitommi 2018-09-11T15:58:30.000100Z

Could you pr the curent status into the repo?

mgrbyte 2018-09-11T15:58:49.000100Z

the example repo or my "real" app?

mgrbyte 2018-09-11T15:59:19.000100Z

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

mgrbyte 2018-09-11T16:07:02.000100Z

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

ikitommi 2018-09-11T16:31:30.000100Z

https://github.com/mgrbyte/capi-spec-transforms/pull/1

mgrbyte 2018-09-12T10:00:26.000100Z

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

mgrbyte 2018-09-12T13:15:32.000100Z

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.

ikitommi 2018-09-10T17:27:44.000100Z

@mgrbyte run the code:

ikitommi 2018-09-10T17:27:52.000100Z

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=>

ikitommi 2018-09-10T17:28:23.000100Z

… 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"}]

ikitommi 2018-09-10T17:29:09.000100Z

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.

ikitommi 2018-09-10T17:29:21.000100Z

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

vuuvi 2018-09-10T21:39:37.000100Z

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

vuuvi 2018-09-10T21:40:09.000100Z

the swagger.json gets returned by the API

vuuvi 2018-09-10T21:40:13.000100Z

but there’s an error

vuuvi 2018-09-10T21:40:24.000100Z

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

vuuvi 2018-09-10T21:40:40.000100Z

vuuvi 2018-09-10T21:40:45.000100Z