From 818737a7c6acecef0d879d898327221bee2bc2f2 Mon Sep 17 00:00:00 2001 From: Benjamin Dauvergne Date: Thu, 17 May 2018 15:22:27 +0200 Subject: [PATCH] natural_key: fix get_by_natural_key_json for objects with partial unique indexes (fixes #23857) Unicity on NULL column must be explicitely stated in the list of natural keys. --- src/authentic2/a2_rbac/models.py | 13 ++++++-- src/authentic2/natural_key.py | 57 ++++++++++++++++++++------------ tests/test_natural_key.py | 18 +++++++--- 3 files changed, 61 insertions(+), 27 deletions(-) diff --git a/src/authentic2/a2_rbac/models.py b/src/authentic2/a2_rbac/models.py index 4f8a5aaa..dad83d1f 100644 --- a/src/authentic2/a2_rbac/models.py +++ b/src/authentic2/a2_rbac/models.py @@ -115,7 +115,10 @@ class Permission(PermissionAbstractBase): object_id_field='admin_scope_id') -Permission._meta.natural_key = ['operation', 'ou', 'target'] +Permission._meta.natural_key = [ + ['operation', 'ou', 'target'], + ['operation', 'ou__isnull', 'target'], +] class Role(RoleAbstractBase): @@ -247,7 +250,13 @@ class Role(RoleAbstractBase): Role._meta.natural_key = [ - ['uuid'], ['slug', 'ou'], ['name', 'ou'], ['slug', 'service'], ['name', 'service'] + ['uuid'], + ['slug', 'ou__isnull', 'service__isnull'], + ['name', 'ou__isnull', 'service__isnull'], + ['slug', 'ou', 'service'], + ['name', 'ou', 'service'], + ['slug', 'ou', 'service__isnull'], + ['name', 'ou', 'service__isnull'], ] diff --git a/src/authentic2/natural_key.py b/src/authentic2/natural_key.py index ad0bf95c..04c556e8 100644 --- a/src/authentic2/natural_key.py +++ b/src/authentic2/natural_key.py @@ -24,6 +24,8 @@ def natural_key_json(self): names.add(key) for name in names: + if name.endswith('__isnull'): + name = name.split('__isnull')[0] field = self._meta.get_field(name) if not (field.concrete or isinstance(field, GenericForeignKey)): raise ValueError('field %s is not concrete' % name) @@ -49,6 +51,9 @@ def get_by_natural_key_json(self, d): for natural_key in natural_keys: get_kwargs = {} for name in natural_key: + isnull = name.endswith('__isnull') + if isnull: + name = name.split('__isnull')[0] field = model._meta.get_field(name) if not (field.concrete or isinstance(field, GenericForeignKey)): raise ValueError('field %s is not concrete' % name) @@ -57,34 +62,44 @@ def get_by_natural_key_json(self, d): try: value = d[name] except KeyError: - break - if isinstance(field, GenericForeignKey): - try: - ct_nk = d[field.ct_field] - except KeyError: - break - try: - ct = ContentType.objects.get_by_natural_key_json(ct_nk) - except ContentType.DoesNotExist: + if not isnull: break - related_model = ct.model_class() - try: - value = related_model._default_manager.get_by_natural_key_json(value) - except related_model.DoesNotExist: + value = None + if isinstance(field, GenericForeignKey): + if isnull and value: break - get_kwargs[field.ct_field] = ct - name = field.fk_field - value = value.pk + elif not isnull: + if not value: + break + try: + ct_nk = d[field.ct_field] + except KeyError: + break + try: + ct = ContentType.objects.get_by_natural_key_json(ct_nk) + except ContentType.DoesNotExist: + break + related_model = ct.model_class() + try: + value = related_model._default_manager.get_by_natural_key_json(value) + except related_model.DoesNotExist: + break + name = field.fk_field + value = value.pk elif field.is_relation: - if value is None: - name = '%s__isnull' % name - value = True - else: + if isnull and value: + break + elif not isnull: + if not value: + break try: value = field.related_model._default_manager.get_by_natural_key_json(value) except field.related_model.DoesNotExist: break - get_kwargs[name] = value + if isnull: + get_kwargs[name + '__isnull'] = True + else: + get_kwargs[name] = value else: try: return self.get(**get_kwargs) diff --git a/tests/test_natural_key.py b/tests/test_natural_key.py index a4930c65..dda37e11 100644 --- a/tests/test_natural_key.py +++ b/tests/test_natural_key.py @@ -1,3 +1,5 @@ +import pytest + from django.contrib.contenttypes.models import ContentType from authentic2.a2_rbac.models import Role, OrganizationalUnit as OU, Permission @@ -29,12 +31,20 @@ def test_natural_key_json(db, ou1): } assert role == Role.objects.get_by_natural_key_json(nk) assert role == Role.objects.get_by_natural_key_json({'uuid': role.uuid}) - assert role == Role.objects.get_by_natural_key_json({'slug': role.slug, 'ou': ou_nk}) - assert role == Role.objects.get_by_natural_key_json({'name': role.name, 'ou': ou_nk}) + if service_nk: + with pytest.raises(Role.DoesNotExist): + Role.objects.get_by_natural_key_json({'slug': role.slug, 'ou': ou_nk}) + else: + assert Role.objects.get_by_natural_key_json({'slug': role.slug, 'ou': ou_nk}) == role + if service_nk: + with pytest.raises(Role.DoesNotExist): + assert Role.objects.get_by_natural_key_json({'name': role.name, 'ou': ou_nk}) + else: + assert Role.objects.get_by_natural_key_json({'name': role.name, 'ou': ou_nk}) == role assert role == Role.objects.get_by_natural_key_json( - {'slug': role.slug, 'service': service_nk}) + {'slug': role.slug, 'ou': ou_nk, 'service': service_nk}) assert role == Role.objects.get_by_natural_key_json( - {'name': role.name, 'service': service_nk}) + {'name': role.name, 'ou': ou_nk, 'service': service_nk}) for permission in Permission.objects.all(): ou_nk = permission.ou and permission.ou.natural_key_json() -- 2.17.0