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

Fix issue with missing type parameter #650

Closed
wants to merge 1 commit into from

Conversation

ianmacl
Copy link

@ianmacl ianmacl commented Oct 8, 2014

If the swagger definition is missing the type parameter it kills the UI.

@webron
Copy link
Contributor

webron commented Oct 8, 2014

Two things - if a parameter doesn't have a type (assuming it's Swagger 2.0 and not a body parameter), it's an illegal declaration and the ui should be dead for all that matters.

The second thing, this should be opened against swagger-js.

@ianmacl
Copy link
Author

ianmacl commented Oct 8, 2014

While true that it is invalid, I disagree that the UI should be dead for all that matters. I think being a little forgiving is ok, and IMO, if you can prevent your entire UI from breaking because of one syntax error without providing any clues as to why, then that's a good thing. I did notice the swagger-js repo after. I'll open the pull request against there.

@ianmacl ianmacl closed this Oct 8, 2014
@webron
Copy link
Contributor

webron commented Oct 8, 2014

It can't ignore the problem and display the UI as is, because it wouldn't know what do display for that parameter, and would also break the try it now and so on. This would be an issue for swagger-js as well because it would break applications that use it to read swagger specs. I agree it should give an error to the user, but that's about it.

@ianmacl
Copy link
Author

ianmacl commented Oct 8, 2014

I'm more saying that if resource A has an operation that has a parameter that is missing a type, then one should still be able to pull down resource B and see it's list of operations. I would think that the proper behaviour would be to report an error message and skip the parameter (or display Unknown as the type and assume string?)

As it is, if any one of your resources is missing a type, then you cannot see the list of operations for any of your resources.

@webron
Copy link
Contributor

webron commented Oct 8, 2014

Again, I agree with the error message, though we have now a validation tool to give you the errors on a given specification file. Personally, I prefer the tools being more strict (again, that's my preference) and failing on malformed specs.

@ianmacl
Copy link
Author

ianmacl commented Oct 8, 2014

Pardon my ignorance... can you point me to the validator? I did some googling but could not locate it.

@fehguy
Copy link
Contributor

fehguy commented Oct 8, 2014

@ianmacl
Copy link
Author

ianmacl commented Oct 8, 2014

Thanks fehguy. I submitted a pull request to add documentation on /validator/debug.

@atomgomba
Copy link

Parameters of type file are causing an exception in the current swagger-ui (6b448c1) at line 537 in swagger-client.js. If parameter type is file then getType() function returns undefined and innerType.toString() will throw an exception. I had to add to the if statement in getType() this code else if (type == "file") { return type; } in order to get my API doc displayed.

@webron
Copy link
Contributor

webron commented Oct 16, 2014

@atomgomba - can you share a sample spec? while obviously it shouldn't die, I also want to make sure the spec is right.

@atomgomba
Copy link

Yes, here it is:
http://pastebin.com/AC8cB8C1
Please note the swagger field is an integer because of that other bug and it was simpler for me to make the UI work.

@webron
Copy link
Contributor

webron commented Oct 16, 2014

okay, that looks valid. However, it would be better to open a dedicated issue for this problem.

@atomgomba
Copy link

Sir, consider it done: #662

@webron
Copy link
Contributor

webron commented Oct 16, 2014

Thanks. And please, calling me "Sir" just makes me feel old ;)

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.

4 participants