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

fix: Fix validation for web frameworks #148

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions appmap/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@
# not using flask
pass

try:
from . import django
except ImportError:
# not using django
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems ok to me, python isn't as fragile re import order as ruby can be.


def enabled():
return Env.current.enabled
30 changes: 18 additions & 12 deletions appmap/command/appmap_agent_validate.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,31 +23,37 @@ def check_python_version():
except AppMapPyVerException as e:
raise ValidationFailure(str(e))

def _check_version(dist, v):
def _check_version(dist, versions):
dist_version = None
try:
dist_version = version(dist)
for v in versions:
try:
dist_version = version(dist)

required = parse(v)
actual = parse(dist_version)
if required.major != actual.major:
dist_version = None
continue

required = parse(v)
actual = parse(dist_version)
if actual < required:
raise ValidationFailure(f'{dist} must have version >= {required}, found {actual}')

if actual < required:
raise ValidationFailure(f'{dist} must have version >= {required}, found {actual}')
except PackageNotFoundError:
pass
return dist_version
except PackageNotFoundError:
dist_version = None
Comment on lines 27 to +43
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: this changed logic is a bit convoluted and difficult to follow. How about something like:

try:
  dist_version = version(dist)
  actual = parse(dist_version)
  required = next(v for v in map(parse, versions) if v.major == actual.major)
  if actual < required:
    raise ValidationFailure
except PackageNotFoundError, StopIteration:
  return None
return dist_version


return dist_version

# Note that, per https://www.python.org/dev/peps/pep-0426/#name,
# comparison of distribution names are case-insensitive.
def check_django_version():
return _check_version('django', '3.2')
return _check_version('django', ['2.2', '3.2'])

def check_flask_version():
return _check_version('flask', '1.1')
return _check_version('flask', ['1.1', '2.0'])

def check_pytest_version():
return _check_version('pytest', '6.2')
return _check_version('pytest', ['6.2'])

def _run():
errors = [ValidationFailure('internal error')] # shouldn't ever see this
Expand Down
59 changes: 52 additions & 7 deletions appmap/test/test_command.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import re


from importlib_metadata import version
from importlib_metadata import PackageNotFoundError, version
import pytest

import appmap._implementation
Expand Down Expand Up @@ -115,15 +115,60 @@ def test_python_version(self, capsys, mocker):

self.check_errors(capsys, 1, 1, r'Minimum Python version supported is \d\.\d, found')

def test_django_version(self, capsys, mocker):
@pytest.mark.parametrize('django_version', ['3.2.1', '2.2.1'])
def test_django_version_current(self, capsys, mocker, django_version):
def side_effect(dist):
if dist == 'django':
return django_version
elif dist == 'flask':
raise PackageNotFoundError()
return version(dist)

m = mocker.patch('appmap.command.appmap_agent_validate.version',
side_effect=side_effect)

self.check_errors(capsys, 0, 0, None)


@pytest.mark.parametrize('django_version', ['3.1.0', '2.1.0'])
def test_django_version_old(self, capsys, mocker, django_version):
def side_effect(dist):
if dist == 'django':
return django_version
elif dist == 'flask':
raise PackageNotFoundError()
return version(dist)

m = mocker.patch('appmap.command.appmap_agent_validate.version',
side_effect=lambda d: '3.1' if d == 'django' else version(d))
side_effect=side_effect)

self.check_errors(capsys, 1, 1, 'django must have version >= 3.2, found 3.1')
self.check_errors(capsys, 1, 1, 'django must have version >=')


@pytest.mark.parametrize('flask_version', ['1.1.4', '2.0.1'])
def test_flask_version_current(self, capsys, mocker, flask_version):
def side_effect(dist):
if dist == 'django':
raise PackageNotFoundError()
elif dist == 'flask':
return flask_version
return version(dist)

m = mocker.patch('appmap.command.appmap_agent_validate.version',
side_effect=side_effect)

self.check_errors(capsys, 0, 0, None)

@pytest.mark.parametrize('flask_version', ['1.0', '2.0.0.alpha1'])
def test_flask_version_old(self, capsys, mocker, flask_version):
def side_effect(dist):
if dist == 'django':
raise PackageNotFoundError()
elif dist == 'flask':
return flask_version
return version(dist)

def test_flask_version(self, capsys, mocker):
m = mocker.patch('appmap.command.appmap_agent_validate.version',
side_effect=lambda d: '1.0' if d == 'flask' else version(d))
side_effect=side_effect)

self.check_errors(capsys, 1, 1, 'flask must have version >= 1.1, found 1.0')
self.check_errors(capsys, 1, 1, 'flask must have version >=')
4 changes: 2 additions & 2 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ PyYAML = "^5.3.0"
inflection = "^0.3.0"
importlib-metadata = ">=0.8"
Django = { version = "^3.1.6", python = "^3.6", optional = true }
flask = { version = "^1.1.2", python = "^3.6", optional = true }

[tool.poetry.dev-dependencies]
httpretty = "^1.0.5"
Expand All @@ -48,7 +49,6 @@ pyfakefs = "^4.3.2"
pprintpp = "^0.4.0"
coverage = "^5.3"
pytest-mock = "^3.5.1"
flask = "^1.1.2"
SQLAlchemy = { version = "^1.4.11", python = "^3.6"}
tox = "^3.22.0"
tox-pyenv = "^1.1.0"
Expand All @@ -58,7 +58,7 @@ requests = "^2.25.1"
pytest-django = "^4.4.0"

[tool.poetry.extras]
test = ["Django"]
test = ["Django", "flask"]

[build-system]
requires = ["poetry-core>=1.0.0"]
Expand Down
9 changes: 7 additions & 2 deletions tox.ini
Original file line number Diff line number Diff line change
@@ -1,13 +1,18 @@
[tox]
skipsdist = true
envlist = py3{6,7,8,9}-django{32,22}
envlist = py3{6,7,8,9}-django{32,22},py3{6,7,8,9}-flask{10,20}

[testenv]
deps=
poetry
django32: Django>=3.2,<4.0
django32: flask>=2.0
django22: Django>=2.2,<3.0
flask20: flask>=2.0
flask10: flask>=1.1,<2.0
commands =
poetry install -v
django32: poetry run {posargs:pytest -v}
django22: poetry run pytest appmap/test/test_django.py
django22: poetry run pytest appmap/test/test_django.py appmap/test/test_command.py
flask20: poetry run pytest appmap/test/test_flask.py appmap/test/test_command.py
flask10: poetry run pytest appmap/test/test_flask.py appmap/test/test_command.py