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

Allow for multiple SwaggerDocs in a single web.Application. #104

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

Conversation

garyvdm
Copy link
Contributor

@garyvdm garyvdm commented Oct 14, 2022

No description provided.

@codecov
Copy link

codecov bot commented Oct 14, 2022

Codecov Report

Base: 98.77% // Head: 98.77% // No change to project coverage 👍

Coverage data is based on head (58d5bec) compared to base (705ee77).
Patch has no changes to coverable lines.

❗ Current head 58d5bec differs from pull request most recent head f894dfe. Consider uploading reports for the commit f894dfe to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #104   +/-   ##
=======================================
  Coverage   98.77%   98.77%           
=======================================
  Files          14       14           
  Lines        1302     1302           
=======================================
  Hits         1286     1286           
  Misses         16       16           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@hh-h
Copy link
Owner

hh-h commented Oct 14, 2022

Hello, thank you for your PR. Could you please describe the case it solves?

@garyvdm
Copy link
Contributor Author

garyvdm commented Oct 14, 2022

We have an app that has an api that we need to change. Our plan is to have a v1 api, and a v2 api, with the v1 api deprecated. (We have users of the v1 api that won't be able to change immediately, and so we will need to still support the v1 api with compatibility code.)

I would like to provide 2 swagger docs for the 2 apis version, from the same aiohttp app.

@hh-h
Copy link
Owner

hh-h commented Oct 18, 2022

Thank you for the explanation, I'll take a look.

@garyvdm garyvdm force-pushed the multi-tenant branch 2 times, most recently from 5ed8c29 to 915102c Compare November 7, 2022 16:40
aiohttp_swagger3/swagger.py Outdated Show resolved Hide resolved
aiohttp_swagger3/swagger.py Outdated Show resolved Hide resolved
aiohttp_swagger3/ui_settings.py Outdated Show resolved Hide resolved
@garyvdm
Copy link
Contributor Author

garyvdm commented Dec 15, 2022

Sorry for the delay. I had a break from work.

Not sure why the builds are failing. I don't think it related change, but I'm investigating.

@hh-h hh-h closed this Jan 29, 2023
@hh-h hh-h reopened this Jan 29, 2023
@garyvdm
Copy link
Contributor Author

garyvdm commented Jan 30, 2023

Woot, thank you for fixing the CI .

I've rebased to the latest from master.

Copy link
Owner

@hh-h hh-h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, I want to apologize for the very long review. I took the time to read the changes and I want to say that it is a very good idea and implementation.

Now you can have conflicts between swagger instances, you should add a check for conflicts right below https://github.com/hh-h/aiohttp-swagger3/pull/104/files#diff-a2cd31a0003fe58689bade08ea5f0a88483ea36acd0569c0f47b90c0c39f6da2R100

Something like this

for route in self._app.router.routes():
    if ui_path == route.resource.canonical:
        raise Exception(f"I'm very bad at wording, please come up with something using {ui_path} and {route.resource.canonical}, thanks :)")

And you should also add a test for this change.

Also please add a usage example in examples/ but I wrote it for you, so just put it there :)

from aiohttp import web

from aiohttp_swagger3 import RapiDocUiSettings, ReDocUiSettings, SwaggerDocs, SwaggerUiSettings


async def handler(request):
    """
    ---
    responses:
      '200':
        description: OK.

    """
    return web.json_response()


def main():
    app = web.Application()
    swagger1 = SwaggerDocs(
        app,
        swagger_ui_settings=SwaggerUiSettings(path="/swagger1"),
        redoc_ui_settings=ReDocUiSettings(path="/redoc1"),
        rapidoc_ui_settings=RapiDocUiSettings(path="/rapidoc1"),
    )
    swagger1.add_routes([web.get("/handler1", handler, allow_head=False)])

    swagger2 = SwaggerDocs(
        app,
        swagger_ui_settings=SwaggerUiSettings(path="/swagger2"),
        redoc_ui_settings=ReDocUiSettings(path="/redoc2"),
        rapidoc_ui_settings=RapiDocUiSettings(path="/rapidoc2"),
    )
    swagger2.add_routes([web.get("/handler2", handler, allow_head=False)])

    web.run_app(app)


if __name__ == "__main__":
    main()



async def _swagger_spec(request: web.Request) -> web.Response:
async def _swagger_spec(spec: Any, request: web.Request) -> web.Response:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace Any with Dict


async def _rapidoc_ui(request: web.Request) -> web.Response:
return web.Response(text=request.app[_RAPIDOC_UI_INDEX_HTML], content_type="text/html")
async def _ui(ui_text: str, request: web.Request) -> web.Response:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please, rename it to _ui_handler

assert resp1_json["info"]["title"] == "test app 1"
assert "/handler1" in resp1_json["paths"]

resp1 = await client.get("/docs2/swagger.json")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be resp2

assert resp1.status == 200
resp1_json = await resp1.json()
assert resp1_json["info"]["title"] == "test app 1"
assert "/handler1" in resp1_json["paths"]
Copy link
Owner

@hh-h hh-h Feb 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also check that /handler2 not it resp1_json["paths"]

assert resp1.status == 200
resp1_json = await resp1.json()
assert resp1_json["info"]["title"] == "test app 2"
assert "/handler2" in resp1_json["paths"]
Copy link
Owner

@hh-h hh-h Feb 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also check that /handler1 not it resp2_json["paths"]

@hh-h hh-h self-assigned this Feb 4, 2023
Repository owner deleted a comment Jan 23, 2024
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.

2 participants