Add support for absolute OpenAPI server URLs

[bombaywalla]

I believe this PR should address issue #116 . At least for the CLJ clients. Currently openapi/base-url picks the first server listed in the servers field. To allow the selection of any server, I added the capability of providing a :server-url key in the opts parameter of martian-http/bootstrap-openapi.

I have made the changes to each CJ client and added tests for each of them.

I have not yet made the corresponding changes to the CLJS clients or added tests for them. I am not sure how the tests for the CLJS clients are supposed to work. I ran lein test on the CLJ clients just fine. Running lein test from the cljs-http directory results in

Syntax error (NoSuchMethodError) compiling at (figwheel/main.cljc:1:1).
'java.util.stream.Collector com.google.common.collect.ImmutableSortedSet.toImmutableSortedSet(java.util.Comparator)'

I'd appreciate any guidance on running tests for CLJS clients.

[oliyh]

Hello,

lein test should be all you need. Alternatively you could try starting a cljs repl in the project and visiting the auto testing page at http://localhost:9500/test.html

(You will also need to start the stub server using the code in martian.runner)

The error seems to be with figwheel compilation though so that might not help. How about lein fig, does that fail the same way?

[bombaywalla]

lein fig fails the same way.

In poking around a bit, I tried lein with-profiles dev test from the clj-http directory and that got me a bit further. (Is that the right thing to do? Have not used lein in a long time :-)

That resulted in

[Figwheel:WARNING] Attempting to dynamically add "target" to classpath!
[Figwheel:WARNING] Target directory "target" is not on the classpath
[Figwheel:WARNING] Please fix this by adding "target" to your classpath
 I.E.
 For Clojure CLI Tools in your deps.edn file:
    ensure "target" is in your :paths key

 For Leiningen in your project.clj:
   add it to the :resource-paths key

[Figwheel] Compiling build test to "target/public/cljs-out/test-main.js"
[Figwheel] Successfully compiled build test to "target/public/cljs-out/test-main.js" in 18.527 seconds.
Launching Javascript environment with script:  ["/opt/google/chrome/google-chrome" "--headless" "--disable-gpu" "--remote-debugging-port=9222" "--no-sandbox" :open-url]
Exception in thread "main" java.io.IOException: Cannot run program "/opt/google/chrome/google-chrome": error=2, No such file or directory, compiling:(/private/var/folders/lk/mf8f6ghs0zqcw70nmy739khm0000gn/T/form-init7172352567084510349.clj:1:125)

So, it looks like I need to install google-chrome. Do I also need to add target to the :resource-paths key?

[oliyh]

Ah, yes it's configured to use Google Chrome to run the tests. Not sure how it relates to the error you see but it if it gets you further then it will help!

[bombaywalla]

I'm on a Mac. I made the following change to test.cljs.edn (so that it would pick up Chrome from the Applications directory).

dorabs-imac:cljs-http dorab$ git diff test.cljs.edn
diff --git a/cljs-http/test.cljs.edn b/cljs-http/test.cljs.edn
index a388efa..d063267 100644
--- a/cljs-http/test.cljs.edn
+++ b/cljs-http/test.cljs.edn
@@ -2,5 +2,6 @@
   ;; use an alternative landing page for the tests so that we don't
   ;; launch the application
   :open-url "http://[[server-hostname]]:[[server-port]]/test.html"
-  :launch-js ["/opt/google/chrome/google-chrome" "--headless" "--disable-gpu" "--remote-debugging-port=9222" "--no-sandbox" :open-url]}
+  ;; :launch-js ["/opt/google/chrome/google-chrome" "--headless" "--disable-gpu" "--remote-debugging-port=9222" "--no-sandbox" :open-url]}
+  :launch-js ["/Applications/Google Chrome.app/Contents/MacOS/Google Chrome" "--headless" "--disable-gpu" "--remote-debugging-port=9222" "--no-sandbox" :open-url]}
 {:main martian.test-runner}

and then ran lein with-profiles dev test from the cljs-http directory. The tests passed. However. when I deliberately change the test (expecting it to fail), it does not fail. Any suggestions?

A transcript of the session is below.

dorabs-imac:cljs-http dorab$ pwd
/Users/dorab/Projects/martian/cljs-http
dorabs-imac:cljs-http dorab$ lein clean
dorabs-imac:cljs-http dorab$ git diff test/martian/cljs_http_test.cljs
dorabs-imac:cljs-http dorab$ lein with-profiles dev test
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
[Figwheel:WARNING] Attempting to dynamically add "target" to classpath!
[Figwheel:WARNING] Target directory "target" is not on the classpath
[Figwheel:WARNING] Please fix this by adding "target" to your classpath
 I.E.
 For Clojure CLI Tools in your deps.edn file:
    ensure "target" is in your :paths key

 For Leiningen in your project.clj:
   add it to the :resource-paths key

[Figwheel] Compiling build test to "target/public/cljs-out/test-main.js"
[Figwheel] Successfully compiled build test to "target/public/cljs-out/test-main.js" in 17.491 seconds.
Launching Javascript environment with script:  ["/Applications/Google Chrome.app/Contents/MacOS/Google Chrome" "--headless" "--disable-gpu" "--remote-debugging-port=9222" "--no-sandbox" :open-url]
Environment output being logged to: target/public/cljs-out/test/js-environment.log

Testing martian.cljs-http-test

Ran 2 tests containing 2 assertions.
0 failures, 0 errors.
:figwheel.main.testing/success
dorabs-imac:cljs-http dorab$ # Change the test to fail
dorabs-imac:cljs-http dorab$ git diff test/martian/cljs_http_test.cljs
diff --git a/cljs-http/test/martian/cljs_http_test.cljs b/cljs-http/test/martian/cljs_http_test.cljs
index de60fd9..c5c8d59 100644
--- a/cljs-http/test/martian/cljs_http_test.cljs
+++ b/cljs-http/test/martian/cljs_http_test.cljs
@@ -27,7 +27,7 @@
   (async done
          (go (let [m (<! (martian-http/bootstrap-openapi "http://localhost:8888/openapi.json"))]

-               (is (= "http://localhost:8888/openapi/v3"
+               (is (= "xhttp://localhost:8888/openapi/v3"
                       (:api-root m)))

                (is (contains? (set (map first (martian/explore m)))
dorabs-imac:cljs-http dorab$ lein with-profiles dev test
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
[Figwheel:WARNING] Attempting to dynamically add "target" to classpath!
[Figwheel:WARNING] Target directory "target" is not on the classpath
[Figwheel:WARNING] Please fix this by adding "target" to your classpath
 I.E.
 For Clojure CLI Tools in your deps.edn file:
    ensure "target" is in your :paths key

 For Leiningen in your project.clj:
   add it to the :resource-paths key

[Figwheel] Compiling build test to "target/public/cljs-out/test-main.js"
[Figwheel] Successfully compiled build test to "target/public/cljs-out/test-main.js" in 18.025 seconds.
Launching Javascript environment with script:  ["/Applications/Google Chrome.app/Contents/MacOS/Google Chrome" "--headless" "--disable-gpu" "--remote-debugging-port=9222" "--no-sandbox" :open-url]
Environment output being logged to: target/public/cljs-out/test/js-environment.log

Testing martian.cljs-http-test

Ran 2 tests containing 2 assertions.
0 failures, 0 errors.
:figwheel.main.testing/success
dorabs-imac:cljs-http dorab$ 

[bombaywalla]

How do you run tests? Especially for a particular module? (e.g., cljs-http) Please share with me the command you use, or the one that I should use. I tried cd cljs-http; lein with-profile dev,test test. That "works", but with the issue mentioned above. I tried several different approaches, but am not getting anywhere. Thanks.

[oliyh]

Hi,

Yes, that should be all you need (you don't strictly need the Dev profile bit either, that is implied). You do see the testing output so it is running, but it's possible that the test is not written correctly and it's not running the assertion that you broke.

I'll try to find some time to verify this, but don't let it hold you back for now.

Thanks

[bombaywalla]

Thanks for the response.

I think there is some problem with the CLJS tests. Most likely in the way I am running them, or something to do with the async nature of the tests. One clue is that the test output says Ran 2 tests containing 2 assertions.. In reality, there are 2 tests and 4 assertions. If I add another deftest to the test file, it prints out Ran 3 tests containing 2 assertions.. So, the number of tests is correct, but the number of assertions seems to be just for the first test. Perhaps another clue is that if I add

(defmethod cljs.test/report [:cljs.test/default :end-run-tests] [m]
  (if (cljs.test/successful? m)
    (println "SUCCESS SUCCESS SUCCESS!")
    (println "FAIL FAIL FAIL")))

to the end of cljs_http_test.cljs I do then get the intentionally modified test to fail. But, the failure is an exception, and the reporting still is incorrect.

Sorry I do not have much exposure to CLJS so cannot provide better diagnostics or insights. I will continue with the PR for the CLJS modules. Thanks.

[bombaywalla]

By the way, for me, the dev profile is necessary. Without it, figwheel cannot be found (see the first message in this thread/PR).

[bombaywalla]

I am now almost sure that only the first deftest is being run. Instead of modifying the second deftest to fail, if I modify the first deftest to fail, the tests indeed do fail.

[bombaywalla]

I believe I have located the problem with the cljs-http tests. The bootstrap-openapi-test has the (done) inside the (async ...) rather than under the (go ...).

You could just fix this in the current SNAPSHOT, or I could add that fix to this PR (my preference), or I could make a new PR to fix the test. Let me know which way you'd like to proceed.

[bombaywalla]

I notice that cljs-http and cljs-http-promise have an optional parameter :trim-base-url? to the function bootstrap-openapi. That extra parameter is not on any of the CLJ modules. Should it be? If so, do you want those changes as part of this PR or a separate one? Let me know how you'd like for me to proceed.

[bombaywalla]

I notice that cljs-http and clj-http-promise use [clojure.string :as str] whereas all the other modules use [clojure.string :as string]. Since I am making changes in these files would you prefer that I change the cljs-http and cljs-http-promise modules to match all the others for consistency (my preference), or leave them as is?

[oliyh]

Hello,

Firstly thank you for your persistence with this, I really appreciate you fixing up my slightly dodgy code :)

I am very much in favour of consistency, so please fix imports as you describe.

It sounds like you've found the issue with the async tests, I suspected it would be something like that, please feel free to fix in this PR.

For trim-base-url I've forgotten what it's for. I'd leave it for now, if it seems to be a glaring inconsistency we can have another issue/PR to add it to clj.

Many thanks

[bombaywalla]

You're welcome. I'm learning about CLJS and promesa, so all is good. Thanks for martian in the first place.

:trim-base-url seems to remove a trailing slash from the base URL. I don't see it used anywhere (not even in tests). I'd make a guess that some OpenAPI/Swagger definitions mistakenly might have a trailing slash in the base URLs and this was an attempt to allow a martian user to fix that during use. I am leaving it as is for now, so it only applies to CLJS.

[bombaywalla]

This PR is ready for review (and merge if all is okay).

From my reading of the code, I did not need to make any changes to the re-frame and vcr modules. If I do, please let me know and I shall update the PR.

[oliyh]

Yes the purpose of base-url should just be to return the common root URL of the API, so makes sense to enhance it with this functionality.

Thanks

[bombaywalla]

The requested changes have been made. Please re-review this PR. Thanks.

[bombaywalla]

Thanks. I believe this PR addresses #116 which can now be closed.