[Clojure] Add model generation and conforming #122

f-f · 2018-05-21T16:26:07Z

PR checklist

Read the contribution guidelines.
Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
Filed the PR against the correct branch: Default: master.
Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

This big PR adds support for generating Clojure Specs from OpenAPI Models.
Spec is a core library available since Clojure 1.9. Description from the linked page:

The spec library specifies the structure of data, validates or destructures it, and can generate data based on the spec.

Things going on in this PR:

Update of the Clojure version 1.7.0 -> 1.9.0: This is backwards compatible, so users can just bump their version and everything should be fine.
Generate spec files from Models: they are in the specs/ folder, and imported by all api files. This is done with the Data Specs from the spec-tools library, as they are much nicer to read/write than vanilla specs.
Add types to api functions via defn-spec: in this way at development time all the inputs and outputs of function calls will be checked and will except if they don't conform (you need to call orchestra.spec.test/instrument for it to be active).
Add conforming of data returned from the API to the proper model/spec: this includes e.g. creating date objects instead of getting back strings.
Add :decode-models setting in api-context: so the above conforming can be turned off if it's indesiderable. This is opt-out (but we can make it opt-in if we want to keep backwards compatibility).

I am not entirely sure this is correct in all cases (e.g. I'm suspicious about multiple arities), but we have an API much bigger than Petstore on which this is already deployed, and the generated code looks fine and works.

/cc @wing328 @xhh

Moreover, if this PR goes in I'd be available to be in the Clojure generator "technical committee", as the list is currently empty, and I could help with it.

- Generate clojure.specs from models - Optionally validate them at runtime (validation is active if orchestra.spec.test/instrument is called after specs are imported) - Coerce the results of the API calls to get objects that conform to the spec (e.g. get Date objects for dates and time instead of strings)

So that the order of the forms will be resolved by the compiler, otherwise we'd have to implement a topological ordering.

wing328 · 2018-05-24T14:07:59Z

Add conforming of data returned from the API to the proper model/spec: this includes e.g. creating date objects instead of getting back strings.

If I understand correctly, this is a breaking change and therefore I've labeled this PR as such.

I'm testing the change and will merge when the test passes.

wing328 · 2018-05-24T14:19:15Z

samples/client/petstore/clojure/pom.xml

@@ -1,32 +0,0 @@
-<project>


@f-f please add back pom.xml so that CI (circleci) can run the tests for Clojure Petstore sample

Yes sorry, I accidentally deleted them.

No problem. Let us know if you need help running the tests locally for verification.

wing328 · 2018-05-24T14:20:50Z

samples/client/petstore/clojure/test/open_api_petstore/api/pet_test.clj

@@ -1,104 +0,0 @@
-(ns open-api-petstore.api.pet-test


@f-f please add back this file as well as other "*_test.clj" files for Petstore integration tests.

f-f · 2018-05-26T14:09:44Z

If I understand correctly, this is a breaking change and therefore I've labeled this PR as such

It is if the :decode-models setting is true (which is the current state). If not, the code behaves as it was before. If we want to keep backwards compatibility we can default it to false, and make this new behaviour opt-in.

wing328 · 2018-05-26T14:11:54Z

Thanks for the explanation. I would suggest defaulting :decode-models to false to make it backward compatible.

f-f · 2018-05-26T14:32:02Z

👍 I pushed the change to the setting and the updated sample.

However, the tests are failing because we are on Java7, but this PR requires Java8. How can I upgrade it?

jmini · 2018-05-26T15:34:05Z

The generator itself now requires java 8 so I think that it is fine to upgrade it.

wing328 · 2018-05-26T15:59:57Z

However, the tests are failing because we are on Java7, but this PR requires Java8. How can I upgrade it?

Please remove https://github.com/f-f/openapi-generator/blob/2750e249a08956ad67d87585d832b94813555b28/CI/pom.xml.circleci.java7#L839

f-f · 2018-05-27T12:18:05Z

@wing328 I pushed a fix for the tests, but they don't pass on my machine due to the API returning a 500 (which happens when I run the tests on master). They also don't pass on the CI seemingly due to the same issue (but CI log doesn't show the exceptions?).

I'm testing by running mvn verify inside the samples/client/petstore/clojure. Am I missing any step?

wing328 · 2018-05-27T16:04:19Z

@f-f the CircleCI tests failed with the following message:

lein test open-api-petstore.api.pet-test

lein test open-api-petstore.api.store-test

lein test :only open-api-petstore.api.store-test/test-place-and-delete-order

FAIL in (test-place-and-delete-order) (store_test.clj:31)
expected: (= (attr order) (attr fetched))
  actual: (not (= 200 nil))

lein test open-api-petstore.api.user-test

lein test :only open-api-petstore.api.user-test/test-create-and-delete-user

FAIL in (test-create-and-delete-user) (user_test.clj:27)
expected: (= (attr user) (attr fetched))
  actual: (not (= 0 nil))

lein test open-api-petstore.core-test

I'll mark this as a breaking change without fallback as the Clojure client no longer works with JDK7

f-f · 2018-05-27T16:25:22Z

the CircleCI tests failed with the following message

@wing328 yes I saw that, but I'm not able to replicate locally (to get debug info and inspect what's wrong with it), as I get a HTTP 500 from the API on these two tests - and I get the 500 also when running the tests on the current master, so it looks like I didn't introduce the problem here. Any suggestions?

I'll mark this as a breaking change without fallback as the Clojure client no longer works with JDK7

👍

wing328 · 2018-05-28T07:01:47Z

One way is to run the petstore server locally: https://hub.docker.com/r/swaggerapi/petstore/

and you will need to update the host table:

127.0.0.1 petstore.swagger.io

f-f · 2018-05-29T08:20:02Z

@wing328 thanks a lot!

I fixed the existing tests, and added a test for the new decoding feature; now all tests pass locally.

wing328 · 2018-05-30T14:24:15Z

@f-f The CircleCI failed with the following errors:

Retrieving clojure-complete/clojure-complete/0.2.4/clojure-complete-0.2.4.jar from clojars
Exception in thread "main" java.io.FileNotFoundException: Could not locate open_api_petstore/specs/Order__init.class or open_api_petstore/specs/Order.clj on classpath. Please check that namespaces with dashes use underscores in the Clojure file name., compiling:(open_api_petstore/api/pet.clj:1:1)
	at clojure.lang.Compiler.load(Compiler.java:7526)
	at clojure.lang.RT.loadResourceScript(RT.java:379)
	at clojure.lang.RT.loadResourceScript(RT.java:370)
	at clojure.lang.RT.load(RT.java:460)
	at clojure.lang.RT.load(RT.java:426)
	at clojure.core$load$fn__6548.invoke(core.clj:6046)
	at clojure.core$load.invokeStatic(core.clj:6045)
	at clojure.core$load.doInvoke(core.clj:6029)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invokeStatic(core.clj:5848)
	at clojure.core$load_one.invoke(core.clj:5843)
	at clojure.core$load_lib$fn__6493.invoke(core.clj:5888)
	at clojure.core$load_lib.invokeStatic(core.clj:5887)
	at clojure.core$load_lib.doInvoke(core.clj:5868)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invokeStatic(core.clj:659)

Please take a look when you've time.

f-f · 2018-05-30T16:12:43Z

@wing328 yeah I saw that and it's failing because of not finding the classes, which the last commit I pushed (88726c1) should have fixed (needs the capitalized filenames).
However, on my mac I have a case-insentitive HFS, so I'm not able to push the updated petstore sample with the capitalized filenames (but the tests pass on my machine). I'll fix as soon I get to my linux machine.

f-f · 2018-06-05T12:52:12Z

I found a small bug in the meanwhile, so I'll ping again here once it's fixed

wing328 · 2018-06-17T03:18:48Z

I found a small bug in the meanwhile, so I'll ping again here once it's fixed

Thanks for the update. If you need help from us, please let us know.

Since it's a breaking change, I've changed the target branch to be "4.1.x" (for breaking changes without fallback)

langford · 2018-07-05T16:34:42Z

@f-f Thank you for taking the time to do this work, and share it back to the community.

I'm seeing this working without test failures on MacOSX, does this fail anywhere other than CI reliably?

langford · 2018-07-05T16:47:19Z

Also, @f-f, you should be able to use

    git config core.ignorecase false

in the project directory to fix your issue with case insensitivity

https://stackoverflow.com/a/17688308

f-f · 2018-07-07T16:25:42Z

Hi @langford, thanks for dropping by!

Also thanks for the ignorecase trick, TIL 👍

I'll add a bit of context to what's blocking this PR:

at first (when I implemented this for internal use) I generated all the specs in the same file and it was working fine for our project. However, when pull requesting it I realized it did not work as reliably for the Petstore sample API (my guess is something about the order in which specs are evaluated, but I didn't investigate too much)
...as I just went for "ok if the order is a problem let's just use Clojure's namespace resolution algorithm to unroll the DAG for us and evaluate the specs in the right order". So I switched to generating one spec per file.
I did a PoC, and worked nicely. However, this has issues for importing all the relevant specs in the files that need to use them (and name shadowing between various parameters that functions have and the spec names). So I thought to just capitalize the specs names (as it's convention to capitalize "types").
This is the current state, and works nicely for the Petstore. However it stopped working for our internal project, because we have a model called Package. Turns out, in Clojure you cannot define a Package var, because the name is taken:

IllegalStateException Package already refers to: class java.lang.Package

^ this is the bug I was referring to in my last comment. The solution I'd like to adopt would be to uncapitalize again the model names, and then just rename the name clashes where relevant. I'll try to put something together later today or tomorrow or next week.

@wing328 thanks for asking if I'd need some help :) My only desire would be to have a more complex example API to test the program on (as this PR works fine on the Petstore tests but breaks on our API)

wing328 · 2018-07-07T16:51:56Z

@f-f you can consider using fake pestore spec instead of petstore.yaml/json as fake petstore covers a lot more edge cases (which may not be all addressed by this PR)

langford · 2018-07-09T19:53:17Z

This is the current state, and works nicely for the Petstore. However it stopped working for our internal project, because we have a model called Package. Turns out, in Clojure you cannot define a Package var, because the name is taken:

Would prefixing all the spec names with a long hardcoded string allow this to be verified as working and merged sooner? It seems like that step could allow this all to be validated without worrying about removing the capitalization code.

I offer this alternative as it seems this would also be less mysterious than context-dependent renames for people using the plugin on their own codebases. I doubt many people would go "of course my model name collides with the java keyword "strictfp", that's why it renames.

Example:

    oags-spec-Petstore, oags-spec-Package

more complex example API to test the program on

Checking large specs is a great idea. Other specs that might work for validation purposes:

YAML, possibly handwritten:
https://github.com/stripe/openapi/blob/master/openapi/spec3.yaml

JSON generated:
https://github.com/Azure/azure-rest-api-specs/blob/master/specification/billing/resource-manager/Microsoft.Billing/preview/2018-03-01-preview/billing.json

Several different APIs using many features (2.0, but seems like they should be compatible):
https://gist.github.com/arno-di-loreto/5a3df2250721fb154060#file-simple_openapi_specification_22_documentation_with_gfm-yaml

wing328 · 2018-08-05T12:47:11Z

@f-f is there anything we (the community) can help with this PR?

f-f · 2018-08-06T11:25:07Z

I had the fix on our patched version since a while, but wanted to test it and think about it a bit more.
Anyways I just pushed it here, and it passes the tests on the petstore and on our internal (quite complex) API, so it's good to go by me :)

Things I did to fix:

uncapitalize the generated specs, so they are not mixed up with classnames
added a postfix to the spec names, so they don't risk being mixed up with other names, as @langford suggested

@wing328

is there anything we (the community) can help with this PR?

Yeah, as I mentioned before we should setup more complex tests. I opened #743 to track that

wing328 · 2018-08-11T17:09:41Z

@f-f I'll merge it into 4.0.x tomorrow (Sunday) if no one has further feedback on this PR.

wing328 · 2018-08-13T07:05:05Z

@f-f PR merged into 4.0.x branch. Thanks for your contribution 👍

f-f · 2018-08-13T10:14:52Z

@wing328 thank you! :)

f-f added 3 commits May 19, 2018 00:19

[Clojure] Make model conforming configurable and opt-out

acf1d8d

[Clojure] Move specs from a single file to a ns per model

8a29496

So that the order of the forms will be resolved by the compiler, otherwise we'd have to implement a topological ordering.

wing328 added Client: Clojure Enhancement: General labels May 24, 2018

wing328 added this to the 3.0.0 milestone May 24, 2018

wing328 added the Breaking change (without fallback) label May 24, 2018

wing328 reviewed May 24, 2018

View reviewed changes

f-f force-pushed the clojure/add-model-generation branch from 2abe0c8 to 91fa925 Compare May 26, 2018 14:05

[Clojure] Update petstore sample and set automatic decoding off

2750e24

f-f force-pushed the clojure/add-model-generation branch from 91fa925 to 2750e24 Compare May 26, 2018 14:31

f-f added 2 commits May 26, 2018 19:25

[Clojure] Stop testing Clojure generator on Java7

473cf36

[Clojure] Fix tests and handling of multiple arity

79f80ca

[Clojure] Fix tests and add testing for the new decoding feature

6a7aab2

[Clojure] Capitalize names of generated models

88726c1

[Clojure] Rename petstore specs to be capitalized

f4a7ee1

wing328 modified the milestones: 3.0.0, 3.0.1 Jun 1, 2018

wing328 modified the milestones: 3.0.1, 4.0.0 Jun 11, 2018

wing328 changed the base branch from master to 4.0.x June 12, 2018 14:44

wing328 added the WIP Work in Progress label Jun 17, 2018

Revert to lowercase spec names, and postfix the data specs

6e8fc9f

f-f mentioned this pull request Aug 6, 2018

[Clojure] Add more complex spec tests to run the generator on #743

Open

wing328 merged commit 74d7012 into OpenAPITools:4.0.x Aug 13, 2018

f-f deleted the clojure/add-model-generation branch August 13, 2018 10:15

wing328 removed the WIP Work in Progress label Dec 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clojure] Add model generation and conforming #122

[Clojure] Add model generation and conforming #122

f-f commented May 21, 2018 •

edited

Loading

wing328 commented May 24, 2018 •

edited

Loading

wing328 May 24, 2018

f-f May 26, 2018

wing328 May 26, 2018

wing328 May 24, 2018

f-f commented May 26, 2018

wing328 commented May 26, 2018

f-f commented May 26, 2018

jmini commented May 26, 2018

wing328 commented May 26, 2018

f-f commented May 27, 2018 •

edited

Loading

wing328 commented May 27, 2018

f-f commented May 27, 2018

wing328 commented May 28, 2018

f-f commented May 29, 2018

wing328 commented May 30, 2018

f-f commented May 30, 2018

f-f commented Jun 5, 2018

wing328 commented Jun 17, 2018

langford commented Jul 5, 2018

langford commented Jul 5, 2018

f-f commented Jul 7, 2018

wing328 commented Jul 7, 2018

langford commented Jul 9, 2018

wing328 commented Aug 5, 2018

f-f commented Aug 6, 2018

wing328 commented Aug 11, 2018

wing328 commented Aug 13, 2018

f-f commented Aug 13, 2018

[Clojure] Add model generation and conforming #122

[Clojure] Add model generation and conforming #122

Conversation

f-f commented May 21, 2018 • edited Loading

PR checklist

Description of the PR

wing328 commented May 24, 2018 • edited Loading

wing328 May 24, 2018

Choose a reason for hiding this comment

f-f May 26, 2018

Choose a reason for hiding this comment

wing328 May 26, 2018

Choose a reason for hiding this comment

wing328 May 24, 2018

Choose a reason for hiding this comment

f-f commented May 26, 2018

wing328 commented May 26, 2018

f-f commented May 26, 2018

jmini commented May 26, 2018

wing328 commented May 26, 2018

f-f commented May 27, 2018 • edited Loading

wing328 commented May 27, 2018

f-f commented May 27, 2018

wing328 commented May 28, 2018

f-f commented May 29, 2018

wing328 commented May 30, 2018

f-f commented May 30, 2018

f-f commented Jun 5, 2018

wing328 commented Jun 17, 2018

langford commented Jul 5, 2018

langford commented Jul 5, 2018

f-f commented Jul 7, 2018

wing328 commented Jul 7, 2018

langford commented Jul 9, 2018

wing328 commented Aug 5, 2018

f-f commented Aug 6, 2018

wing328 commented Aug 11, 2018

wing328 commented Aug 13, 2018

f-f commented Aug 13, 2018

f-f commented May 21, 2018 •

edited

Loading

wing328 commented May 24, 2018 •

edited

Loading

f-f commented May 27, 2018 •

edited

Loading