diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 74f4a8258..2092c664b 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -29,6 +29,14 @@ endif::[] //===== Bug fixes // +[[release-notes-unreleased]] +==== Unreleased + +[float] +===== Bug fixes + +* Fix ``AttributeError`` on FastAPI >= 0.137 when resolving transaction names for routes added via ``include_router()`` + [[release-notes-6.x]] === Python Agent version 6.x diff --git a/elasticapm/contrib/starlette/__init__.py b/elasticapm/contrib/starlette/__init__.py index 3dfb225c9..c4e2af7d2 100644 --- a/elasticapm/contrib/starlette/__init__.py +++ b/elasticapm/contrib/starlette/__init__.py @@ -53,6 +53,42 @@ logger = get_logger("elasticapm.errors.client") +try: + # FastAPI >= 0.137.2 exposes a public helper that flattens routes added via + # include_router() (nested under _IncludedRouter in 0.137) into matchable + # RouteContext objects. Older versions don't have it; see _flatten_routes(). + from fastapi.routing import iter_route_contexts +except ImportError: + iter_route_contexts = None + + +def _flatten_routes(routes): + """ + Yield the matchable routes from an app's route list. + + FastAPI 0.137 nests routes added via include_router() under _IncludedRouter + tree nodes, which expose no ``path`` attribute. They have to be flattened + into their effective route contexts (each providing matches() and the full + templated path) before they can be matched against a scope. + + FastAPI >= 0.137.2 provides the public iter_route_contexts() for this, which + also wraps plain routes uniformly. On older versions fall back to the + private _IncludedRouter.effective_route_contexts(), and on FastAPI < 0.137 + (no _IncludedRouter) the routes are already matchable as-is. + """ + if any(hasattr(route, "effective_route_contexts") for route in routes): + if iter_route_contexts is not None: + yield from iter_route_contexts(routes) + return + for route in routes: + if hasattr(route, "effective_route_contexts"): + yield from route.effective_route_contexts() + else: + yield route + return + for route in routes: + yield route + def make_apm_client(config: Optional[Dict] = None, client_cls=Client, **defaults) -> Client: """Builds ElasticAPM client. @@ -268,10 +304,12 @@ def get_route_name(self, request: Request) -> str: return route_name def _get_route_name(self, scope, routes, route_name=None): - for route in routes: + for route in _flatten_routes(routes): match, child_scope = route.matches(scope) if match == Match.FULL: - route_name = route.path + route_name = getattr(route, "path", None) + if route_name is None: + continue child_scope = {**scope, **child_scope} if isinstance(route, Mount) and route.routes: child_route_name = self._get_route_name(child_scope, route.routes, route_name) @@ -281,4 +319,6 @@ def _get_route_name(self, scope, routes, route_name=None): route_name += child_route_name return route_name elif match == Match.PARTIAL and route_name is None: - route_name = route.path + partial_path = getattr(route, "path", None) + if partial_path is not None: + route_name = partial_path diff --git a/tests/contrib/asyncio/fastapi_tests.py b/tests/contrib/asyncio/fastapi_tests.py new file mode 100644 index 000000000..f6111040c --- /dev/null +++ b/tests/contrib/asyncio/fastapi_tests.py @@ -0,0 +1,97 @@ +# BSD 3-Clause License +# +# Copyright (c) 2019, Elasticsearch BV +# All rights reserved. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are met: +# +# * Redistributions of source code must retain the above copyright notice, this +# list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright notice, +# this list of conditions and the following disclaimer in the documentation +# and/or other materials provided with the distribution. +# +# * Neither the name of the copyright holder nor the names of its +# contributors may be used to endorse or promote products derived from +# this software without specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" +# AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE +# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE +# DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE +# FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL +# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR +# SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER +# CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +from tests.fixtures import TempStoreClient + +import pytest # isort:skip + +fastapi = pytest.importorskip("fastapi") # isort:skip + +from fastapi import APIRouter, FastAPI # noqa: E402 +from starlette.testclient import TestClient # noqa: E402 + +import elasticapm # noqa: E402 +from elasticapm.conf import constants # noqa: E402 +from elasticapm.contrib.starlette import ElasticAPM, make_apm_client # noqa: E402 + +pytestmark = [pytest.mark.starlette] + + +@pytest.fixture +def fastapi_app(elasticapm_client): + router = APIRouter(prefix="/api") + + @router.get("/items/{item_id}") + async def get_item(item_id: int): + return {"item_id": item_id} + + app = FastAPI() + app.include_router(router) + app.add_middleware(ElasticAPM, client=elasticapm_client) + + yield app + + elasticapm.uninstrument() + + +def test_included_router_request_succeeds(fastapi_app, elasticapm_client): + client = TestClient(fastapi_app) + + response = client.get("/api/items/42") + + assert response.status_code == 200 + assert response.json() == {"item_id": 42} + assert len(elasticapm_client.events[constants.TRANSACTION]) == 1 + + +def test_included_router_transaction_name(fastapi_app, elasticapm_client): + client = TestClient(fastapi_app) + + response = client.get("/api/items/42") + + assert response.status_code == 200 + + transaction = elasticapm_client.events[constants.TRANSACTION][0] + assert transaction["name"] == "GET /api/items/{item_id}" + + +def test_included_router_options_partial_match(fastapi_app, elasticapm_client): + client = TestClient(fastapi_app) + + response = client.options("/api/items/42") + + assert response.status_code == 405 + assert len(elasticapm_client.events[constants.TRANSACTION]) == 1 + + +def test_make_client_with_fastapi_framework(): + c = make_apm_client(config={"SERVICE_NAME": "foo"}, client_cls=TempStoreClient, framework_name="fastapi") + c.close() + assert c.config.service_name == "foo" diff --git a/tests/requirements/reqs-starlette-newest.txt b/tests/requirements/reqs-starlette-newest.txt index b81367f0a..8217c6b69 100644 --- a/tests/requirements/reqs-starlette-newest.txt +++ b/tests/requirements/reqs-starlette-newest.txt @@ -1,4 +1,5 @@ starlette>0.15,<1 +fastapi>=0.137.2 aiofiles httpx flask