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

[ENH] Make root_path for app configurable #400

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

alyssadai
Copy link
Contributor

@alyssadai alyssadai commented Jan 8, 2025

Changes proposed in this pull request:

  • The root path for the FastAPI app is now configurable via an optional environment variable NB_NAPI_ROOT_PATH
    • Meant for use in deployment scenarios with proxy servers that use a stripped path prefix

To test the behaviour locally using NGINX, I followed these steps:

  1. Install NGINX:
sudo apt update && sudo apt install nginx
  1. Create an NGINX config file for the host in /etc/nginx/sites-available/fastapi_test:
server {
    listen 8080; # Nginx will listen on port 8080 locally

    location /api/ {
        proxy_pass http://127.0.0.1:8000/; # Forward to FastAPI app and strip `/api`
        proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
        proxy_set_header X-Forwarded-Host $host;
        proxy_set_header X-Forwarded-Prefix /api;
    }
}
  1. Apply changes
sudo service nginx restart
  1. Set environment variables including NB_NAPI_ROOT_PATH=/api and launch FastAPI app locally
  2. As a check, ensure that the following URLs work:

Checklist

This section is for the PR reviewer

  • PR has an interpretable title with a prefix ([ENH], [FIX], [REF], [TST], [CI], [MNT], [INF], [MODEL], [DOC]) (see our Contributing Guidelines for more info)
  • PR has a label for the release changelog or skip-release (to be applied by maintainers only)
  • PR links to GitHub issue with mention Closes #XXXX
  • Tests pass
  • Checks pass
  • If the PR changes the SPARQL query template, the default Neurobagel query file has also been regenerated

For new features:

  • Tests have been added

For bug fixes:

  • There is at least one test that would fail under the original bug conditions.

Summary by Sourcery

Make the root path for the app configurable via the NB_NAPI_ROOT_PATH environment variable. Update the documentation links to reflect the configured root path.

Enhancements:

  • Update the root endpoint to handle requests with or without a trailing slash.

Tests:

  • Add tests to verify that the documentation works correctly with the defined root path.

Copy link

sourcery-ai bot commented Jan 8, 2025

Reviewer's Guide by Sourcery

This pull request introduces a configurable root path for the FastAPI app using the environment variable NB_NAPI_ROOT_PATH. This enhancement addresses issues encountered with proxy servers that modify the path prefix during deployment. The root endpoint now correctly handles requests with or without a trailing slash, and documentation has been updated to reflect the configured root path.

Sequence diagram for request handling with configurable root path

sequenceDiagram
    participant Client
    participant NGINX
    participant FastAPI

    Note over Client,FastAPI: Example with root_path=/api

    Client->>NGINX: GET /api/docs
    NGINX->>FastAPI: GET /docs
    Note over NGINX,FastAPI: NGINX strips /api prefix
    FastAPI-->>NGINX: HTML response with /api/openapi.json
    NGINX-->>Client: HTML response

    Client->>NGINX: GET /api/openapi.json
    NGINX->>FastAPI: GET /openapi.json
    FastAPI-->>NGINX: OpenAPI JSON
    NGINX-->>Client: OpenAPI JSON
Loading

File-Level Changes

Change Details Files
Make root path configurable
  • Added NB_NAPI_ROOT_PATH environment variable to configure the root path of the FastAPI app.
  • Set the root_path parameter of the FastAPI instance to the value of NB_NAPI_ROOT_PATH.
  • Updated the root endpoint to use the configured root path when generating the link to the API documentation.
  • Added ROOT_PATH constant to store the environment variable name and value for the root path configuration in utility.py
app/main.py
app/api/utility.py
Update documentation links
  • Updated the overridden Swagger UI and Redoc HTML functions to use the configured root path when generating the link to the OpenAPI JSON file.
  • Ensured that the documentation links work correctly with the defined root path by updating the openapi_url parameter in the overridden_swagger and overridden_redoc functions to include the root_path from the request scope if available, defaulting to an empty string if not set
app/main.py
Handle trailing slash in root path
  • Added tests to verify that the root endpoint handles requests with or without a trailing slash correctly.
  • Added tests to verify that the documentation works correctly with the defined root path, including cases where the root path is not set correctly.
  • Updated the test_root function to use a monkeypatch fixture to set the root_path attribute of the app object, allowing for testing different root path configurations without modifying the application code directly
tests/test_routing.py

Assessment against linked issues

Issue Objective Addressed Explanation
#392 Add environment variable for the root path
#392 Set the root_path parameter when initializing FastAPI app
#356 Update the default value for NB_GRAPH_ADDRESS from '206.12.99.17' to 'localhost' or '127.0.0.1'
#356 Update the default value for NB_GRAPH_PORT from '5820' to '7200' for GraphDB

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@alyssadai alyssadai changed the title Make root_path for app configurable [ENH] Make root_path for app configurable Jan 8, 2025
@alyssadai alyssadai added the pr-patch Incremental feature improvement, will increment patch version when merged (0.0.+1) label Jan 8, 2025
Copy link

codecov bot commented Jan 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.11%. Comparing base (6298af7) to head (164ad5f).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #400      +/-   ##
==========================================
+ Coverage   96.95%   97.11%   +0.16%     
==========================================
  Files          24       24              
  Lines         820      831      +11     
==========================================
+ Hits          795      807      +12     
+ Misses         25       24       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alyssadai alyssadai marked this pull request as ready for review January 8, 2025 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-patch Incremental feature improvement, will increment patch version when merged (0.0.+1)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support subdirectory root paths when API is behind a proxy Replace old default values for ENV variables
1 participant