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

Add option to create references for models #274

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

HughPowell
Copy link

When users use Swagger to automatically generate clients inline models create can create multiple classes for the logically same model.

Add and option (:refs?) to create references and a high level definitions key to hold references.

@HughPowell HughPowell force-pushed the swagger-refs branch 3 times, most recently from 4bbb46c to 8ac2798 Compare April 18, 2023 05:01
@codecov-commenter
Copy link

Codecov Report

Merging #274 (3702f2a) into master (d15ec93) will increase coverage by 0.13%.
Report is 4 commits behind head on master.
The diff coverage is 94.23%.

@@            Coverage Diff             @@
##           master     #274      +/-   ##
==========================================
+ Coverage   88.01%   88.15%   +0.13%     
==========================================
  Files          16       16              
  Lines        2052     2110      +58     
  Branches      183      185       +2     
==========================================
+ Hits         1806     1860      +54     
- Misses         63       65       +2     
- Partials      183      185       +2     
Files Changed Coverage Δ
src/spec_tools/swagger/core.cljc 96.13% <94.23%> (-1.43%) ⬇️

... and 1 file with indirect coverage changes

@opqdonut
Copy link
Member

This is related to metosin/reitit#558

- Update Clojure to 1.11.1 so we can access `update-vals`
- Add an option to swagger/transform to allow users to specify that they
  want to create references
- When references are to be created create Swagger $refs and return the
  definitions under a ::swagger/definitions key in the top level map
@HughPowell
Copy link
Author

HughPowell commented Sep 20, 2023

Sorry @opqdonut, I've been away for the last month. This has now been updated, tested and ready for review.

@opqdonut
Copy link
Member

Thanks for the ping! I've got a lot on my plate right now but I'll try to have a look at this and remember the context. Feel free to ping me again if you don't hear back.

@HughPowell
Copy link
Author

Hi @opqdonut, just following up on this. Thanks, Hugh.

@opqdonut
Copy link
Member

opqdonut commented Oct 4, 2023

Thanks for the ping! The code looks good in general (I'll have to do a careful reread though), but I have some doubts about how this interacts with some bugs & future developments we have on the reitit side. I'll try to get a small cabal together and think about these things.

Have you tested this change with reitit? Or are you using some other routing library?

@opqdonut
Copy link
Member

opqdonut commented Oct 4, 2023

Some notes:

  • this adds :definitions for spec swagger
  • meanwhile Fix malli swagger defs malli#863 added :definitions for malli swagger
    • everywhere, lifted to the very top level of the swagger fragment
    • :definitions are based on the malli registry structure

:description "",
:required true,
:schema {:$ref "#/definitions/RefSpec"}}],
:definitions {"RefSpec" {:type "object",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this :definitions is then lifted to the very top level of the swagger doc by

https://github.com/metosin/reitit/blob/15e0c95cb6e8ab1f653204f910dfb1851c0b0663/modules/reitit-swagger/src/reitit/swagger.cljc#L136

We could add a test that documents this feature end-to-end on the reitit side.

@HughPowell HughPowell closed this Oct 6, 2023
@HughPowell HughPowell reopened this Oct 6, 2023
@HughPowell
Copy link
Author

HughPowell commented Oct 6, 2023

Have you tested this change with reitit? Or are you using some other routing library?

Yes, this has (only) been tested with Reitit.

Copy link
Member

@opqdonut opqdonut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My apologies for the delay. I'm coming out of hibernation re: reitit stuff. I reread this PR and it looks good to go, I just have two new questions.

(reduce into (sorted-set))
(into []))})
(let [children' (map #(if (contains? % :$ref)
(first (vals (::definitions %)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this actually look up the value of the ref instead of assuming that ::definitions is a singleton?

(walk/postwalk
(fn [x]
(if (map? x)
(reduce-kv
(fn [acc k v]
(if (accept? k)
(-> acc (dissoc k) (merge (expand k v acc options)))
(merge-with merge-only-maps (dissoc acc k) (expand k v acc options))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this change needed?

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