Replace use of apistar package with local API theme files #543
Conversation
WalkthroughThe changes introduced in this pull request enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant OpenAPISchema
participant Server
User->>API: Initialize API with openapi_theme and debug
API->>OpenAPISchema: Pass openapi_theme
API->>Server: Serve with debug mode
Server->>API: Start server in debug mode
API->>User: API is running
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
responder/statics.py (2)
1-1: Consider using a tuple for immutable constants.While the list is appropriate, using a tuple would better convey the immutable nature of this constant.
-API_THEMES = ["elements", "rapidoc", "redoc", "swagger_ui"] +API_THEMES = ("elements", "rapidoc", "redoc", "swagger_ui")
6-6: Consider enhancing security of DEFAULT_SECRET_KEY.While not part of the current changes, the default secret key "NOTASECRET" could pose a security risk if not properly overridden in production. Consider:
- Adding a warning in the documentation about changing this value
- Generating a random default key
- Adding runtime checks to prevent using the default in production
responder/ext/schema/__init__.py (1)
44-44: Consider adding debug logging for theme fallbackWhen an invalid theme is provided and falls back to the default, it would be helpful to log this for debugging purposes.
+import logging + +logger = logging.getLogger(__name__) + class OpenAPISchema: def __init__( # ... ): - self.docs_theme = api_theme if api_theme in API_THEMES else DEFAULT_API_THEME + if api_theme not in API_THEMES: + logger.debug(f"Invalid API theme '{api_theme}', falling back to {DEFAULT_API_THEME}") + self.docs_theme = DEFAULT_API_THEME + else: + self.docs_theme = api_themeresponder/api.py (3)
31-31: Enhance api_theme parameter documentation.While the documentation is clear, consider adding:
- The default theme value
- A reference to where these themes are defined
- :param api_theme: OpenAPI documentation theme, must be one of ``elements``, ``rapidoc``, ``redoc``, ``swagger_ui`` + :param api_theme: OpenAPI documentation theme, must be one of ``elements``, ``rapidoc``, ``redoc``, ``swagger_ui``. Defaults to ``elements``. + These themes are defined in ``responder/statics.py``.
360-361: Consider using dict.setdefault() for more concise code.The debug parameter handling can be simplified.
- if "debug" not in kwargs: - kwargs.update({"debug": self.debug}) + kwargs.setdefault("debug", self.debug)
Line range hint
20-361: Architecture Review: Clean implementation of local API themes.The changes effectively:
- Remove the apistar dependency by introducing local theme support
- Maintain backward compatibility through default values
- Properly propagate configuration through the object hierarchy
- Add debug mode support consistently across the application
The implementation is clean and aligns well with the PR objectives.
Consider adding a migration guide in the project's documentation to help users transition from apistar to the new local themes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- responder/api.py (6 hunks)
- responder/ext/schema/init.py (5 hunks)
- responder/ext/schema/docs/elements.html (1 hunks)
- responder/ext/schema/docs/rapidoc.html (1 hunks)
- responder/ext/schema/docs/redoc.html (1 hunks)
- responder/ext/schema/docs/swagger_ui.html (1 hunks)
- responder/statics.py (1 hunks)
- setup.py (0 hunks)
💤 Files with no reviewable changes (1)
- setup.py
✅ Files skipped from review due to trivial changes (4)
- responder/ext/schema/docs/elements.html
- responder/ext/schema/docs/rapidoc.html
- responder/ext/schema/docs/redoc.html
- responder/ext/schema/docs/swagger_ui.html
🧰 Additional context used
🪛 Ruff
responder/ext/schema/__init__.py
10-10:
responder.staticfiles.StaticFilesimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
🔇 Additional comments (8)
responder/statics.py (1)
3-3: Verify the theme name change impact.The theme name has been updated from "swaggerui" to "swagger_ui". While this provides better consistency, we should verify that this change doesn't break existing configurations.
✅ Verification successful
Let me gather more context by searching for theme-related configurations and usages.
Based on the search results, I can now generate the final response:
Theme name change is safe and consistent
The change from "swaggerui" to "swagger_ui" is safe because:
- No references to the old "swaggerui" theme name exist in the codebase
- The new "swagger_ui" value is properly listed in
API_THEMES- The code includes validation that ensures only themes from
API_THEMESare accepted (if api_theme in API_THEMES else DEFAULT_API_THEME)- The documentation correctly reflects the theme name as "swagger_ui" in API parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any hardcoded references to the old theme name that might need updating rg -i "swaggerui" --type pyLength of output: 27
Script:
#!/bin/bash # Search for API theme related configurations and usages rg -i "api.*theme" --type py -A 3 # Also search for swagger UI related configurations rg -i "swagger.*ui" --type py -A 3Length of output: 2556
responder/ext/schema/__init__.py (3)
30-30: LGTM: Well-structured parameter additionThe addition of the
api_themeparameter with a default value maintains backward compatibility while enabling theme customization.
55-56: LGTM: Secure and maintainable path handlingThe path resolution is implemented securely using
.resolve()and relative to the current file, which is a robust approach for managing local theme files.
55-56: Verify presence of all theme template filesLet's ensure all theme files mentioned in API_THEMES are present in the docs directory.
✅ Verification successful
Based on the execution results, I can generate the final response as we have verified the presence of all theme files.
All theme template files are present and correctly located
All required theme files (elements.html, rapidoc.html, redoc.html, swagger_ui.html) are present in the expected location under responder/ext/schema/docs directory. The template initialization in the code should work as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all theme template files exist # Test: Check if all theme HTML files exist in the docs directory echo "Checking theme files in docs directory..." for theme in elements rapidoc redoc swagger_ui; do fd -p "responder/ext/schema/docs/${theme}.html" doneLength of output: 636
responder/api.py (4)
20-20: LGTM: Clean import of theme constant.The import of
DEFAULT_API_THEMEfrom local.staticsmodule aligns with the PR objective of replacing the apistar package dependency.
58-58: LGTM: Clean parameter addition.The
api_themeparameter is correctly added with a default value, ensuring backward compatibility for existing code.
333-340: LGTM: Clean debug mode implementation.The debug parameter is:
- Well-documented in the method signature
- Correctly propagated to uvicorn.run()
Also applies to: 355-355
125-125: LGTM: Verify OpenAPISchema integration.The
api_themeis correctly propagated to the OpenAPISchema initialization. Let's verify the OpenAPISchema class accepts this parameter.✅ Verification successful
Let me try a different approach to verify the OpenAPISchema class definition and its constructor parameters.
These commands will help us:
- Find the OpenAPISchema class definition and its constructor
- Locate the file containing OpenAPISchema
- Find where api_theme is being used in Python files
This will give us a better understanding of whether the OpenAPISchema class actually accepts the api_theme parameter.
OpenAPISchema constructor does not match the usage
The verification reveals a mismatch between the code and its usage. From the search results:
- The OpenAPISchema constructor in
responder/ext/schema/__init__.pydoes not listapi_themein its primary parameters, yet:- The code in
responder/api.pyis passingapi_themeto OpenAPISchema initialization- However,
responder/ext/schema/__init__.pydoes showapi_themebeing used with a default value and being assigned toself.docs_themeLet me verify if it's an optional keyword argument by checking the full constructor definition.
The OpenAPISchema integration is correct
The verification confirms that the
api_themeparameter is properly defined in the OpenAPISchema constructor as an optional parameter withDEFAULT_API_THEMEas its default value. The code inresponder/api.pycorrectly passes this parameter to the OpenAPISchema initialization, maintaining consistency with the API's theme configuration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify OpenAPISchema constructor accepts api_theme parameter ast-grep --pattern 'class OpenAPISchema { $$$ def __init__($_, $$$, api_theme=$_) { $$$ } $$$ }'Length of output: 110
Script:
#!/bin/bash # Search for OpenAPISchema class definition and constructor rg "class OpenAPISchema" -A 10Length of output: 630
Script:
#!/bin/bash # Search for files containing OpenAPISchema fd OpenAPISchemaLength of output: 18
Script:
#!/bin/bash # Look for api_theme usage in Python files rg "api_theme" --type py -A 2Length of output: 865
Script:
#!/bin/bash # Get the full constructor definition of OpenAPISchema rg "def __init__" responder/ext/schema/__init__.py -A 15Length of output: 436
responder/ext/schema/__init__.py
Outdated
| from responder.staticfiles import StaticFiles | ||
| from responder.statics import API_THEMES, DEFAULT_API_THEME | ||
| from responder.templates import Templates |
There was a problem hiding this comment.
Remove unused StaticFiles import
The StaticFiles import is not used in this file. Consider removing it to keep the imports clean and relevant.
Apply this diff:
-from responder.staticfiles import StaticFiles
from responder.statics import API_THEMES, DEFAULT_API_THEME
from responder.templates import Templates📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from responder.staticfiles import StaticFiles | |
| from responder.statics import API_THEMES, DEFAULT_API_THEME | |
| from responder.templates import Templates | |
| from responder.statics import API_THEMES, DEFAULT_API_THEME | |
| from responder.templates import Templates |
🧰 Tools
🪛 Ruff
10-10:
responder.staticfiles.StaticFilesimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
| return self.templates.render( | ||
| f"{self.docs_theme}.html", | ||
| title=self.title, | ||
| version=self.version, |
There was a problem hiding this comment.
Add error handling for template rendering
The current implementation might raise unhandled exceptions if the template file is missing. Consider adding proper error handling.
@property
def docs(self):
- return self.templates.render(
- f"{self.docs_theme}.html",
- title=self.title,
- version=self.version,
- schema_url="/schema.yml",
- )
+ try:
+ return self.templates.render(
+ f"{self.docs_theme}.html",
+ title=self.title,
+ version=self.version,
+ schema_url="/schema.yml",
+ )
+ except jinja2.TemplateNotFound:
+ logger.error(f"Template not found for theme: {self.docs_theme}")
+ # Fallback to a basic template or raise a more descriptive error
+ raise ValueError(f"API documentation theme '{self.docs_theme}' is not properly installed")Committable suggestion was skipped due to low confidence.
responder/api.py
Outdated
| @@ -120,6 +122,7 @@ def __init__( | |||
| license=license, | |||
| openapi_route=openapi_route, | |||
| static_route=static_route, | |||
| api_theme=api_theme, | |||
There was a problem hiding this comment.
For clarity, this variable should called openapi_theme
There was a problem hiding this comment.
Thanks for the guidance. I used your https://github.com/kennethreitz/responder/commit/0874ce87e98b4f75ba6bcff157c3b437414bd1fa.patch, where it was like api_theme=api_theme.
Now, 1107f5d adjusts the code according to your suggestions. Is it correct?
responder/statics.py
Outdated
| DEFAULT_ENCODING = "utf-8" | ||
| DEFAULT_API_THEME = "swaggerui" | ||
| DEFAULT_API_THEME = "swagger_ui" |
There was a problem hiding this comment.
and this variable DEFAULT_OPENAPI_THEME
There was a problem hiding this comment.
1107f5d also renames to DEFAULT_OPENAPI_THEME, see comment above.
This has already been submitted by @tabotkevin with GH-480, but got lost for whatever reason.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
responder/api.py (2)
360-361: Consider using dict.setdefault for cleaner code.The debug parameter handling could be more concise using Python's dict.setdefault method.
- if "debug" not in kwargs: - kwargs.update({"debug": self.debug}) + kwargs.setdefault("debug", self.debug)
Based on the gathered information, I can now provide a final response as there's enough evidence about how debug mode is implemented and used in the codebase.
Debug mode security concerns confirmed - action required
The codebase shows several security considerations regarding debug mode:
Debug mode is propagated throughout the application, affecting multiple components:
- Server error middleware (
ServerErrorMiddleware, debug=debug)- Exception middleware (
ExceptionMiddleware, debug=debug)- Uvicorn server configuration (
uvicorn.run(..., debug=debug))There are no safeguards or validations to prevent debug mode in production:
- No environment-based checks for debug mode
- No warnings when debug is enabled
- No documentation about security implications
Recommendations:
- Add environment-based validation to prevent debug mode in production
- Add warning logs when debug mode is enabled
- Document security implications in the docstring
- Consider implementing a production check in the
serve()method🔗 Analysis chain
Line range hint
333-361: Verify debug mode security implications.The debug parameter can expose sensitive information if enabled in production. Consider:
- Adding a warning when debug is enabled
- Preventing debug mode in production environments
- Documenting security implications in the docstring
Let me gather more information about debug mode usage and configuration in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any existing debug mode validations or environment checks rg -i "debug.*mode.*production" --type py rg -i "production.*debug.*mode" --type pyLength of output: 87
Script:
#!/bin/bash # Check for debug-related configurations and environment checks rg -i "debug\s*=|debug_mode|DEBUG" --type py -A 2Length of output: 1906
Script:
#!/bin/bash # Look for environment-related configurations rg -i "env|environment|production" --type py -A 2Length of output: 2559
Script:
#!/bin/bash # Check if there's any documentation about debug mode fd README.md | xargs rg -i "debug"Length of output: 34
Script:
#!/bin/bash # Check for any configuration or settings files fd -e yml -e yaml -e ini -e conf | xargs catLength of output: 93
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- responder/api.py (6 hunks)
- responder/ext/schema/init.py (5 hunks)
- responder/ext/schema/docs/elements.html (1 hunks)
- responder/ext/schema/docs/rapidoc.html (1 hunks)
- responder/ext/schema/docs/redoc.html (1 hunks)
- responder/ext/schema/docs/swagger_ui.html (1 hunks)
- responder/statics.py (1 hunks)
- setup.py (0 hunks)
💤 Files with no reviewable changes (1)
- setup.py
🚧 Files skipped from review as they are similar to previous changes (6)
- responder/ext/schema/init.py
- responder/ext/schema/docs/elements.html
- responder/ext/schema/docs/rapidoc.html
- responder/ext/schema/docs/redoc.html
- responder/ext/schema/docs/swagger_ui.html
- responder/statics.py
🔇 Additional comments (3)
responder/api.py (3)
20-20: LGTM: Import of local theme constant.The import of
DEFAULT_OPENAPI_THEMEaligns with the PR's objective of replacing the apistar package with local theme files.
31-31: Consider parameter name consistency and docstring enhancement.For better clarity and consistency:
- Consider renaming
api_themetoopenapi_themeas suggested in previous reviews.- Enhance the docstring to include default value and link to theme documentation.
- :param api_theme: OpenAPI documentation theme, must be one of ``elements``, ``rapidoc``, ``redoc``, ``swagger_ui`` + :param openapi_theme: OpenAPI documentation theme (default: "swagger_ui"). Must be one of: + - ``elements``: Elements-based theme + - ``rapidoc``: RapiDoc theme + - ``redoc``: Redoc theme + - ``swagger_ui``: Swagger UI theme (default) + For more details, see the OpenAPI documentation themes guide.
333-340: LGTM: Debug mode implementation.The debug parameter is well-documented and correctly implemented, providing proper control over the application's debug mode through uvicorn.
Also applies to: 355-355
responder/api.py
Outdated
| @@ -120,6 +122,7 @@ def __init__( | |||
| license=license, | |||
| openapi_route=openapi_route, | |||
| static_route=static_route, | |||
| openapi_theme=api_theme, | |||
There was a problem hiding this comment.
Maintain consistent parameter naming.
The parameter is named api_theme in the API class but passed as openapi_theme to OpenAPISchema. This inconsistency could cause confusion. Consider using openapi_theme consistently throughout the codebase.
- openapi_theme=api_theme,
+ openapi_theme=openapi_theme, # Parameter should be renamed in __init__ signatureCommittable suggestion was skipped due to low confidence.
responder/api.py
Outdated
| @@ -54,6 +55,7 @@ def __init__( | |||
| cors=False, | |||
| cors_params=DEFAULT_CORS_PARAMS, | |||
| allowed_hosts=None, | |||
| api_theme=DEFAULT_OPENAPI_THEME, | |||
There was a problem hiding this comment.
Its still here, everywhere api_theme is used should be replaced with openapi_theme and same thing for DEFAULT_API_THEME to DEFAULT_OPENAPI_THEME
There was a problem hiding this comment.
Thanks for spotting the other spot has now been fixed. 80eebd5 includes all the renames.
Problem
apistar is not maintained any longer.
Details
This patch has already been submitted by @tabotkevin with GH-480, but got lost for whatever reason.