From 3d16700a031b468a403561d1bc29df5121be2093 Mon Sep 17 00:00:00 2001 From: Charlie Denton Date: Thu, 18 Jan 2018 23:17:33 +0000 Subject: [PATCH 1/3] Add shortcut to annotate and filter Routes by level --- CHANGELOG.md | 1 + conman/routes/expressions.py | 24 +++++++++++++++++ conman/routes/managers.py | 14 ++++++++++ tests/routes/test_expressions.py | 45 ++++++++++++++++++++++++++++++++ tests/routes/test_managers.py | 24 +++++++++++++++++ 5 files changed, 108 insertions(+) create mode 100644 conman/routes/expressions.py create mode 100644 tests/routes/test_expressions.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 87e64fe..b10f7fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Added +- Added `Route.objects.with_level()` to allow access to `level` in querysets. - Added `Route.get_subclasses()`. - Added `TemplateHandler`. A simpler handler that requires only a template. This is the new default for `Route.handler_class`. diff --git a/conman/routes/expressions.py b/conman/routes/expressions.py new file mode 100644 index 0000000..56aeb4d --- /dev/null +++ b/conman/routes/expressions.py @@ -0,0 +1,24 @@ +from django.db.models.expressions import Func + + +class CharCount(Func): + """ + Count the occurrences of a char within a field. + + Works by finding the difference in length between the whole string, and the + string with the char removed. + """ + template = "CHAR_LENGTH(%(field)s) - CHAR_LENGTH(REPLACE(%(field)s, '%(char)s', ''))" + + def __init__(self, field, *, char, **extra): + """ + Add some validation to the invocation. + + "Char" must always: + + - be passed as a keyword argument + - be exactly one character. + """ + if len(char) != 1: + raise ValueError('CharCount must count exactly one char.') + super().__init__(field, char=char, **extra) diff --git a/conman/routes/managers.py b/conman/routes/managers.py index 6fb5cd4..de3c6e7 100644 --- a/conman/routes/managers.py +++ b/conman/routes/managers.py @@ -4,6 +4,7 @@ from polymorphic.managers import PolymorphicManager from .exceptions import InvalidURL +from .expressions import CharCount from .utils import split_path @@ -72,3 +73,16 @@ def move_branch(self, old_url, new_url): Value(new_url), Substr('url', len(old_url) + 1), # 1 indexed )) + + def with_level(self, level=None): + """ + Annotate the queryset with the (1-indexed) level of each item. + + The level reflects the number of forward slashes in the path. + + If "level" is passed in, the queryset will be filtered by the level. + """ + qs = self.annotate(level=CharCount('url', char='/')) + if level is None: + return qs + return qs.filter(level=level) diff --git a/tests/routes/test_expressions.py b/tests/routes/test_expressions.py new file mode 100644 index 0000000..7c4af00 --- /dev/null +++ b/tests/routes/test_expressions.py @@ -0,0 +1,45 @@ +from django.db import connection +from django.test import TestCase +from django.test.utils import CaptureQueriesContext + +from conman.routes.expressions import CharCount +from conman.routes.models import Route + +from .factories import RouteFactory + + +class TestCharCount(TestCase): + """Tests for CharCount.""" + def test_query(self): + """Match the exact value of the generated query.""" + with CaptureQueriesContext(connection): + # The "only" here is handy to keep the query as short as possible. + list(Route.objects.only('id').annotate(level=CharCount('url', char='/'))) + # Excuse the line wrapping here -- I wasn't sure of a nice way to do it. + # I decided it was better to just keep it simple. + expected = ( + 'SELECT "routes_route"."id", ' + + 'CHAR_LENGTH("routes_route"."url") - ' + + '''CHAR_LENGTH(REPLACE("routes_route"."url", '/', '')) AS "level" ''' + + 'FROM "routes_route"' + ) + self.assertEqual(connection.queries[0]['sql'], expected) + + def test_annotation(self): + """Test the expression can be used for annotation.""" + RouteFactory.create(url='/sixth/level/path/including/root/') + route = Route.objects.annotate(level=CharCount('url', char='/')).get() + self.assertEqual(route.level, 6) # The number of "/" in the path. + + def test_calling_format(self): + """Ensure the 'char' argument is always a keyword-arg.""" + with self.assertRaises(TypeError): + CharCount('url', 'a') + + def test_char_length(self): + """Ensure 'char' length is always 1.""" + msg = 'CharCount must count exactly one char.' + for not_a_char in ['', 'no']: + with self.subTest(char=not_a_char): + with self.assertRaisesMessage(ValueError, msg): + CharCount('url', char=not_a_char) diff --git a/tests/routes/test_managers.py b/tests/routes/test_managers.py index 7dd1d42..6fd772a 100644 --- a/tests/routes/test_managers.py +++ b/tests/routes/test_managers.py @@ -198,3 +198,27 @@ def test_descendant_destination_occupied(self): self.assertEqual(route.url, original_url) child.refresh_from_db() self.assertEqual(child.url, original_url + 'child/') + + +class RouteManagerWithPathTest(TestCase): + """Test Route.objects.with_level.""" + def test_no_level_passed(self): + """No level passed, so items are annotated, but no filter is applied.""" + RouteFactory.create(url='/') + RouteFactory.create(url='/branch/') + RouteFactory.create(url='/branch/leaf/') + result = Route.objects.with_level().values_list('level', 'url') + expected = ( + (1, '/'), + (2, '/branch/'), + (3, '/branch/leaf/'), + ) + self.assertCountEqual(result, expected) + + def test_level_passed(self): + """When passing a level, the filter is automatically applied.""" + RouteFactory.create(url='/') # Not in QS. + branch = RouteFactory.create(url='/branch/') + RouteFactory.create(url='/branch/leaf/') # Not in QS. + result = Route.objects.with_level(2) + self.assertCountEqual(result, [branch]) From 9a9c0da5a4c9a04a3b4bbf04d7a11b1e5c783141 Mon Sep 17 00:00:00 2001 From: Charlie Denton Date: Sun, 21 Jan 2018 18:18:05 +0000 Subject: [PATCH 2/3] Add Route.level property --- CHANGELOG.md | 1 + conman/routes/models.py | 10 ++++++++++ tests/routes/test_models.py | 12 ++++++++++++ 3 files changed, 23 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b10f7fd..d27533a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Added +- Added `Route.level` property. - Added `Route.objects.with_level()` to allow access to `level` in querysets. - Added `Route.get_subclasses()`. - Added `TemplateHandler`. A simpler handler that requires only a template. diff --git a/conman/routes/models.py b/conman/routes/models.py index 3d618e0..f62649e 100644 --- a/conman/routes/models.py +++ b/conman/routes/models.py @@ -136,6 +136,16 @@ def handle(self, request, path): # Deal with the request return handler.handle(request, path) + @property + def level(self): + """Fetch the one-indexed 'level' of this item in the URL tree.""" + return self.url.count('/') + + @level.setter + def level(self, new_value): + """Silently fails to allow queryset annotation to work.""" + pass + def move_to(self, new_url, *, move_children): """ Move this Route to a new url. diff --git a/tests/routes/test_models.py b/tests/routes/test_models.py index 10e109f..03915e2 100644 --- a/tests/routes/test_models.py +++ b/tests/routes/test_models.py @@ -47,6 +47,18 @@ def test_url_form_widget(self): widget = Route._meta.get_field('url').formfield().widget self.assertIsInstance(widget, forms.TextInput) + def test_level(self): + """Route.level is based on the url field.""" + urls = ( + (1, '/'), + (2, '/branch/'), + (3, '/branch/leaf/'), + ) + for level, url in urls: + with self.subTest(url=url): + route = RouteFactory.build(url=url) + self.assertEqual(route.level, level) + class RouteUniquenessTest(TestCase): """Check uniqueness conditions on Route are enforced in the DB.""" From d3963e4bcf038a3f6480dd5d3ceaba50afeaf50c Mon Sep 17 00:00:00 2001 From: Charlie Denton Date: Sun, 21 Jan 2018 21:34:09 +0000 Subject: [PATCH 3/3] Change Route.level to be zero-indexed --- conman/routes/managers.py | 4 ++-- conman/routes/models.py | 4 ++-- tests/routes/test_expressions.py | 5 +++-- tests/routes/test_managers.py | 8 ++++---- tests/routes/test_models.py | 8 ++++---- 5 files changed, 15 insertions(+), 14 deletions(-) diff --git a/conman/routes/managers.py b/conman/routes/managers.py index de3c6e7..9f51027 100644 --- a/conman/routes/managers.py +++ b/conman/routes/managers.py @@ -76,13 +76,13 @@ def move_branch(self, old_url, new_url): def with_level(self, level=None): """ - Annotate the queryset with the (1-indexed) level of each item. + Annotate the queryset with the (0-indexed) level of each item. The level reflects the number of forward slashes in the path. If "level" is passed in, the queryset will be filtered by the level. """ - qs = self.annotate(level=CharCount('url', char='/')) + qs = self.annotate(level=CharCount('url', char='/') - 1) if level is None: return qs return qs.filter(level=level) diff --git a/conman/routes/models.py b/conman/routes/models.py index f62649e..dd7f0c4 100644 --- a/conman/routes/models.py +++ b/conman/routes/models.py @@ -138,8 +138,8 @@ def handle(self, request, path): @property def level(self): - """Fetch the one-indexed 'level' of this item in the URL tree.""" - return self.url.count('/') + """Fetch the 'level' of this item in the URL tree.""" + return self.url.count('/') - 1 # 0-indexed. @level.setter def level(self, new_value): diff --git a/tests/routes/test_expressions.py b/tests/routes/test_expressions.py index 7c4af00..78f63d0 100644 --- a/tests/routes/test_expressions.py +++ b/tests/routes/test_expressions.py @@ -27,9 +27,10 @@ def test_query(self): def test_annotation(self): """Test the expression can be used for annotation.""" - RouteFactory.create(url='/sixth/level/path/including/root/') + RouteFactory.create(url='/fifth/level/zero/indexed/path/') route = Route.objects.annotate(level=CharCount('url', char='/')).get() - self.assertEqual(route.level, 6) # The number of "/" in the path. + # The number of "/" in the path minus one for zero-indexing. + self.assertEqual(route.level, 5) def test_calling_format(self): """Ensure the 'char' argument is always a keyword-arg.""" diff --git a/tests/routes/test_managers.py b/tests/routes/test_managers.py index 6fd772a..535fc2d 100644 --- a/tests/routes/test_managers.py +++ b/tests/routes/test_managers.py @@ -209,9 +209,9 @@ def test_no_level_passed(self): RouteFactory.create(url='/branch/leaf/') result = Route.objects.with_level().values_list('level', 'url') expected = ( - (1, '/'), - (2, '/branch/'), - (3, '/branch/leaf/'), + (0, '/'), + (1, '/branch/'), + (2, '/branch/leaf/'), ) self.assertCountEqual(result, expected) @@ -220,5 +220,5 @@ def test_level_passed(self): RouteFactory.create(url='/') # Not in QS. branch = RouteFactory.create(url='/branch/') RouteFactory.create(url='/branch/leaf/') # Not in QS. - result = Route.objects.with_level(2) + result = Route.objects.with_level(1) self.assertCountEqual(result, [branch]) diff --git a/tests/routes/test_models.py b/tests/routes/test_models.py index 03915e2..051374a 100644 --- a/tests/routes/test_models.py +++ b/tests/routes/test_models.py @@ -48,11 +48,11 @@ def test_url_form_widget(self): self.assertIsInstance(widget, forms.TextInput) def test_level(self): - """Route.level is based on the url field.""" + """Route.level is based on the url field, and is zero-indexed.""" urls = ( - (1, '/'), - (2, '/branch/'), - (3, '/branch/leaf/'), + (0, '/'), + (1, '/branch/'), + (2, '/branch/leaf/'), ) for level, url in urls: with self.subTest(url=url):