Fix malli swagger defs #863
Conversation
This allows us to see the whole spec at once and fix a bug w/ custom registry definitions
...in json-schema. Also removes the leading `:` which the definitions keys also didn't have. This allows the refs and definitions keys to match and be standards-compliant.
Perhaps this really needs to be fixed at a lower level, but I noticed that at times I was getting definitions whose values where just refs back to the same definition. These seemed to come in addition to the correct result that had the actual definition in it. So this just filters out the circular ones.
|
Note that if you use any recursive schemas, you'll still see some missing defs in your swagger due to #464. |
| ([x] | ||
| (swagger-spec x nil)) | ||
| ([x options] | ||
| (expand-qualified-keywords x options))) |
There was a problem hiding this comment.
I'd like to see some tests for swagger-spec
There was a problem hiding this comment.
I'll see what I can put together from existing spec / schema swagger-spec tests and whatever was in reitit-malli for its roughly-equivalent code.
|
Good stuff! Moving the malli swagger generation from reitit to malli would be consistent with what we do with spec & schema. |
|
@opqdonut Is this just waiting on some tests for |
|
Yeah, just waiting for tests. See my comment for one possible test case. |
...in json-schema-test/references-test. Fix was in 23488b2.
...including one commented-out failing test that should pass once issue metosin#464 is fixed.
| :name "body", | ||
| :required true, | ||
| :schema {:$ref "#/definitions/malli.swagger-test~1body", | ||
| :definitions {::body {:minLength 1, :type "string"}}}}]} |
There was a problem hiding this comment.
should it be :definitions {"malli.swagger-test~1body" ...}?
There was a problem hiding this comment.
That's a good question. I think it doesn't need to be because by the time it gets serialized to JSON it gets converted to that. The one aspect I'm a little surprised by is that the / -> ~1 conversion is happening. But maybe that's just because it's being converted by JSON-Schema-aware code? Probably worth a little more investigation.
There was a problem hiding this comment.
Yeah it would be good to know where that munging is happening
There was a problem hiding this comment.
Jsonista and other clojure json libraries just convert :foo.bar/baz to "foo.bar/baz". Grepping for ~ doesn't give me any hits from the malli or reitit repositories.
There was a problem hiding this comment.
Oh! I remember now. We don't want those to have / changed to ~1 because they aren't pointers, they're just regular old JSON object keys. You have to encode / as ~1 only in JSON pointers (a.k.a. the values of $ref keys). See here. This is because the JSON keys in the pointers are separated by /, so if you have a / in a key (like we do here), you have to encode that as ~1 and it will then match your /-containing key.
So yeah, the qualified keywords get expanded into the appropriate string when serialized to JSON.
There was a problem hiding this comment.
Oh right. That makes sense. Could you add that link as a comment to -ref? I think this is ready merge after that!
There was a problem hiding this comment.
Actually, I'll just do that for you so we can avoid the round-trip.
These are changes I needed to make in here to fix metosin/reitit#558 (I will be issuing a PR for the changes needed in reitit soon too).