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

Specifying multiple protocols doesn't work #165

Closed
sebastianhaas opened this issue May 31, 2016 · 16 comments
Closed

Specifying multiple protocols doesn't work #165

sebastianhaas opened this issue May 31, 2016 · 16 comments
Labels

Comments

@sebastianhaas
Copy link

Specifying multiple protocols for loopback-component-explorer like

{
  "loopback-component-explorer": {
    "mountPath": "/explorer",
    "host": "localhost:3000",
    "protocol": ["http", "https"]
  }
}

results in an invalid swagger.json:

...
"host":"localhost:3000",
"basePath":"/api",
"schemes":[["http","https"]]
...

I doubt this behaviour is intended because of https://github.com/strongloop/loopback-swagger/blob/master/lib/specgen/swagger-spec-generator.js#L130 as well as swagger-api/swagger-ui#1006.

Also see http://swagger.io/specification/, section Schema states that field schemes is an array of strings.

I will create a PR to fix it if it is confirmed as a bug.

@richardpringle
Copy link
Contributor

That doesn't look right. @sebastianhaas how are you producing the swagger though, I'm not sure that I've seen the component-explorer path produced in a swagger file before...

@sebastianhaas
Copy link
Author

sebastianhaas commented Aug 11, 2016

how are you producing the swagger though

I'm not sure if that was your question, but I just navigated to /explorer/swagger.json. Given the configuration above, it will result in the invalid output.

@richardpringle
Copy link
Contributor

@sebastianhaas, can you for the loopback-sandbox and write the steps to reproduce in the readme?

Thanks in advance.

@sebastianhaas
Copy link
Author

@richardpringle richardpringle added bug and removed triage labels Aug 17, 2016
@richardpringle
Copy link
Contributor

@sebastianhaas, there's definitely something wrong here... but I'm not sure that fixing the extra set of brackets addresses much.

What are you doing with the swagger.json file? Are you aware of the API definition generator?

I'm not sure that the schemes there are entirely relevant to your API in general...

In any case, there should not be an extra set of brackets, so if you want to make the fix, I will try to make sure that the PR gets landed.

@sebastianhaas
Copy link
Author

I wasn't aware of the API definition generator. However, I don't really see how this matters in regard to the bug I reported. I tried using it, it seems to be broken anyway.

I'm using swagger.json to auto-generate an Angular 2 client using swagger-codegen.
So yeah. I would prefer if this gets fixed, I mean, why wouldn't it, this is clearly out of specification and you guys are promoting loopback's swagger support quite blatant :)

@richardpringle
Copy link
Contributor

I don't believe that "schemes" is doing anything in the context of the explorer. It's not taking into consideration the scheme of the API itself, it's just being set based on the component-explorer configuration.

Even though the swagger.json is "mostly" working for you, that's not the recommended way of grabbing the specification for your API. I think there might be some flaws that you run into if your API gets more complex.

What was broken with the export-api-def?

@sebastianhaas
Copy link
Author

I don't believe that "schemes" is doing anything in the context of the explorer. It's not taking into consideration the scheme of the API itself, it's just being set based on the component-explorer configuration.

That might be true, but I wouldn't bet on it. The explorer is using the swagger file itself to determine the endpoints etc., so I suspect the protocol does matter.

Just for the sake of completeness, the actual swagger scheme is built in loopback-swagger, with the relevant code for the protocols somewhere here:
https://github.com/strongloop/loopback-swagger/blob/63f734c87704ca9b60e4c9f404b3e733ebe67f5a/lib/specgen/swagger-spec-generator.js#L136

Even though the swagger.json is "mostly" working for you, that's not the recommended way of grabbing the specification for your API.

What would the recommended way be? export-api-def is using https://github.com/strongloop/loopback-api-definition internally, which exports a swagger file, so I really don't see the difference or am I missing something?

@superkhau superkhau removed the bug label Dec 7, 2016
@sebastianhaas
Copy link
Author

@superkhau how can this not be an issue? Afaik there is no way to specify protocols for API definition files, so using loopback:api-def-generator always includes post processing which is needlessly annoying.

Just allow passing options in at https://github.com/strongloop/loopback-api-definition/blob/master/lib/get-api-def.js#L23 so we can at least specify one protocol + host properly.

@superkhau
Copy link
Contributor

superkhau commented Dec 29, 2016

@sebastianhaas I purposely removed the bug label so someone can take over and confirm the bug and take over where @richardpringle left off as he is not at IBM anymore. "unlabeled" lets us know someone needs to look at this -- when someone decides to take this issue on, you will see the "triage" label applied at that time.

That said, after reading your discussion above. It sounds like a bug -- array of strings for protocol looks correct and matching the swagger spec you linked above. Just need someone to slap a bug label on it after cloning your provided repo to confirm.

@stale
Copy link

stale bot commented Aug 23, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@sebastianhaas
Copy link
Author

I think this is still an issue that needs to be addressed.

@stale stale bot removed the stale label Aug 24, 2017
@stale
Copy link

stale bot commented Oct 23, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Oct 23, 2017
@sebastianhaas
Copy link
Author

Still open and important.

@stale stale bot removed the stale label Oct 23, 2017
@stale
Copy link

stale bot commented Dec 22, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Dec 22, 2017
@stale
Copy link

stale bot commented Jan 5, 2018

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

@stale stale bot closed this as completed Jan 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants