From 712dbd55dd7d00486ef6e7db7f74ce2e2aee73a8 Mon Sep 17 00:00:00 2001 From: Alan Potter Date: Thu, 13 Oct 2022 14:09:51 -0400 Subject: [PATCH 1/4] fix: improve Flask normalized-path parsing Instead of relying on an internal Flask API to parse a Rule, parse the result of calling repr on it. The format of the string returned by repr is the same for Flask 1 and Flask 2, and has all the information we need. --- appmap/flask.py | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/appmap/flask.py b/appmap/flask.py index 5be7c13d..0edee753 100644 --- a/appmap/flask.py +++ b/appmap/flask.py @@ -1,6 +1,7 @@ import datetime import json import os.path +import re import time from functools import wraps @@ -9,7 +10,6 @@ import jinja2 from flask import _app_ctx_stack, request from werkzeug.exceptions import BadRequest -from werkzeug.routing import parse_rule from appmap._implementation import generation, web_framework from appmap._implementation.detect_enabled import DetectEnabled @@ -45,6 +45,10 @@ def request_params(req): return values_dict(params.lists()) +NP_REGEXP = re.compile(r"<)|(?P>)") + + class AppmapFlask(AppmapMiddleware): def __init__(self, app=None): super().__init__() @@ -112,16 +116,20 @@ def before_request(self): def before_request_main(self, rec, request): Metadata.add_framework("flask", flask.__version__) np = None - # See - # https://github.com/pallets/werkzeug/blob/2.0.0/src/werkzeug/routing.py#L213 - # for a description of parse_rule. if request.url_rule: - np = "".join( - [ - f"{{{p}}}" if c else p - for c, _, p in parse_rule(request.url_rule.rule) - ] + # Transform request.url to the expected normalized-path form. For example, + # "/post///summary" becomes "/post/{username}/{post_id}/summary". + # Notes: + # * the value of `repr` of this rule begins with "//summary'" + # * the variable names in a rule can only contain alphanumerics: + # * flask 1: https://github.com/pallets/werkzeug/blob/1dde4b1790f9c46b7122bb8225e6b48a5b22a615/src/werkzeug/routing.py#L143 + # * flask 2: https://github.com/pallets/werkzeug/blob/99f328cf2721e913bd8a3128a9cdd95ca97c334c/src/werkzeug/routing/rules.py#L56 + r = repr(request.url_rule) + np = NP_XLATE.sub( + lambda m: "{" if m["l"] else "}" if m["r"] else "", + NP_REGEXP.findall(r)[0], ) + call_event = HttpServerRequestEvent( request_method=request.method, path_info=request.path, From 0b906a3a9a17efa9122349a836339879ba885795 Mon Sep 17 00:00:00 2001 From: Alan Potter Date: Thu, 13 Oct 2022 14:13:07 -0400 Subject: [PATCH 2/4] test: use tox to test Flask 1 Remove Flask 1 dependency from pyproject.toml. Instead, add a flask1 factor to tox.ini, and use it for testing Flask 1. --- pyproject.toml | 5 ----- requirements-flask1.txt | 5 +++++ tox.ini | 19 +++++++++++++------ 3 files changed, 18 insertions(+), 11 deletions(-) create mode 100644 requirements-flask1.txt diff --git a/pyproject.toml b/pyproject.toml index b44bf3cf..5d01cc5f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -58,11 +58,6 @@ pyfakefs = "^4.3.2" pprintpp = ">=0.4.0" coverage = "^5.3" pytest-mock = "^3.5.1" -flask = "^1.1.2" -MarkupSafe = "2.0.1" -jinja2 = ">=2.10.1,<3" -itsdangerous = ">=0.24,<2" -Werkzeug = ">=0.15,<2" SQLAlchemy = { version = "^1.4.11", python = "^3.7"} tox = "^3.22.0" tox-pyenv = "^1.1.0" diff --git a/requirements-flask1.txt b/requirements-flask1.txt new file mode 100644 index 00000000..11c5ae2b --- /dev/null +++ b/requirements-flask1.txt @@ -0,0 +1,5 @@ +flask == 1.1.2 +MarkupSafe == 2.0.1 +jinja2 >=2.10.1, <3 +itsdangerous >=0.24,<2 +Werkzeug >=0.15,<2 \ No newline at end of file diff --git a/tox.ini b/tox.ini index 6286d777..90b55434 100644 --- a/tox.ini +++ b/tox.ini @@ -1,20 +1,27 @@ [tox] isolated_build = true -envlist = py37-django{2,3}, py3{8,9,10}-django{2,3,4} +# The *-web environments test the latest versions of Django and Flask with the full test suite. For +# older version of the web frameworks, just run the tests that are specific to them. +envlist = py3{8,9,10}-web, py3{7,8,9,10}-flask1-django{2,3} [testenv] + deps= poetry>=1.2.0 pytest-django - django2: Django>=2.2,<3.0 - django3: Django>=3.2,<4.0 - django4: Django>=4.0,<5.0 + web: Django >=4.0, <5.0 + web: Flask >= 2 + flask1: -rrequirements-flask1.txt + django2: Django >=2.2, <3.0 + django3: Django >=3.2, <4.0 + allowlist_externals = env commands = # Turn off recording while installing. It's not necessary, and the warning messages that come # out of the agent confuse poetry. env APPMAP=false poetry install -v - django3: poetry run {posargs:pytest -v} + web: poetry run {posargs:pytest -vv} + flask1: poetry run pytest appmap/test/test_flask.py django2: poetry run pytest appmap/test/test_django.py - django4: poetry run pytest appmap/test/test_django.py + django3: poetry run pytest appmap/test/test_django.py From 26cde29b5bf857482c717cf8fd8b3da1740e3e74 Mon Sep 17 00:00:00 2001 From: Alan Potter Date: Thu, 13 Oct 2022 14:15:01 -0400 Subject: [PATCH 3/4] feat: support Flask 2 Fix up Flask support so it's compatible with v2. Also fixes initial configuration, so a Flask app will be recorded by default. --- appmap/_implementation/flask.py | 39 +++++++++ appmap/_implementation/web_framework.py | 3 +- appmap/flask.py | 91 +++++++------------- appmap/test/data/flask/app.py | 11 +-- appmap/test/data/flask/init/sitecustomize.py | 1 + appmap/test/test_flask.py | 13 ++- requirements-dev.txt | 7 ++ 7 files changed, 98 insertions(+), 67 deletions(-) create mode 100644 appmap/_implementation/flask.py create mode 100644 appmap/test/data/flask/init/sitecustomize.py create mode 100644 requirements-dev.txt diff --git a/appmap/_implementation/flask.py b/appmap/_implementation/flask.py new file mode 100644 index 00000000..66932d6b --- /dev/null +++ b/appmap/_implementation/flask.py @@ -0,0 +1,39 @@ +""" remote_recording is a Flask app that can be mounted to expose the remote-recording endpoint. """ +import json + +from flask import Flask + +from . import generation +from .recorder import Recorder +from .web_framework import AppmapMiddleware + +remote_recording = Flask(__name__) + + +@remote_recording.route("/record", methods=["GET"]) +def status(): + if not AppmapMiddleware.should_record(): + return "Appmap is disabled.", 404 + + return {"enabled": Recorder.get_current().get_enabled()} + + +@remote_recording.route("/record", methods=["POST"]) +def start(): + r = Recorder.get_current() + if r.get_enabled(): + return "Recording is already in progress", 409 + + r.start_recording() + return "", 200 + + +@remote_recording.route("/record", methods=["DELETE"]) +def stop(): + r = Recorder.get_current() + if not r.get_enabled(): + return "No recording is in progress", 404 + + r.stop_recording() + + return json.loads(generation.dump(r)) diff --git a/appmap/_implementation/web_framework.py b/appmap/_implementation/web_framework.py index 3cfe6a8c..f063e86e 100644 --- a/appmap/_implementation/web_framework.py +++ b/appmap/_implementation/web_framework.py @@ -134,7 +134,8 @@ class AppmapMiddleware(ABC): def __init__(self): self.record_url = "/_appmap/record" - def should_record(self): + @staticmethod + def should_record(): return DetectEnabled.should_enable("remote") or DetectEnabled.should_enable( "requests" ) diff --git a/appmap/flask.py b/appmap/flask.py index 0edee753..b8a72d51 100644 --- a/appmap/flask.py +++ b/appmap/flask.py @@ -1,20 +1,19 @@ -import datetime -import json -import os.path import re import time -from functools import wraps import flask import flask.cli import jinja2 from flask import _app_ctx_stack, request +from flask.cli import ScriptInfo from werkzeug.exceptions import BadRequest +from werkzeug.middleware.dispatcher import DispatcherMiddleware -from appmap._implementation import generation, web_framework +import appmap.wrapt as wrapt from appmap._implementation.detect_enabled import DetectEnabled from appmap._implementation.env import Env from appmap._implementation.event import HttpServerRequestEvent, HttpServerResponseEvent +from appmap._implementation.flask import remote_recording from appmap._implementation.recorder import Recorder from appmap._implementation.web_framework import AppmapMiddleware from appmap._implementation.web_framework import TemplateHandler as BaseTemplateHandler @@ -50,61 +49,35 @@ def request_params(req): class AppmapFlask(AppmapMiddleware): - def __init__(self, app=None): + """ + A Flask extension to add remote recording to an application. + Should be loaded by default, but can also be added manually. + + For example: + + ``` + from appmap.flask import AppmapFlask + + app = new Flask(__Name__) + AppmapFlask().init_app(app) + ``` + """ + + def __init__(self): super().__init__() - self.app = app - if app is not None: - self.init_app(app) def init_app(self, app): if self.should_record(): - # it may record requests but not remote (APPMAP=false) self.recorder = Recorder.get_current() if DetectEnabled.should_enable("remote"): - app.add_url_rule( - self.record_url, - "appmap_record_get", - view_func=self.record_get, - methods=["GET"], - ) - app.add_url_rule( - self.record_url, - "appmap_record_post", - view_func=self.record_post, - methods=["POST"], - ) - app.add_url_rule( - self.record_url, - "appmap_record_delete", - view_func=self.record_delete, - methods=["DELETE"], + app.wsgi_app = DispatcherMiddleware( + app.wsgi_app, {"/_appmap": remote_recording} ) app.before_request(self.before_request) app.after_request(self.after_request) - def record_get(self): - if not self.should_record(): - return "Appmap is disabled.", 404 - - return {"enabled": self.recorder.get_enabled()} - - def record_post(self): - if self.recorder.get_enabled(): - return "Recording is already in progress", 409 - - self.recorder.start_recording() - return "", 200 - - def record_delete(self): - if not self.recorder.get_enabled(): - return "No recording is in progress", 404 - - self.recorder.stop_recording() - - return json.loads(generation.dump(self.recorder)) - def before_request(self): if not self.should_record(): return @@ -182,18 +155,18 @@ class TemplateHandler(BaseTemplateHandler): pass -def wrap_cli_fn(fn): - @wraps(fn) - def install_middleware(*args, **kwargs): - app = fn(*args, **kwargs) - if app: - appmap_flask = AppmapFlask() - appmap_flask.init_app(app) - return app +def install_extension(wrapped, _, args, kwargs): + app = wrapped(*args, **kwargs) + if app: + AppmapFlask().init_app(app) - return install_middleware + return app if Env.current.enabled: - flask.cli.call_factory = wrap_cli_fn(flask.cli.call_factory) - flask.cli.locate_app = wrap_cli_fn(flask.cli.locate_app) + # ScriptInfo.load_app is the function that's used by the Flask cli to load an app, no matter how + # the app's module is specified (e.g. with the FLASK_APP env var, the `--app` flag, etc). Hook + # it so it installs our extension on the app. + ScriptInfo.load_app = wrapt.wrap_function_wrapper( + "flask.cli", "ScriptInfo.load_app", install_extension + ) diff --git a/appmap/test/data/flask/app.py b/appmap/test/data/flask/app.py index 93b75aa4..7d326f8f 100644 --- a/appmap/test/data/flask/app.py +++ b/appmap/test/data/flask/app.py @@ -1,15 +1,16 @@ -"""Rudimentary Flask application for testing.""" +""" +Rudimentary Flask application for testing. + +NB: This should not explicitly reference the `appmap` module in any way. Doing so invalidates +testing of record-by-default. +""" # pylint: disable=missing-function-docstring from flask import Flask, make_response from markupsafe import escape -from appmap.flask import AppmapFlask - app = Flask(__name__) -appmap_flask = AppmapFlask(app) - @app.route("/") def hello_world(): diff --git a/appmap/test/data/flask/init/sitecustomize.py b/appmap/test/data/flask/init/sitecustomize.py new file mode 100644 index 00000000..d1fe4fec --- /dev/null +++ b/appmap/test/data/flask/init/sitecustomize.py @@ -0,0 +1 @@ +import appmap diff --git a/appmap/test/test_flask.py b/appmap/test/test_flask.py index 357e7e26..bd8fe3d4 100644 --- a/appmap/test/test_flask.py +++ b/appmap/test/test_flask.py @@ -8,6 +8,7 @@ import pytest from appmap._implementation.env import Env +from appmap.flask import AppmapFlask from appmap.test.helpers import DictIncluding from .._implementation.metadata import Metadata @@ -40,6 +41,11 @@ def flask_app(data_dir, monkeypatch): import app # pylint: disable=import-error importlib.reload(app) + + # Add the AppmapFlask extension to the app. This now happens automatically when a Flask app is + # started from the command line, but must be done manually otherwise. + AppmapFlask().init_app(app.app) + return app.app @@ -70,12 +76,15 @@ def test_template(app, events): class TestRecordRequestsFlask(TestRecordRequests): @staticmethod def server_start_thread(env_vars_str): + # Use appmap from our working copy, not the module installed by virtualenv. Add the init + # directory so the sitecustomize.py file it contains will be loaded on startup. This + # simulates a real installation. exec_cmd( """ -# use appmap from our working copy, not the module installed by virtualenv -export PYTHONPATH=`pwd` +export PYTHONPATH="$PWD" cd appmap/test/data/flask/ +PYTHONPATH="$PYTHONPATH:$PWD/init" """ + env_vars_str + """ APPMAP_OUTPUT_DIR=/tmp FLASK_DEBUG=1 FLASK_APP=app.py flask run -p """ diff --git a/requirements-dev.txt b/requirements-dev.txt new file mode 100644 index 00000000..a4c6a1f2 --- /dev/null +++ b/requirements-dev.txt @@ -0,0 +1,7 @@ +#requirements-dev.txt +pip +poetry +tox +django +flask +pytest-django \ No newline at end of file From f15e5917cbed5e10fb973edff67e5e731edc0de4 Mon Sep 17 00:00:00 2001 From: Alan Potter Date: Tue, 25 Oct 2022 07:15:16 -0400 Subject: [PATCH 4/4] fix: inject Django middleware automatically Import appmap.django at startup, to make sure our middleware gets injected. --- appmap/__init__.py | 6 ++++++ appmap/flask.py | 11 ++++------- appmap/test/data/django/app/settings.py | 2 -- appmap/test/data/django/init/sitecustomize.py | 1 + appmap/test/test_django.py | 7 +++++-- 5 files changed, 16 insertions(+), 11 deletions(-) create mode 100644 appmap/test/data/django/init/sitecustomize.py diff --git a/appmap/__init__.py b/appmap/__init__.py index 5f4823fd..fcc294d9 100644 --- a/appmap/__init__.py +++ b/appmap/__init__.py @@ -5,6 +5,12 @@ from ._implementation.labels import labels # noqa: F401 from ._implementation.recording import Recording # noqa: F401 +try: + from . import django # noqa: F401 +except ImportError: + # not using django + pass + try: from . import flask # noqa: F401 except ImportError: diff --git a/appmap/flask.py b/appmap/flask.py index b8a72d51..7a141ecc 100644 --- a/appmap/flask.py +++ b/appmap/flask.py @@ -44,8 +44,8 @@ def request_params(req): return values_dict(params.lists()) -NP_REGEXP = re.compile(r"<)|(?P>)") +NP_PARAMS = re.compile(r"", "{}") class AppmapFlask(AppmapMiddleware): @@ -93,15 +93,12 @@ def before_request_main(self, rec, request): # Transform request.url to the expected normalized-path form. For example, # "/post///summary" becomes "/post/{username}/{post_id}/summary". # Notes: - # * the value of `repr` of this rule begins with "//summary'" + # * the value of `repr` of this rule begins with "//summary'" # * the variable names in a rule can only contain alphanumerics: # * flask 1: https://github.com/pallets/werkzeug/blob/1dde4b1790f9c46b7122bb8225e6b48a5b22a615/src/werkzeug/routing.py#L143 # * flask 2: https://github.com/pallets/werkzeug/blob/99f328cf2721e913bd8a3128a9cdd95ca97c334c/src/werkzeug/routing/rules.py#L56 r = repr(request.url_rule) - np = NP_XLATE.sub( - lambda m: "{" if m["l"] else "}" if m["r"] else "", - NP_REGEXP.findall(r)[0], - ) + np = NP_PARAMS.findall(r)[0].translate(NP_PARAM_DELIMS) call_event = HttpServerRequestEvent( request_method=request.method, diff --git a/appmap/test/data/django/app/settings.py b/appmap/test/data/django/app/settings.py index 047ec3f0..90418e3e 100644 --- a/appmap/test/data/django/app/settings.py +++ b/appmap/test/data/django/app/settings.py @@ -10,5 +10,3 @@ # Must set ROOT_URLCONF else we get # AttributeError: 'Settings' object has no attribute 'ROOT_URLCONF' ROOT_URLCONF = "app.urls" - -MIDDLEWARE = ["appmap.django.Middleware"] diff --git a/appmap/test/data/django/init/sitecustomize.py b/appmap/test/data/django/init/sitecustomize.py new file mode 100644 index 00000000..d1fe4fec --- /dev/null +++ b/appmap/test/data/django/init/sitecustomize.py @@ -0,0 +1 @@ +import appmap diff --git a/appmap/test/test_django.py b/appmap/test/test_django.py index 537a9a55..b4c65c3c 100644 --- a/appmap/test/test_django.py +++ b/appmap/test/test_django.py @@ -208,12 +208,15 @@ def test_middleware_reset(pytester, monkeypatch): class TestRecordRequestsDjango(TestRecordRequests): @staticmethod def server_start_thread(env_vars_str): + # Use appmap from our working copy, not the module installed by virtualenv. Add the init + # directory so the sitecustomize.py file it contains will be loaded on startup. This + # simulates a real installation. exec_cmd( """ -# use appmap from our working copy, not the module installed by virtualenv -export PYTHONPATH=`pwd` +export PYTHONPATH="$PWD" cd appmap/test/data/django/ +PYTHONPATH="$PYTHONPATH:$PWD/init" """ + env_vars_str + """ APPMAP_OUTPUT_DIR=/tmp python manage.py runserver 127.0.0.1:"""