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

martian-re-frame martian instance has empty :handlers #93

Closed
enspritz opened this issue Aug 25, 2020 · 12 comments · Fixed by #94
Closed

martian-re-frame martian instance has empty :handlers #93

enspritz opened this issue Aug 25, 2020 · 12 comments · Fixed by #94

Comments

@enspritz
Copy link

Using martian-re-frame 0.1.13 and friends. The code sample in the martian-re-frame documentation fails with Uncaught Error: No protocol method ISet.-disjoin defined for type cljs.core/List: ([:my-operation-id {... params ...} :http-success :http-failure]). I noted issue #57 and now have martian initialization completing before performing a re-frame dispatch, but that still results ::http-failure being called with :unknown-route followed by the aforementioned error.

Inspecting the martian value in re-frame's app-db reveals that the :handlers key holds a blank seq. Wondering if this could be source of trouble, I tried feedingmartian.re-frame/init various swagger json documents, such as ones from the official examples repo, our own that we have on hand here, and martian/core/test-resources/swagger.json and martian/core/test-resources/swagger.json. Every document resulted in an empty :handlers. The :base-url changes appropriately, and the :interceptors seem to contain everything that it should, but :handlers is conspicuously empty. This is the case in Firefox and Google Chrome.

At the Clojure-based lein repl, these JSON files uniformly produce a perfectly usable martian, just as advertised, and :handlers is flush with the correct entries. Also, martian's own re-frame test suite cd re-frame && lein test passes 100%. I ran the test suite with the dependency versions in project.clj as tagged in martian v0.1.13 and also separately with the versions we are using here.

@oliyh
Copy link
Owner

oliyh commented Aug 25, 2020

Hi,

Thanks for letting me know. It sounds like there are possibly two issues here - the first problem relating to disjoin and the second with having no handlers (which very much sounds like a failed bootstrap).

The readme should be updated so that it doesn't guide you into repeating the error in #57, as well - I will update that.

There were some reasonably big changes regarding OpenAPI in the latest release and it's possible a mistake was made. Could you try 0.1.12 if possible to see if it worked then?

Many thanks

@oliyh
Copy link
Owner

oliyh commented Aug 25, 2020

Hi,

I've updated the example in the readme and ensured it works. Part of this was updating the example Swagger server, https://pedestal-api.herokuapp.com/, to allows cross origin requests. You should be able to do this in a REPL:

(martian/init "http://pedestal-api.herokuapp.com/swagger.json")
(martian.core/explore @(re-frame/subscribe [::martian/instance]))
;; => [[:all-pets Get all pets in the store] [:create-pet Create a pet] [:get-pet Get a pet by id] [:update-pet Update a pet] [:delete-pet Delete a pet by id]]

@enspritz
Copy link
Author

enspritz commented Aug 26, 2020

Just checking to see whether I'm running up against an issue patterned after #58 , seems like I am. All the documents I tested (as noted above) were served as text/plain. After changing the web server's behavior to server the OpenAPI definition with Content-Type: application/json;charset=UTF-8 I see that :handlers now contains paths.

Still receiving the uncaught error regarding ISet.-disjoin; I need to tease apart the issues here.

@enspritz
Copy link
Author

Much obliged. Cursory experimentation shows that martian-re-frame 0.1.12 appears to exhibit the same behavior regarding whether Content-Type is application/json or not.

@enspritz
Copy link
Author

The ISet.-disjoin error is resolved with you change in 0.1.14-SNAPSHOT.

@enspritz
Copy link
Author

Yes, material changes in OpenAPI 3... What I am attempting to do here is to consume an OpenAPI 3.0.1 JSON document with martian, but have found that parameters are not being recognized at all. This seems to be implicated in whatever is causing the :unknown-route failures now.

Using martian/explore to inspect at a CLJ REPL (and not CLJS), martian's core/test-resources/openapi.json file seems to work fine, but with my own file all parameters are empty maps. I'm now comparing the two files to iteratively eliminate differences and have parameters recognized, but to no avail so far.

@enspritz
Copy link
Author

Sample OpenAPI def and REPL session:

{
  "openapi": "3.0.1",
  "info": {
    "title": "My API",
    "version": "1"
  },
  "paths": {
    "/project/{projectKey}": {
      "get": {
        "summary": "Get specific values from a configuration for a specific project",
        "operationId": "getProjectConfiguration",
        "parameters": [
          {
            "name": "projectId",
            "in": "path",
            "description": "Project ID",
            "required": true,
            "schema": {
              "type": "string",
              "format": "string"
            }
          },
          {
            "name": "key",
            "in": "query",
            "description": "Obtains values corresponding to these keys from a project's configuration",
            "schema": {
              "type": "string",
              "format": "string"
            }
          }
        ],
        "responses": {
          "200": {
            "description": "Configuration for the specified project",
            "content": {
              "application/json": {
                "schema": {
                  "type": "string",
                  "format": "string"
                }
              }
            }
          },
          "403": {
            "description": "Refusing access to requested resource, perhaps due to insufficient privilege"
          },
          "404": {
            "description": "Requested resource was not found"
          }
      }
   }
}

In lein repl:

user=> (require '[martian.core :as martian])
nil
user=> (require '[martian.clj-http :as martian-http])
nil
user=> (def m (martian-http/bootstrap-openapi "http://localhost:8000/api"))
#'user/m
user=> (martian/explore m :get-project-configuration)
{:summary "Get specific values from a configuration for a specific project", :parameters {}, :returns {200 java.lang.String, 403 nil, 404 nil}}

@enspritz
Copy link
Author

Tried converting the OpenAPI 3 doc to Swagger 2 and directing martian to use that Swagger 2 output, and it seems to function correctly ^_^

me $ api-spec-converter --from=openapi_3 --to=swagger_2 --syntax=json ./src/api.json

In lein repl:

user=> (martian/explore m :get-project-configuration)
{:summary "Get specific values from a configuration for a specific project", :parameters {:project-key java.lang.String, #schema.core.OptionalKey{:k :key} (maybe java.lang.String)}, :returns {200 java.lang.String, 403 Any, 404 Any}}
user=> (martian/request-for m :get-project-configuration {:projectKey "kitty"})
{:method :get, :url "http://localhost:8000/api/project/123", :as :text, :headers {"Accept" "application/json"}}

@enspritz
Copy link
Author

So one thing I draw from this exercise is that either my OpenAPI document, while other tooling and the online validators are satisfied with it, requires a little more refinement in order to satisfy martian's needs, or, perhaps martian requires a little more TLC with regard to parsing OpenAPI 3 as you had mentioned..

@oliyh
Copy link
Owner

oliyh commented Aug 26, 2020

Hi,

Thank you so much for such a detailed investigation. The OpenAPI stuff is very new and I would imagine the issue lies there. Thanks for the small reproducible case - that really helps.

Martian should be able to cope with any valid schema, with the one caveat about operation-ids.

oliyh added a commit that referenced this issue Aug 26, 2020
oliyh added a commit that referenced this issue Aug 26, 2020
@oliyh oliyh closed this as completed in #94 Aug 26, 2020
oliyh added a commit that referenced this issue Aug 26, 2020
* Test for expected behaviour #93
* fixing poor keywordizing #93
@oliyh
Copy link
Owner

oliyh commented Aug 26, 2020

Hi,

There was indeed a mistake, I have pushed 0.1.14-SNAPSHOT which should fix your problem. If so I will do a new release. Thanks again for the report and the help reproducing!

@enspritz
Copy link
Author

I can confirm 0.1.14-SNAPSHOT fixes the params problem as well. Thank you kindly!

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 a pull request may close this issue.

2 participants