Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for primitive arrays (fixes #55) #56

Closed

Conversation

timgilbert
Copy link

Ok, after much scratching my head I've come up with this fix for #55. It works for what I need it to do, which is generating schemas for lists of primitives as body or response parameters.

One thing it doesn't currently do is refer back to schema names that define lists of primitives; instead it will copy them into the parameter definition. I left some commented code in a test that demonstrates this. I may spend a bit of time trying to fix that, but the rest of the PR seems useful enough that I'm submitting it as-is for now. Please let me know if there's anything you'd like me to change.

@ikitommi
Copy link
Member

ikitommi commented Aug 5, 2015

some comments:

  • vector? is not matching lists or sets (in json_schema.clj we are matching clojure.lang.Sequential & clojure.lang.IPersistentSet).
  • In Swagger, there are parameters (swagger-spesific parameter descriptions) and properties (json schema description of models/schemas). I believe the primitive array schemas could be modelled just as parameters, so no need to generate a (clojure) schema names for those.
    {
      "name": "id",
      "in": "path",
      "description": "ID of pet to use",
      "required": true,
      "type": "array",
      "items": {
        "type": "string"
      }
    }

with this, there is no need to tune the with-named-sub-schemas, which is responsible for ensuring that a deeply nested schema has valid schema names at all levels to support the properties-part of the spec.

here's a commented swagger2.clj:

 (let [[paths definitions] (-> swagger
                              ; this is responsible for ensuring schema names for deeply nested schemas (to support properties)
                              ensure-body-and-response-schema-names
                              ; here's the beef (including extracting parameters)
                              (extract-paths-and-definitions options))]

, so I would dig in from extract-paths-and-definitions -> convert-operation -> convert-parameters & convert-responses.

for there, there seems to guards like "the body model should have a schema":

  (if-let [schema (rsc/peek-schema model)] ; checks that there is valid reference within

, which could say ".. or it's a primitive array".

So, all changes could be done swagger2.clj. Maybe :)

sorry for the bad docs and looking forward to this change!

@timgilbert
Copy link
Author

Thanks for taking the time to look this over @ikitommi - I hope I haven't nuked my local branch of ring-swagger with a billion (println) statements added! ;)

I should have some time to look at this this weekend, I'll make the vector? check more general and follow your suggestion for looking at the (peek-schema) guards. IIRC, I was nervous about messing with those because I thought I might damage the ring-swagger feature where it can detect [ some/schema-def ] and hoist some/schema-def up to the top level of the swagger definitions. Your explanation makes sense though.

@dzhus
Copy link

dzhus commented Sep 29, 2015

I believe the primitive array schemas could be modelled just as parameters

If parameters are Swagger-specific, won't that make array schemas invisible to validators treating Swagger spec as standard JSON schema? Shouldn't properties be used instead?

(this is in fact a vaguely concealed attempt to ping involved parties because I want this fixed but I'm not sure if I can do it myself)

@ikitommi
Copy link
Member

SORRY for keeping you waiting. This got buried under everything :( But should be now fixed in [metosin/ring-swagger "0.23.0-SNAPSHOT"] - there was an obvious way with the latest code base to do this. Added creds to you.

@ikitommi ikitommi closed this Aug 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants