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.

Sign up for GitHub

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

Jump to bottom

Add publish_apidoc flag to SpecTree constructor #311

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add publish_apidoc flag to SpecTree constructor #311

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f0ea3c8
🔖 chore(spec.py): add publish_apidoc flag to SpecTree constructor
AndreasBBS May 14, 2023
7d585b6
🐛 fix(spectree): remove unnecessary app attribute from backend
AndreasBBS May 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions spectree/plugins/falcon_plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,11 @@ def __init__(self, spectree):
self.INT_ARGS_NAMES = ("num_digits", "min", "max")

def register_route(self, app: Any):
self.app = app
self.app.add_route(
app.add_route(
self.config.spec_url, self.OPEN_API_ROUTE_CLASS(self.spectree.spec)
)
for ui in self.config.page_templates:
self.app.add_route(
app.add_route(
f"/{self.config.path}/{ui}",
self.DOC_PAGE_ROUTE_CLASS(
self.config.page_templates[ui],
Expand All @@ -95,7 +94,7 @@ def find_node(node):
for child in node.children:
find_node(child)

for route in self.app._router._roots:
for route in self.spectree.app._router._roots:
find_node(route)

return routes
Expand Down
8 changes: 3 additions & 5 deletions spectree/plugins/starlette_plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,13 @@ def __init__(self, spectree):
self.conv2type = {conv: typ for typ, conv in CONVERTOR_TYPES.items()}

def register_route(self, app):
self.app = app

self.app.add_route(
app.add_route(
self.config.spec_url,
lambda request: JSONResponse(self.spectree.spec),
)

for ui in self.config.page_templates:
self.app.add_route(
app.add_route(
f"/{self.config.path}/{ui}",
lambda request, ui=ui: HTMLResponse(
self.config.page_templates[ui].format(
Expand Down Expand Up @@ -177,7 +175,7 @@ def parse_route(app, prefix=""):
else:
parse_route(route, prefix=f"{prefix}{route.path}")

parse_route(self.app)
parse_route(self.spectree.app)
return routes

def bypass(self, func, method):
Expand Down
5 changes: 4 additions & 1 deletion spectree/spec.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ def __init__(
validation_error_model: Optional[ModelType] = None,
naming_strategy: NamingStrategy = get_model_key,
nested_naming_strategy: NestedNamingStrategy = get_nested_key,
publish_apidoc: bool = True,
**kwargs: Any,
):
self.naming_strategy = naming_strategy
Expand All @@ -77,6 +78,7 @@ def __init__(
self.validation_error_model = validation_error_model or ValidationError
self.config: Configuration = Configuration.parse_obj(kwargs)
self.backend_name = backend_name
self.publish_apidoc = publish_apidoc
if backend:
self.backend = backend(self)
else:
Expand All @@ -95,7 +97,8 @@ def register(self, app: Any):
init step.
"""
self.app = app
self.backend.register_route(self.app)
if self.publish_apidoc:
self.backend.register_route(self.app)
Comment on lines +100 to +101
Copy link
Member

Choose a reason for hiding this comment

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

I think register_route is only used when we need to generate the OpenAPI doc.

You don't have to call this method if all you need is the validation. The code changes to the other two files look good to me.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can add this to the README HowTo part? I think this flag is not necessary unless we support more features like "only expose part of the doc". WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to me that other parts rely on the self.app inside spectree so I kept the register part of the code that sets that variable. For example the starlette_plugin.py uses it in find_routes. Tracking down the implications of removing the self.app was too complex for the fix I was trying to do so for safety I decided to keep it. If you are confident that it be the same as omitting the register(app) step then it's fine with me adding it to the README.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for your time. I have double-checked the code, all the self.app usages are related to generating the OpenAPI config. Without the spec.register(), we should be safe to use the validation feature without binding the OpenAPI endpoints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it also the case for the plugins?

Falcon uses self.spectree.app here:

spectree/spectree/plugins/falcon_plugin.py

Lines 87 to 101 in 7d585b6

def find_routes(self):
routes = []
def find_node(node):
if node.resource and node.resource.__class__.__name__ not in DOC_CLASS:
routes.append(node)
for child in node.children:
find_node(child)
for route in self.spectree.app._router._roots:
find_node(route)
return routes

Starlette uses self.spectree.app here:

spectree/spectree/plugins/starlette_plugin.py

Lines 143 to 179 in 7d585b6

def find_routes(self):
routes = []
def parse_route(app, prefix=""):
# :class:`starlette.staticfiles.StaticFiles` doesn't have routes
if not app.routes:
return
for route in app.routes:
if route.path.startswith(f"/{self.config.path}"):
continue
func = route.app
if isinstance(func, partial):
try:
func = func.__wrapped__
except AttributeError:
pass
if inspect.isclass(func):
for method in METHODS:
if getattr(func, method, None):
routes.append(
Route(
f"{prefix}{route.path}",
{method.upper()},
getattr(func, method),
)
)
elif inspect.isfunction(func):
routes.append(
Route(f"{prefix}{route.path}", route.methods, route.endpoint)
)
else:
parse_route(route, prefix=f"{prefix}{route.path}")
parse_route(self.spectree.app)
return routes

Are this find_routes methods also only for generating the OpenAPI config? I refactored this two plugins because they were using self.app on themselves and the BasePlugin does not have a self.app. I felt it wasn't very consistent across plugins and the app can still be accessed from self.spectree.app. This might be a change to keep.

If you give me the confirmation these find_routes methods on the plugins are also strictly for OpenAPI generation I'll go ahead and remove the changes and add a new entry to README.md documenting that omitting the register(app) will cause the OpenAPI routes to not be generated but the validation will still be enforced.

Also let me know if you want me to keep the self.app -> self.spectree.app refactor.

P.S.: No need to thank me for my time. It's my pleasure to be able to contribute to the tools I use.

Copy link
Member

Choose a reason for hiding this comment

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

  • find_routes is only used to generate the OpenAPI config.
  • Yes, they should not access self.app since there is no such attribute.
  • You're right. The implementation here is a bit ugly. SpecTree and BasePlugin can access each other. Need to refactor this part.
  • I'm okay with the self.app -> self.spectree.app refactor. To make it consistent, you can also choose to delete the app arg from register_root() and use self.spectree.app directly.


@property
def spec(self):
Expand Down