Skip to content

Add add_specs to api #553

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

alvarolopez
Copy link

This PR is basically #465 without the removal of the test.

Implements #464

@alvarolopez
Copy link
Author

Wrong PR.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.845% when pulling a7c910b on alvarolopez:specs into a8f3582 on noirbizarre:master.

@coveralls
Copy link

coveralls commented Nov 22, 2018

Coverage Status

Coverage increased (+0.02%) to 96.823% when pulling 378e13e on alvarolopez:specs into 9b9ad42 on noirbizarre:master.

@casparjespersen
Copy link

What is status on this?

@alvarolopez
Copy link
Author

alvarolopez commented Sep 5, 2019

@casparjespersen the code is working, but it is pending to be reviewed/accepted by a maintainer. Note that this is basically #465 including the unit test.

@j5awry
Copy link
Collaborator

j5awry commented Sep 5, 2019

I've approved, and will send it to the core team for a final check. Going through the ticket, it looks like @noirbizarre approved the general addition already. Code and tests look good to me.

Sorry for not responding / checking earlier. Quite a backlog accumulated before more maintainers were added, and we're still going through things.

@alvarolopez
Copy link
Author

@j5awry no problems, thans for taking care of this.

@SteadBytes
Copy link
Collaborator

SteadBytes commented Sep 8, 2019

The Swagger UI can already be disabled using doc=False:

import flask
from flask_restplus import Api, Resource

app = flask.Flask(__name__)

api = Api(app, version='1.0',
          title='test', doc=False)

ns1 = api.namespace('api/v1', description='test')

@ns1.route('/my-resource')
class MyResource(Resource):
    def get(self):
        return {"message": "hello"}


if __name__ == "__main__":
    app.run(debug=True)

Requesting localhost:5000 will return a 404.

However, swagger.json is still available - curl localhost:5000/swagger.json

{
    "swagger": "2.0",
    "basePath": "/",
    "paths": {
        "/api/v1/my-resource": {
            "get": {
                "responses": {
                    "200": {
                        "description": "Success"
                    }
                },
                "operationId": "get_my_resource",
                "tags": [
                    "api/v1"
                ]
            }
        },
        "/api/v2/my-resource": {
            "get": {
                "responses": {
                    "200": {
                        "description": "Success"
                    }
                },
                "operationId": "get_my_new_resource",
                "tags": [
                    "api/v2"
                ]
            }
        }
    },
    "info": {
        "title": "test",
        "version": "1.0"
    },
    "produces": [
        "application/json"
    ],
    "consumes": [
        "application/json"
    ],
    "tags": [
        {
            "name": "api/v1",
            "description": "test"
        },
        {
            "name": "api/v2",
            "description": "test"
        }
    ],
    "responses": {
        "ParseError": {
            "description": "When a mask can't be parsed"
        },
        "MaskError": {
            "description": "When any error occurs on mask"
        }
    }
}

Instead of adding another kwarg to the Api constructor, perhaps the existing doc parameter should be altered to not expose the Swagger Specs in addition to hiding the Swagger UI view?

Also it would be worth adding this change to the documentation -https://flask-restplus.readthedocs.io/en/stable/swagger.html#disabling-the-documentation

@alvarolopez
Copy link
Author

alvarolopez commented Sep 9, 2019

@SteadBytes from my point of view the Swagger UI and the specs (i.e. swagger.json) are two different assets with different purposes that need to be handled separately. I agree on the documentation point though.

@alvarolopez alvarolopez force-pushed the specs branch 3 times, most recently from d426f82 to 1ca98c6 Compare September 12, 2019 07:28
@j5awry
Copy link
Collaborator

j5awry commented Sep 13, 2019

I agree with @alvarolopez on this. While the swagger GUI uses the generated swagger.json, they are distinct entities as far as end user cases. In that vein, I think we should ensure the other cases around swagger.json are discussed, tested, and we are clear on the outcomes:

  1. Is there a case when hiding swagger.json from users is expected, but it is viewable in the GUI?
  2. Does the current use of hiding swagger.json also remove it from the GUI?

@alvarolopez alvarolopez force-pushed the specs branch 2 times, most recently from 46795b3 to b64b9a3 Compare September 17, 2019 08:56
@SteadBytes
Copy link
Collaborator

@j5awry @alvarolopez , yes that's a good point. On second thoughts I agree with that. @j5awry, I can answer 2 for you; yes, the Swagger UI is not available if swagger.json is hidden. Visiting the URL for Swagger UI returns a 404

@alvarolopez
Copy link
Author

@SteadBytes well, I guess this is a side-effect of removing the swagger.json that I find difficult to circumvent. I would just document that disabling specs removes the web UI as well.

@SteadBytes
Copy link
Collaborator

Yes, removing swagger.json does mean that the Swagger UI shouldn't be shown. I don't think it makes sense to try and show an empty Swagger UI. As long as it is clearly documented that is the behaviour I would expect.

However I think @j5awry was also asking whether we should support a possible use case of showing the Swagger UI but not directly exposing swagger.json? IMO that isn't something we should try to support. It would also mean that we'd have to change how the Swagger UI retrieves the specs to build the page as they wouldn't be publicly available.

This flag allows to select if specs are added into the API or not.

Fixes noirbizarre#464
@alvarolopez
Copy link
Author

So, I have added the documentation and also disabled spec generation.

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.

6 participants