Skip to content
This repository has been archived by the owner on Apr 16, 2023. It is now read-only.

Add easy way to annotate Route QS with "level" #131

Merged
merged 3 commits into from
Jan 21, 2018
Merged
Show file tree
Hide file tree
Changes from 2 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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

### 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.
This is the new default for `Route.handler_class`.
Expand Down
24 changes: 24 additions & 0 deletions conman/routes/expressions.py
Original file line number Diff line number Diff line change
@@ -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)
14 changes: 14 additions & 0 deletions conman/routes/managers.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from polymorphic.managers import PolymorphicManager

from .exceptions import InvalidURL
from .expressions import CharCount
from .utils import split_path


Expand Down Expand Up @@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see why you chose 1-indexed, but I'm not sure that's the best api. It seems more natural to me to care about how many parts the url has rather than the number of / separators.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, I wasn't sure about it myself. I went for this simply because the implementation was marginally easier -- you'll probably not be the last person to think it's strange -- I'll look into changing it.

Copy link
Owner Author

Choose a reason for hiding this comment

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


The level reflects the number of forward slashes in the path.
Copy link
Owner Author

Choose a reason for hiding this comment

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

The level is one-indexed; it reflects the number of forward slashes in the path.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we guarantee elsewhere that a url always starts and ends with a /?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, it's a strange necessity of conman -- it makes other similar operations easy.

Copy link
Owner Author

Choose a reason for hiding this comment

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

On the whole, it shouldn't cause too many issues, I hope.


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)
10 changes: 10 additions & 0 deletions conman/routes/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you sure the annotation is used instead of the property if it exists? Or does that not matter?

Copy link
Owner Author

Choose a reason for hiding this comment

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

It's not that the annotation is used, exactly, it's that when an annotated object is instantiated, the attribute is set. Without this, and with the @property, a "can't set attribute" error is raised.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see - the annotation is used for sql-level operations like filtering, but the descriptor whenever we're in python.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Exactly :)


def move_to(self, new_url, *, move_children):
"""
Move this Route to a new url.
Expand Down
45 changes: 45 additions & 0 deletions tests/routes/test_expressions.py
Original file line number Diff line number Diff line change
@@ -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)
24 changes: 24 additions & 0 deletions tests/routes/test_managers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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])
12 changes: 12 additions & 0 deletions tests/routes/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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."""
Expand Down