diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ba9c15eb..e184ca03 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -8,7 +8,7 @@ jobs: matrix: # Testing Python 3.7 (deb 10), Python 3.9 (deb 11), Python 3.11 (deb 12) python-version: ["3.9", "3.11"] - django-version: ["3.2.25", "4.2.17", "5.1.4"] + django-version: ["3.2.25", "4.2.17", "5.2.9"] database-engine: ["postgres", "mysql"] os: [ubuntu-latest] include: @@ -23,7 +23,7 @@ jobs: os: ubuntu-22.04 exclude: - python-version: 3.9 - django-version: 5.1.4 + django-version: 5.2.9 runs-on: ${{ matrix.os }} @@ -82,7 +82,7 @@ jobs: - name: Run tests run: | - .venv/bin/coverage run --include="binder/*" -m unittest discover -vt . -s tests + DJANGO_VERSION=${{ matrix.django-version }} .venv/bin/coverage run --include="binder/*" -m unittest discover -vt . -s tests env: BINDER_TEST_MYSQL: ${{ matrix.database-engine == 'mysql' && 1 || 0 }} CY_RUNNING_INSIDE_CI: 1 diff --git a/Dockerfile b/Dockerfile index c1247412..86f3dcdd 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,7 +1,8 @@ -FROM python:3.9 +FROM python:3.10 ENV PYTHONUNBUFFERED 1 RUN mkdir /binder WORKDIR /binder ADD setup.py . ADD README.md . +RUN pip install django==5.2.9 RUN pip install -e .[test] diff --git a/README.md b/README.md index bec26022..a3432c85 100644 --- a/README.md +++ b/README.md @@ -11,12 +11,24 @@ Code Yellow backend framework for SPA webapps with REST-like API. - Run with `./test` - Access the test database directly by with `docker compose run --rm db psql -h db -U postgres`. -- It may be possible to recreate the test database (for example when you added/changed models). One way of achieving this is to just remove all the docker images that were build `docker compose rm`. The database will be created during the setup in `tests/__init__.py`. The tests are set up in such a way that there is no need to keep migration files. The setup procedure in `tests/__init__.py` handles the preparation of the database by directly calling some build-in Django commands. To only run a selection of the tests, use the `-k` flag like `./test -k tests.test_some_specific_test`. +## Refreshing the test database +After changing models, you may need to forcibly 'refresh' the test database. Use: +- `docker compose stop binder db` +- `docker compose rm -f binder db` + +After running these commands, the next `./test` may or may not fail with: +``` +django.db.utils.OperationalError: connection to server at "db" (172.20.0.2), port 5432 failed: Connection refused + Is the server running on that host and accepting TCP/IP connections? +``` + +If it fails, just retry it. + ## MySQL support MySQL is supported, but only with the goal to replace it with diff --git a/binder/decorators.py b/binder/decorators.py index df9f3556..e49fd86b 100644 --- a/binder/decorators.py +++ b/binder/decorators.py @@ -16,7 +16,7 @@ def decorated(request, *args, **kwargs): time_start = time.time() logger.info('request dispatch; verb={}, user={}/{}, path={}'.format( request.method, - request.user.id, + request.user.pk, request.user, request.path, )) diff --git a/binder/history.py b/binder/history.py index f92f42ca..4d65ef4e 100644 --- a/binder/history.py +++ b/binder/history.py @@ -23,7 +23,7 @@ class Changeset(models.Model): def __str__(self): uuid = self.uuid[:8] if self.uuid else None username = self.user.username if self.user else None - return '{}/{} by {} on {}'.format(self.id, uuid, username, self.date.strftime('%Y%m%d-%H%M%S')) + return '{}/{} by {} on {}'.format(self.pk, uuid, username, self.date.strftime('%Y%m%d-%H%M%S')) class Meta: ordering = ['id'] @@ -34,13 +34,23 @@ class Change(models.Model): changeset = models.ForeignKey(Changeset, on_delete=models.CASCADE, db_index=True, related_name='changes') model = models.CharField(max_length=64, db_index=True) oid = models.IntegerField(db_index=True) + oid_string = models.TextField(db_index=True, blank=True) + """ + Unfortunately, the original `oid` field is an `IntegerField`, which means that models with a non-integer primary key are not supported. + The `oid` field could be modified, but that would be a potentially big and undesirable migration that is sneakily pushed to downstream projects. + + Instead, this field will simply be used as alternative, only for models with a non-integer primary key. + """ field = models.CharField(max_length=64, db_index=True) diff = models.BooleanField(default=False) before = models.TextField(blank=True, null=True) after = models.TextField(blank=True, null=True) + def choose_oid(self): + return self.oid if self.oid > 0 else self.oid_string + def __str__(self): - return '{}: {}({}).{} {} -> {}'.format(self.id, self.model, self.oid, self.field, self.before[:20], self.after[:20]) + return '{}: {}({}).{} {} -> {}'.format(self.pk, self.model, self.oid, self.field, self.before[:20], self.after[:20]) class Meta: ordering = ['id'] @@ -135,7 +145,7 @@ def change(model, oid, field, old, new): # # The target model may be a non-Binder model (e.g. User), so lbyl. if hasattr(model, 'binder_serialize_m2m_field'): - old = model(id=oid).binder_serialize_m2m_field(field) + old = model(pk=oid).binder_serialize_m2m_field(field) _Transaction.changes[hid] = old, new, False @@ -153,7 +163,7 @@ def _commit(): if new is DeferredM2M: # The target model may be a non-Binder model (e.g. User), so lbyl. if hasattr(model, 'binder_serialize_m2m_field'): - new = model(id=oid).binder_serialize_m2m_field(field) + new = model(pk=oid).binder_serialize_m2m_field(field) _Transaction.changes[model, oid, field] = m2m_diff(old, new) # Filter non-changes @@ -181,7 +191,8 @@ def _commit(): change = Change( changeset=changeset, model=model.__name__, - oid=oid, + oid=oid if isinstance(oid, int) else -1, + oid_string=-1 if isinstance(oid, int) else str(oid), field=field, diff=diff, before=jsondumps(old), @@ -202,23 +213,23 @@ def _abort(): -def view_changesets(request, changesets, model_class, oid: int): +def view_changesets(request, changesets, model_class, oid): data = [] userids = set() diff_tracker = dict() for cs in changesets: changes = [] - for c in cs.changes.order_by('model', 'oid', 'field'): + for c in cs.changes.order_by('model', 'oid', 'oid_string', 'field'): after = model_class.format_field_for_history(field_name=c.field, raw_value=c.after, is_before=False, diff_tracker=diff_tracker, oid=oid) before = model_class.format_field_for_history(field_name=c.field, raw_value=c.before, is_before=True, diff_tracker=diff_tracker, oid=oid) - changes.append({'model': c.model, 'oid': c.oid, 'field': c.field, 'diff': c.diff, 'before': before, 'after': after}) - data.append({'date': cs.date, 'uuid': cs.uuid, 'id': cs.id, 'source': cs.source, 'user': cs.user_id, 'changes': changes}) + changes.append({'model': c.model, 'oid': c.choose_oid(), 'field': c.field, 'diff': c.diff, 'before': before, 'after': after}) + data.append({'date': cs.date, 'uuid': cs.uuid, 'id': cs.pk, 'source': cs.source, 'user': cs.user_id, 'changes': changes}) if cs.user_id: userids.add(cs.user_id) users = [] - for u in get_user_model().objects.filter(id__in=userids): - users.append({'id': u.id, 'username': u.username, 'email': u.email, 'first_name': u.first_name, 'last_name': u.last_name}) + for u in get_user_model().objects.filter(pk__in=userids): + users.append({'id': u.pk, 'username': u.username, 'email': u.email, 'first_name': u.first_name, 'last_name': u.last_name}) return JsonResponse({'data': data, 'with': {'user': users}}) @@ -228,13 +239,13 @@ def view_changesets_debug(request, changesets): body = ['', '', '', '', ''] for cs in changesets: username = cs.user.username if cs.user else None - body.append('

Changeset {} by {}: {} on {} {{{}}}'.format(cs.id, cs.source, username, cs.date.strftime('%Y-%m-%d %H:%M:%S'), cs.uuid)) + body.append('

Changeset {} by {}: {} on {} {{{}}}'.format(cs.pk, cs.source, username, cs.date.strftime('%Y-%m-%d %H:%M:%S'), cs.uuid)) body.append('

') body.append('') body.append('') - for c in cs.changes.order_by('model', 'oid', 'field'): + for c in cs.changes.order_by('model', 'oid', 'oid_string', 'field'): body.append(''.format( - c.model, c.oid, c.field, c.diff, c.before, c.after)) + c.model, c.choose_oid(), c.field, c.diff, c.before, c.after)) body.append('
modelobject idfieldbeforeafter
{}{}{}{}{}{}
') body.append('

') body.append('') diff --git a/binder/migrations/0005_change_oid_string.py b/binder/migrations/0005_change_oid_string.py new file mode 100644 index 00000000..b5edd963 --- /dev/null +++ b/binder/migrations/0005_change_oid_string.py @@ -0,0 +1,18 @@ +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('binder', '0004_history_changeset_change_date'), + ] + + operations = [ + migrations.AddField( + model_name='change', + name='oid_string', + field=models.TextField(db_index=True, blank=True), + ), + ] diff --git a/binder/models.py b/binder/models.py index 685694e0..3d8a02f8 100644 --- a/binder/models.py +++ b/binder/models.py @@ -240,9 +240,6 @@ class IntegerFieldFilter(FieldFilter): models.IntegerField, models.ForeignKey, models.AutoField, - models.ManyToOneRel, - models.ManyToManyField, - models.ManyToManyRel, ] allowed_qualifiers = [None, 'in', 'gt', 'gte', 'lt', 'lte', 'range', 'isnull'] @@ -461,6 +458,27 @@ def __new__(cls, name, bases, attrs): class BinderModel(models.Model, metaclass=BinderModelBase): + pk_regex = '[0-9]+' + """ + This regex determines what the router & view will consider to be a valid primary key. + + By default, this only allows integers, which makes sense for models that use an integer primary key, + which are nearly all our models. + - This would allow users to use the `/api/model/123/` endpoint to query model details, + but not `/api/model/abc/`. + - Likewise, it allows users to use `/api/model/123/detail-route`, + but not `/api/model/abc/detail-route`. + + When you use a model with a non-integer primary key, you may need to override this regex. + If you don't, the model detail endpoint and the detail route endpoints will be inaccessible. + + For instance, if you use a string primary key, you could change this regex to e.g. `[a-z|A-Z]+`, + which would allow users to query their details from `/api/model/abc`. + If you want to go wild, you could even use `.+`, which matches nearly any string, + including strings that contain slashes, whitespaces and dots. + (A few binder unit tests use such cursed primary keys.) + """ + def binder_concrete_fields_as_dict(self, skip_deferred_fields=False): fields = {} deferred_fields = self.get_deferred_fields() @@ -477,21 +495,21 @@ def binder_concrete_fields_as_dict(self, skip_deferred_fields=False): return fields @classmethod - def format_instance_for_history(cls, id: int): + def format_instance_for_history(cls, pk): """ This method is called during the history endpoint to determine the display name for related objects. By default, when model `A` has a foreign key field named `f` of type `B`, it will show a change that looks like ``` - { field: f, before: old_id, after: new_id } + { field: f, before: old_pk, after: new_pk } ``` - If you override this method, you can display something nicer than just the ID (e.g. the name). + If you override this method, you can display something nicer than just the pk (e.g. the name). """ - return str(id) + return pk @classmethod - def format_field_for_history(cls, field_name: str, raw_value: str, is_before: bool, diff_tracker: dict, oid: int): + def format_field_for_history(cls, field_name: str, raw_value: str, is_before: bool, diff_tracker: dict, oid): """ This method is called during the history endpoint to improve the way some of the changes in a changeset are displayed. Most fields are intuitive by default, but (m2m) relations need extra attention. @@ -499,19 +517,19 @@ def format_field_for_history(cls, field_name: str, raw_value: str, is_before: bo To demonstrate when this method is called, consider an example model `Zoo`: ``` class Zoo(BinderModel) - contacts = models.ManyToManyField('ContactPerson') + most_popular_animals = models.ManyToManyField('Animal', blank=True, related_name='+') ``` When the history endpoint of a zoo is invoked, the following calls to `format_field_history` could happen: - - `Zoo.format_field_for_history(field_name='contacts', raw_value='[[["id", 6]]]', is_before=False, ...)` - - `Zoo.format_field_for_history(field_name='contacts', raw_value='[[["id", 8]], [["id", 9]]]', is_before=True, ...)` + - `Zoo.format_field_for_history(field_name='most_popular_animals', raw_value='[6]', is_before=False, ...)` + - `Zoo.format_field_for_history(field_name='most_popular_animals', raw_value='[8, 9]', is_before=True, ...)` This would mean that: - - The `ContactPerson` with id 6 was added to `contacts`. - - The `ContactPerson`s with id's 8 and 9 were removed from `contacts`. - - The **existing** contacts are **omitted**. + - The `Animal` with id 6 was added to `most_popular_animals`. + - The `Animal`s with id's 8 and 9 were removed from `most_popular_animals`. + - The **existing** animals are **omitted**. - First of all, this method will do some bookkeeping to figure out the **existing** contacts. - Furthermore, it invokes `ContactPerson.format_instance_for_history(6)` to figure out the display name of `ContactPerson` 6, + First of all, this method will do some bookkeeping to figure out the **existing** animals. + Furthermore, it invokes `Animal.format_instance_for_history(6)` to figure out the display name of `Animal` 6, which will be shown instead of the raw ID. If you do **not** want this formatting (as was the case in older Binder versions), @@ -528,19 +546,19 @@ class Zoo(BinderModel) if field.is_relation: target_model = field.remote_field.model - def get_cached_display_name(target_id: int): + def get_cached_display_name(target_pk): if target_model not in diff_tracker: diff_tracker[target_model] = dict() display_name_cache = diff_tracker[target_model] - if target_id not in display_name_cache: - display_name_cache[target_id] = target_model.format_instance_for_history(target_id) - return display_name_cache[target_id] + if target_pk not in display_name_cache: + display_name_cache[target_pk] = target_model.format_instance_for_history(target_pk) + return display_name_cache[target_pk] if field.remote_field.multiple: if not is_before and field_name not in diff_tracker: try: - dict_id_list = list(cls.objects.filter(id=oid).values(field_name).all()) + dict_id_list = list(cls.objects.filter(pk=oid).values(field_name).all()) for index in range(len(dict_id_list)): dict_id_list[index] = dict_id_list[index][field_name] diff_tracker[field_name] = set(dict_id_list) @@ -552,12 +570,17 @@ def get_cached_display_name(target_id: int): entries = json.loads(raw_value) for entry in entries: - if len(entry) != 1: - raise ValueError() - entry = entry[0] - if len(entry) != 2 or entry[0] != 'id': - raise ValueError() - target_id = entry[1] + # Depending on the context (e.g. whether this is a backward, forward, or m2m relation), + # this `entry` can be either an integer or a tuple like `[['id', 'Rene']]`. + # + # Refactoring this would make sense, but is also going to be complicated, + # especially considering all the existing entries in changeset databases of downstream projects. + if isinstance(entry, list) and len(entry) == 1: + entry = entry[0] + if isinstance(entry, list) and len(entry) == 2 and entry[0] == 'id': + target_id = entry[1] + else: + target_id = entry if is_before: ids.append(target_id) diff_tracker[field_name].add(target_id) @@ -571,9 +594,9 @@ def get_cached_display_name(target_id: int): diff_tracker[target_model] = dict() result.append(get_cached_display_name(id)) - return ', '.join(result) + return ', '.join(str(r) for r in result) else: - return get_cached_display_name(int(raw_value)) + return get_cached_display_name(json.loads(raw_value)) return raw_value except Exception: # If we get an unexpected error when we try to format changes, @@ -595,10 +618,10 @@ def binder_serialize_m2m_field(self, field): # Regular many to many; get a list of the target ids. if not extended_m2m: - return set(field.values_list('id', flat=True)) + return set(field.values_list('pk', flat=True)) # Extended m2m; get dicts of the intermediary join table objects - data = list(field.through.objects.filter(**{field.source_field.name: self.id}).values()) + data = list(field.through.objects.filter(**{field.source_field.name: self.pk}).values()) # Then, modify them to leave out the PKs and source ids. Also, rename target ids to 'id'. for d in data: d.pop('id') @@ -727,7 +750,7 @@ def history_obj_m2m_changed(sender, instance, action, reverse, model, pk_set, ** # Find the corresponding field on the instance field = [f for f in instance._meta.get_fields() if f.concrete and f.many_to_many and f.remote_field.through == sender][0] - history.change(instance.__class__, instance.id, field.name, history.DeferredM2M, history.DeferredM2M) + history.change(instance.__class__, instance.pk, field.name, history.DeferredM2M, history.DeferredM2M) diff --git a/binder/permissions/views.py b/binder/permissions/views.py index 9f0b8e6e..abd794cb 100644 --- a/binder/permissions/views.py +++ b/binder/permissions/views.py @@ -294,7 +294,7 @@ def dispatch_file_field(self, request, pk=None, file_field=None): obj = pk else: try: - obj = self.get_queryset(request).get(pk=int(pk)) + obj = self.get_queryset(request).get(pk=pk) except ObjectDoesNotExist: raise BinderNotFound() diff --git a/binder/plugins/views/file_hash_view.py b/binder/plugins/views/file_hash_view.py index c7af605c..5f21a645 100644 --- a/binder/plugins/views/file_hash_view.py +++ b/binder/plugins/views/file_hash_view.py @@ -82,7 +82,7 @@ def dispatch_file_field(self, request, pk=None, file_field=None): obj = pk else: try: - obj = self.get_queryset(request).get(pk=int(pk)) + obj = self.get_queryset(request).get(pk=pk) except self.model.DoesNotExist: raise BinderNotFound() diff --git a/binder/plugins/views/userview.py b/binder/plugins/views/userview.py index 807a220c..a31128b0 100644 --- a/binder/plugins/views/userview.py +++ b/binder/plugins/views/userview.py @@ -44,7 +44,7 @@ def _maquerade_legacy(self, request, user_to_masquerade_as): """ from hijack.helpers import login_user login_user(request, user_to_masquerade_as) # Ignore returned redirect response object - return self.respond_with_user(request, user_to_masquerade_as.id) + return self.respond_with_user(request, user_to_masquerade_as.pk) @detail_route(name='masquerade') @no_scoping_required() @@ -73,7 +73,7 @@ def get_redirect_url(self): return '' AcquireUserViewAdapter().post(request) - return self.respond_with_user(request, user_to_masquerade_as.id) + return self.respond_with_user(request, user_to_masquerade_as.pk) def _end_masquerade(self, request) -> bool: """ @@ -110,8 +110,10 @@ def endmasquerade(self, request): self._require_model_perm('unmasquerade', request) - self._end_masquerade(request) - return self.respond_with_user(request, request.user.id) + if self._end_masquerade(request): + return self.respond_with_user(request, request.user.id) + else: + return HttpResponse(status=403) def _logout(self, request): if self._end_masquerade(request): @@ -136,7 +138,7 @@ def _require_model_perm(self, perm_type, request, pk=None): We need to be very careful about permission assumptions after this point """ # If the user is trying to change a superuser and is not a superuser, disallow - if pk and self.model.objects.get(pk=int(pk)).is_superuser and not request.user.is_superuser: + if pk and self.model.objects.get(pk=pk).is_superuser and not request.user.is_superuser: # Maybe BinderRequestError? raise BinderForbidden('modify superuser', request.user) @@ -209,8 +211,8 @@ def login(self, request): raise BinderNotAuthenticated() else: self.auth_login(request, user) - logger.info('login for {}/{}'.format(user.id, user)) - return self.respond_with_user(request, user.id) + logger.info('login for {}/{}'.format(user.pk, user)) + return self.respond_with_user(request, user.pk) def _logout(self, request): auth.logout(request) @@ -234,7 +236,7 @@ def logout(self, request): raise BinderMethodNotAllowed() self._require_model_perm('logout', request) - logger.info('logout for {}/{}'.format(request.user.id, request.user)) + logger.info('logout for {}/{}'.format(request.user.pk, request.user)) self._logout(request) return HttpResponse(status=204) @@ -441,12 +443,12 @@ def activate(self, request, pk=None): if user is None or not self.token_generator.check_token(user, body.get('activation_code')): raise BinderNotFound() - logger.info('login for {}/{} via successful activation'.format(user.id, user)) + logger.info('login for {}/{} via successful activation'.format(user.pk, user)) user.is_active = True user.save() self.auth_login(request, user) - return self.respond_with_user(request, user.id) + return self.respond_with_user(request, user.pk) @method_decorator(sensitive_post_parameters()) @method_decorator(never_cache) @@ -496,7 +498,7 @@ def _reset_pass_for_user(self, request, user_id, token, password): if user is None or not self.token_generator.check_token(user, token): raise BinderNotFound() - logger.info('login for {}/{} via successful password reset'.format(user.id, user)) + logger.info('login for {}/{} via successful password reset'.format(user.pk, user)) try: password_validation.validate_password(password, user) @@ -506,7 +508,7 @@ def _reset_pass_for_user(self, request, user_id, token, password): user.set_password(password) user.save() self.auth_login(request, user) - return self.respond_with_user(request, user.id) + return self.respond_with_user(request, user.pk) @method_decorator(sensitive_post_parameters()) @method_decorator(never_cache) @@ -561,7 +563,7 @@ def change_password(self, request): user.set_password(password) user.save() - logger.info('password changed for {}/{}'.format(user.id, user)) + logger.info('password changed for {}/{}'.format(user.pk, user)) if user == request.user: """ @@ -569,7 +571,7 @@ def change_password(self, request): """ update_session_auth_hash(request, user) - return self.respond_with_user(request, user.id) + return self.respond_with_user(request, user.pk) @abstractmethod def _after_soft_delete(self, request, user, undelete): diff --git a/binder/router.py b/binder/router.py index a98329fc..942d1c2f 100644 --- a/binder/router.py +++ b/binder/router.py @@ -124,20 +124,22 @@ def urls(self): urls = [] for route, view in self.route_views.items(): name = view.model.__name__ if view.model else route.route - # List and detail endpoints - if route.list_endpoint: - urls.append(re_path(r'^{}/$'.format(route.route), view.as_view(), {'router': self}, name=name)) - if route.detail_endpoint: - urls.append(re_path(r'^{}/(?P[0-9]+)/$'.format(route.route), view.as_view(), {'router': self}, name=name)) + + pk_regex = '[0-9]+' + try: + pk_regex = view.model.pk_regex + except AttributeError: + # Some models (e.g. built-in django models) don't inherit from BinderModel, hence don't have a pk_regex. + pass # History views if view.model and hasattr(view.model, 'Binder') and view.model.Binder.history: - urls.append(re_path(r'^{}/(?P[0-9]+)/history/$'.format(route.route), view.as_view(), {'history': 'normal', 'router': self}, name=name)) - urls.append(re_path(r'^{}/(?P[0-9]+)/history/debug/$'.format(route.route), view.as_view(), {'history': 'debug', 'router': self}, name=name)) + urls.append(re_path(r'^{}/(?P{})/history/$'.format(route.route, pk_regex), view.as_view(), {'history': 'normal', 'router': self}, name=name)) + urls.append(re_path(r'^{}/(?P{})/history/debug/$'.format(route.route, pk_regex), view.as_view(), {'history': 'debug', 'router': self}, name=name)) # File field endpoints for ff in view.file_fields: - urls.append(re_path(r'^{}/(?P[0-9]+)/{}/$'.format(route.route, ff), + urls.append(re_path(r'^{}/(?P{})/{}/$'.format(route.route, pk_regex, ff), view.as_view(), {'file_field': ff, 'router': self}, name='{}.{}'.format(name, ff))) # Custom endpoints @@ -150,10 +152,16 @@ def urls(self): if method.unauthenticated: kwargs['unauthenticated'] = True if hasattr(method, 'detail_route'): - urls.append(re_path(r'^{}/(?P[0-9]+)/{}/{}$'.format(route.route, route_name, extra), + urls.append(re_path(r'^{}/(?P{})/{}/{}$'.format(route.route, pk_regex, route_name, extra), view.as_view(), kwargs, name='{}.{}'.format(name, route_name))) if hasattr(method, 'list_route'): urls.append(re_path(r'^{}/{}/{}$'.format(route.route, route_name, extra), view.as_view(), kwargs, name='{}.{}'.format(name, route_name))) + # List and detail endpoints + if route.list_endpoint: + urls.append(re_path(r'^{}/$'.format(route.route), view.as_view(), {'router': self}, name=name)) + if route.detail_endpoint: + urls.append(re_path(r'^{}/(?P{})/$'.format(route.route, pk_regex), view.as_view(), {'router': self}, name=name)) + return urls diff --git a/binder/views.py b/binder/views.py index 5285e529..0803237a 100644 --- a/binder/views.py +++ b/binder/views.py @@ -483,7 +483,7 @@ def dispatch(self, request, *args, **kwargs): logger.info('request dispatch; verb={}, user={}/{}, path={}'. format( request.method, - request.user.id, + request.user.pk, request.user, request.path, )) @@ -689,7 +689,7 @@ def _get_objs(self, queryset, request, annotations=None, to_annotate={}): file = getattr(obj, f.attname) if file: # {router-view-instance} - data[f.name] = self.router.model_route(self.model, obj.id, f) + data[f.name] = self.router.model_route(self.model, obj.pk, f) # {duplicate-binder-file-field-hash-code} if isinstance(f, BinderFileField): data[f.name] += '?h={}&content_type={}&filename={}'.format( @@ -710,7 +710,8 @@ def _get_objs(self, queryset, request, annotations=None, to_annotate={}): data[prop] = getattr(obj, prop) if self.model._meta.pk.name in data: - data['id'] = data.pop(self.model._meta.pk.name) + # The following line may look a bit weird, but is needed for mobx-spine + data['id'] = data[self.model._meta.pk.name] datas.append(data) # order matters! datas_by_id[obj.pk] = data @@ -947,7 +948,7 @@ def withs_to_nested_set(withs, result={}): view.router = self.router for annotations, with_pks in annotation_ids.items(): objs = view._get_objs( - view.get_queryset(request).filter(pk__in=with_pks), + view.get_queryset(request).filter(pk__in=list(with_pks)), request=request, annotations=annotations, to_annotate={ @@ -1130,7 +1131,6 @@ def _get_with_ids(self, pks, request, include_annotations, with_map, where_map): view.model.objects .filter(prefix_q_expression(q, rev_field, field, view.model), **{rev_field + '__in': pks}) .values_list(rev_field + '__pk', 'pk') - .distinct() ) else: # Model default orders (this sometimes matters) @@ -1304,7 +1304,14 @@ def _filter_field(self, field_name, qualifier, value, invert, request, include_a return Q(**{partial + 'in': qs}) field = annotations[field_name]['field'] - for field_class in inspect.getmro(field.__class__): + # For foreign keys and similar fields, we should use the filter of the primary key of the related object + class_for_filtering = field.__class__ + if isinstance(field, models.ForeignObject) or isinstance(field, models.ManyToManyField) or isinstance(field, models.ForeignObjectRel): + class_for_filtering = field.related_model._meta.pk.__class__ + if hasattr(models, 'GeneratedField') and isinstance(field, models.GeneratedField): + class_for_filtering = field.output_field.__class__ + + for field_class in inspect.getmro(class_for_filtering): filter_class = self.get_field_filter(field_class) if filter_class: filter = filter_class(field) @@ -1359,7 +1366,7 @@ def _search_base(self, search, request): q = Q() for s, transform in dict( self.transformed_searches, - **{s: int if s == 'id' else str for s in self.searches} + **{s: int if s == 'id' else str for s in self.searches} # FIXME this may need to be adapted to support non-integer primary keys ).items(): try: q |= Q(**{s: transform(search)}) @@ -1553,7 +1560,7 @@ def _after_expr(self, request, after_id, include_annotations): queryset = queryset.annotate(**{name: annotations[name] for name in required_annotations}) try: - obj = queryset.get(pk=int(after_id)) + obj = queryset.get(pk=int(after_id)) # FIXME this may need to be adapted to support non-integer primary keys except (ValueError, self.model.DoesNotExist): raise BinderRequestError(f'invalid value for after_id: {after_id!r}') @@ -1622,7 +1629,7 @@ def _after_expr(self, request, after_id, include_annotations): def _get_filtered_queryset_base(self, request, pk=None, include_annotations=None): queryset = self.get_queryset(request) if pk: - queryset = queryset.filter(pk=int(pk)) + queryset = queryset.filter(pk=pk) # No parameter repetition. Should be extended to .params too after filters have been refactored. for k, v in request.GET.lists(): @@ -1705,7 +1712,7 @@ def get(self, request, pk=None, withs=None, include_annotations=None): ) # Now we add all remaining annotations to this data - data_by_pk = {obj['id']: obj for obj in data} + data_by_pk = {obj[self.model._meta.pk.name]: obj for obj in data} pks = set(data_by_pk) #### with @@ -1876,7 +1883,7 @@ def _store_m2m_field(self, obj, field, value, request): # ReverseManyToOneDescriptor. Yes, really. if getattr(obj._meta.model, field).__class__ == models.fields.related.ReverseManyToOneDescriptor: #### XXX FIXME XXX ugly quick fix for reverse relation + multiput issue - if any(v for v in value if v < 0): + if any(v for v in value if isinstance(v, int) and v < 0): return # If the m2m to be set is actually a reverse FK relation, we need to do extra magic. # We figure out if the remote objects are added or removed. The added ones, we modify/save @@ -1884,10 +1891,10 @@ def _store_m2m_field(self, obj, field, value, request): # doesn't see the changes. The same goes for the removed objects, except there we also # DELETE them if the FK is non-nullable. Interesting stuff. obj_field = getattr(obj, field) - old_ids = set(obj_field.values_list('id', flat=True)) + old_ids = set(obj_field.values_list('pk', flat=True)) new_ids = set(value) - rmobjs = obj_field.model.objects.filter(id__in=old_ids - new_ids) + rmobjs = obj_field.model.objects.filter(pk__in=list(old_ids - new_ids)) # Skip already softdeleted objects for models which # support softdeletion (this could be a lot of records) if any([f.name == 'deleted' for f in obj_field.model._meta.fields]): @@ -1911,7 +1918,7 @@ def _store_m2m_field(self, obj, field, value, request): rmobj_view.delete_obj(rmobj, False, request) - for addobj in obj_field.model.objects.filter(id__in=new_ids - old_ids): + for addobj in obj_field.model.objects.filter(pk__in=list(new_ids - old_ids)): setattr(addobj, obj_field.field.name, obj) try: addobj.save() @@ -2081,8 +2088,8 @@ def _store_field(self, obj, field, value, request, pk=None): for f in self.model._meta.fields: if f.name == field: if isinstance(f, models.ForeignKey): - if not (value is None or isinstance(value, int)): - raise BinderFieldTypeError(self.model.__name__, field) + if value is not None: + self._validate_value(f, field, value) # Previously, this value was updated using the following code: # - setattr(obj, f.attname, value) @@ -2211,13 +2218,15 @@ def delete_old_file(): for f in list(self.model._meta.many_to_many) + list(self._get_reverse_relations()): if f.name == field: # Force it to be seen as a deferred field - if isinstance(obj._meta.get_field(field), models.OneToOneRel): - if value is not None and not isinstance(value, int): + field_object = obj._meta.get_field(field) + if isinstance(field_object, models.OneToOneRel): + value = [value] + else: + if not isinstance(value, list): raise BinderFieldTypeError(self.model.__name__, field) + for v in value: + self._validate_value(field_object, field, v) - value = [value] - elif not (isinstance(value, list) and all(isinstance(v, int) for v in value)): - raise BinderFieldTypeError(self.model.__name__, field) # FIXME # Check if the ids being saved as m2m actually exist. This kinda sucks, it would be much # better to have this handled by the DB transaction layer. Which DOES actually check and @@ -2225,9 +2234,9 @@ def delete_old_file(): # So yeah, we kludge around here. :( #ids = set(value) #### XXX FIXME XXX ugly quick fix for reverse relation + multiput issue - ids = set(v for v in value if v is not None and v > 0) - ids -= set(obj._meta.get_field(field).remote_field.model.objects.filter(id__in=ids).values_list('id', flat=True)) - if ids: + pks = set(v for v in value if v is not None and ((not isinstance(v, int)) or v > 0)) + pks -= set(obj._meta.get_field(field).remote_field.model.objects.filter(pk__in=list(pks)).values_list('pk', flat=True)) + if pks: field_name = obj._meta.get_field(field).remote_field.model.__name__ model_name = self.get_model_view(obj.__class__)._model_name() raise BinderValidationError({ @@ -2235,9 +2244,9 @@ def delete_old_file(): obj.pk: { field: [{ 'code': 'does_not_exist', - 'message': '{} instances {} do not exist'.format(field_name, list(ids)), + 'message': '{} instances {} do not exist'.format(field_name, list(pks)), 'model': field_name, - 'values': list(ids) + 'values': list(pks) }] } } @@ -2246,7 +2255,16 @@ def delete_old_file(): raise BinderInvalidField(self.model.__name__, field) - + def _validate_value(self, field_object, field_name, candidate_value): + for field_class in inspect.getmro(field_object.related_model._meta.pk.__class__): + filter_class = self.get_field_filter(field_class) + if filter_class: + filter = filter_class(field_name) + try: + filter.clean_value(None, candidate_value) + except Exception: + # If the filter can't clean the value, we assume it's invalid + raise BinderFieldTypeError(self.model.__name__, field_name) def _require_model_perm(self, perm_type, request, pk=None): if hasattr(self, 'perms_via'): @@ -2327,6 +2345,8 @@ def _multi_put_parse_request(self, request): return data, deletions + def _is_classic_id(self, pk): + return (pk.name == 'id' and isinstance(pk, models.IntegerField)) or (pk.name.endswith('_ptr') and isinstance(pk, models.ForeignObject)) # Sort object values by model/id def _multi_put_collect_objects(self, data): @@ -2343,12 +2363,16 @@ def _multi_put_collect_objects(self, data): for idx, obj in enumerate(objs): if not isinstance(obj, dict): raise BinderRequestError('with.{}[{}] should be a dictionary'.format(modelname, idx)) - if not 'id' in obj: - raise BinderRequestError('missing id in with.{}[{}]'.format(modelname, idx)) - if not isinstance(obj['id'], int): - raise BinderRequestError('non-numeric id in with.{}[{}]'.format(modelname, idx)) - objects[(model, obj['id'])] = obj + if self._is_classic_id(model._meta.pk): + if 'id' not in obj: + raise BinderRequestError('missing id in with.{}[{}]'.format(modelname, idx)) + objects[(model, obj['id'])] = obj + else: + pk_name = model._meta.pk.name + if pk_name not in obj: + raise BinderRequestError('missing id in with.{}[{}]'.format(modelname, idx)) + objects[(model, obj[pk_name])] = obj return objects @@ -2402,13 +2426,13 @@ def _multi_put_override_superclass(self, objects): # Only look at relations that are included continue - if isinstance(data[field.name], int): + if isinstance(data[field.name], int): # FIXME this may need to be adapted to support non-integer primary keys target = (field.related_model, data[field.name]) if target in overrides: data[field.name] = overrides[target][1] elif isinstance(data[field.name], list): for i, mid in enumerate(data[field.name]): - if isinstance(mid, int): + if isinstance(mid, int): # FIXME this may need to be adapted to support non-integer primary keys target = (field.related_model, mid) if target in overrides: data[field.name][i] = overrides[target][1] @@ -2421,16 +2445,17 @@ def _multi_put_convert_backref_to_forwardref(self, objects): for field in filter(lambda f: f.one_to_many or f.one_to_one, model._meta.get_fields()): if field.name in values: if field.one_to_many: - if not isinstance(values[field.name], list) or not all(isinstance(v, int) for v in values[field.name]): + if not isinstance(values[field.name], list): raise BinderFieldTypeError(model.__name__, field.name) + for v in values[field.name]: + self._validate_value(field, field.name, v) rids = values[field.name] elif field.one_to_one: - if isinstance(values[field.name], int): + if values[field.name] is not None: + self._validate_value(field, field.name, values[field.name]) rids = [values[field.name]] - elif values[field.name] is None: - rids = [] else: - raise BinderFieldTypeError(model.__name__, field.name) + rids = [] for rid in rids: if (field.related_model, rid) in objects: @@ -2451,8 +2476,7 @@ def _multi_put_calculate_dependencies(self, objects): if isinstance(field, models.ForeignKey): if field.name in values: if values[field.name] is not None: - if not isinstance(values[field.name], int): - raise BinderRequestError('with.{}.{} should be an integer'.format(model.__name__, field.name)) + self._validate_value(field, field.name, values[field.name]) deps[field.related_model].append(values[field.name]) for field in model._meta.many_to_many: @@ -2460,7 +2484,7 @@ def _multi_put_calculate_dependencies(self, objects): if not isinstance(values[field.name], list): raise BinderRequestError('with.{}.{} should be a list'.format(model.__name__, field.name)) for i, v in enumerate(values[field.name]): - if not isinstance(multiput_get_id(v), int): + if not isinstance(multiput_get_id(v), int): # FIXME this may need to be adapted to support non-integer primary keys pass deps[field.related_model] += (values[field.name]) @@ -2501,7 +2525,7 @@ def _multi_put_order_dependencies(self, dependencies): ordered_objects += sorted( this_batch, - key=lambda obj: (obj[0].__name__, sign(obj[1]), abs(obj[1])), + key=lambda obj: (obj[0].__name__, sign(obj[1]) if isinstance(obj[1], int) else obj[1], abs(obj[1]) if isinstance(obj[1], int) else obj[1]), ) return ordered_objects @@ -2515,8 +2539,12 @@ def _multi_put_save_objects(self, ordered_objects, objects, request): # Gather non-negative oids per model (unordered) model_oids = defaultdict(set) for model, oid in ordered_objects: - if oid >= 0: - model_oids[model].add(oid) + if self._is_classic_id(model._meta.pk): + if oid >= 0: + model_oids[model].add(oid) + else: + if model.objects.filter(pk=oid).exists(): + model_oids[model].add(oid) # Do one big query to get and lock all the objects of each # type. This saves us from querying each individual object in @@ -2533,51 +2561,56 @@ def _multi_put_save_objects(self, ordered_objects, objects, request): for model, oid in ordered_objects: - values = objects[(model, oid)] + values = dict(objects[(model, oid)]) logger.info('Saving {} {}'.format(model.__name__, oid)) - if oid >= 0: + if self._is_classic_id(model._meta.pk): + if oid >= 0: + try: + obj = locked_objects[(model, oid)] + except KeyError: + raise BinderNotFound('{}[{}]'.format(model.__name__, oid)) + else: + obj = model() + del values['id'] + else: try: obj = locked_objects[(model, oid)] except KeyError: - raise BinderNotFound('{}[{}]'.format(model.__name__, oid)) - if hasattr(obj, 'deleted') and obj.deleted: - raise BinderIsDeleted() - else: - obj = model() + obj = model(**{ model._meta.pk.name: oid }) - values = dict(values) - del values['id'] + if hasattr(obj, 'deleted') and obj.deleted: + raise BinderIsDeleted() # FIXME for field in model._meta.fields: if isinstance(field, models.ForeignKey): if field.name in values: - if values[field.name] is not None and values[field.name] < 0: + if values[field.name] is not None and isinstance(values[field.name], int) and values[field.name] < 0: values[field.name] = new_id_map[(field.related_model, values[field.name])] for field in model._meta.many_to_many: if field.name in values: - values[field.name] = [(new_id_map[(field.related_model, multiput_get_id(i))] if multiput_get_id(i) < 0 else i) for i in values[field.name]] + values[field.name] = [(new_id_map[(field.related_model, multiput_get_id(i))] if isinstance(i, int) and multiput_get_id(i) < 0 else i) for i in values[field.name]] for field in [f for f in model._meta.get_fields() if f.one_to_many]: if field.name in values: - values[field.name] = [multiput_get_id(i) for i in values[field.name] if multiput_get_id(i) >= 0] + values[field.name] = [multiput_get_id(i) for i in values[field.name] if isinstance(multiput_get_id(i), int) and multiput_get_id(i) >= 0] view = self.get_model_view(model) try: view._store(obj, values, request, pk=oid) except BinderValidationError as e: validation_errors.append(e) - if oid < 0: - new_id_map[(model, oid)] = obj.id + if isinstance(oid, int) and oid < 0: + new_id_map[(model, oid)] = obj.pk for base in getmro(model)[1:]: if not ( hasattr(base, 'Meta') and getattr(base.Meta, 'abstract', False) ) and isinstance(base, BinderModel): - new_id_map[(base, oid)] = obj.id - logger.info('Saved as id {}'.format(obj.id)) + new_id_map[(base, oid)] = obj.pk + logger.info('Saved as id {}'.format(obj.pk)) if validation_errors: raise sum(validation_errors, None) @@ -2610,7 +2643,7 @@ def _multi_put_deletions(self, deletions, new_id_map, request): model_view = self.get_model_view(model) for i, pk in enumerate(pks): - if not isinstance(pk, int): + if not isinstance(pk, int): # FIXME this may need to be adapted to support non-integer primary keys raise BinderRequestError( 'with_deletions.{}[{}] should be a numeric id' .format(modelname, i) @@ -2734,7 +2767,7 @@ def put(self, request, pk=None): try: # Step one does the permission check. We cannot do a select for update here, since the queryeset # of get_queryset can potentially have outer joins on nullable values - obj = self.get_queryset(request).get(pk=int(pk)) + obj = self.get_queryset(request).get(pk=pk) # Now that we know we have access to this moel, we can get it again, this time with lock. obj = self.model.objects.select_for_update().get(pk=obj.pk) @@ -2743,7 +2776,7 @@ def put(self, request, pk=None): include_annotations = self._parse_include_annotations(request) annotations = include_annotations.get('') old = self._get_objs( - self.model.objects.filter(pk=int(pk)), + self.model.objects.filter(pk=pk), request=request, annotations=annotations, to_annotate={ @@ -2793,11 +2826,11 @@ def post(self, request, pk=None): new.pop('_meta', None) meta = data.setdefault('_meta', {}) - meta['with'], meta['with_mapping'], meta['with_related_name_mapping'], field_results = self._get_withs([new['id']], request=request, withs=None) + meta['with'], meta['with_mapping'], meta['with_related_name_mapping'], field_results = self._get_withs([new[self.model._meta.pk.name]], request=request, withs=None) self._annotate_obj_with_related_withs(data, field_results) - logger.info('POST created {} #{}'.format(self._model_name(), data['id'])) - for c in self._obj_diff({}, new, '{}[{}]'.format(self._model_name(), data['id'])): + logger.info('POST created {} #{}'.format(self._model_name(), data[self.model._meta.pk.name])) + for c in self._obj_diff({}, new, '{}[{}]'.format(self._model_name(), data[self.model._meta.pk.name])): logger.debug('POST ' + c) return JsonResponse(data) @@ -2823,7 +2856,7 @@ def delete(self, request, pk=None, undelete=False, skip_body_check=False): # This make sure we do all permission checks. We cannot do a select for update here, since there is a possiblity # that we create a queryset that we cannot use in a for select clause. try: - obj = self.get_queryset(request).get(pk=int(pk)) + obj = self.get_queryset(request).get(pk=pk) except ObjectDoesNotExist: raise BinderNotFound() # We now retrieve the model again, without the permission checks, which we already this. @@ -2857,13 +2890,13 @@ def soft_delete(self, obj, undelete, request): except models.ProtectedError as e: protected_objects = defaultdict(list) for prot in e.protected_objects: - protected_objects[self.get_model_view(prot.__class__)._model_name()] += [prot.id] + protected_objects[self.get_model_view(prot.__class__)._model_name()] += [prot.pk] raise BinderValidationError({ self._model_name(): { obj.pk: { 'id': [{ 'code': 'protected', - 'message': 'Could not delete object {}'.format(obj.id), + 'message': 'Could not delete object {}'.format(obj.pk), 'objects': protected_objects, }] } @@ -2888,7 +2921,7 @@ def dispatch_file_field(self, request, pk=None, file_field=None): pk = obj.pk else: try: - obj = self.get_queryset(request).get(pk=int(pk)) + obj = self.get_queryset(request).get(pk=pk) except ObjectDoesNotExist: raise BinderNotFound() @@ -2943,7 +2976,7 @@ def dispatch_file_field(self, request, pk=None, file_field=None): self._store(obj, {file_field_name: file}, request, pk=pk) field = self.model._meta.get_field(file_field_name) - path = self.router.model_route(self.model, obj.id, field) + path = self.router.model_route(self.model, obj.pk, field) # {duplicate-binder-file-field-hash-code} if isinstance(field, BinderFileField): file_field = getattr(obj, file_field_name) @@ -2987,7 +3020,17 @@ def view_history(self, request, pk=None, **kwargs): logger.warning('Debug endpoints disabled.') return HttpResponseForbidden('Debug endpoints disabled.') - changesets = history.Changeset.objects.filter(id__in=set(history.Change.objects.filter(model=self.model.__name__, oid=pk).values_list('changeset_id', flat=True))) + query_params = { + 'model': self.model.__name__ + } + + if isinstance(self.model._meta.pk, models.IntegerField) or isinstance(self.model._meta.pk, models.ForeignObject): + query_params['oid'] = pk + else: + query_params['oid_string'] = pk + changesets = history.Changeset.objects.filter(pk__in=set( + history.Change.objects.filter(**query_params).values_list('changeset_id', flat=True) + )) if debug: return history.view_changesets_debug(request, changesets.order_by('-id')) else: diff --git a/docker-compose.yml b/docker-compose.yml index efbdbbe6..9b604bd0 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -1,6 +1,6 @@ services: db: - image: postgres:13.21 + image: postgres:14.23 environment: - POSTGRES_HOST_AUTH_METHOD=trust # Insecure, but fine for just for running tests. binder: diff --git a/docs/models.md b/docs/models.md index 680b6234..3283a9df 100644 --- a/docs/models.md +++ b/docs/models.md @@ -106,7 +106,7 @@ from binder.models import BinderModel class Animal(BinderModel): name = models.TextField() secret_notes = models.TextField() # This field won't be tracked - + class Binder: history = True exclude_history_fields = ['secret_notes'] @@ -164,9 +164,36 @@ you need to override the `format_instance_for_history` method on the target mode ```python @classmethod -def format_instance_for_history(cls, id: int): +def format_instance_for_history(cls, pk): try: - return ContactPerson.objects.get(id=id).name + return ContactPerson.objects.get(pk=pk).name except: - return 'deleted? ' + str(id) + return 'deleted? ' + str(pk) +``` + +## Non-integer primary keys +If you use non-integer primary keys, you can override the `pk_regex` (default `[0-9]`) +to make sure the router accepts them. For instance: +```python +class ContactPerson(BinderModel): + name = models.CharField(primary_key=True, max_length=50) + + pk_regex = '.*' ``` +This `pk_regex` determines what the router & view will consider to be a valid primary key. + +The default `pk_regex` only allows integer IDs, which makes sense for models that use an integer primary key, +which are nearly all our models. +- This would allow users to use the `/api/model/123/` endpoint to query model details, +but not `/api/model/abc/`. +- Likewise, it allows users to use `/api/model/123/detail-route`, +but not `/api/model/abc/detail-route`. + +When you use a model with a non-integer primary key, you may need to override this regex. +If you don't, the model detail endpoint and the detail route endpoints will be inaccessible. + +For instance, if you use a string primary key, you could change this regex to e.g. `[a-z|A-Z]+`, +which would allow users to query their details from `/api/model/abc`. +If you want to go wild, you could even use `.+`, which matches nearly any string, +including strings that contain slashes, whitespaces and dots. +(A few binder unit tests use such cursed primary keys.) diff --git a/tests/filters/test_generated_field_filtering.py b/tests/filters/test_generated_field_filtering.py new file mode 100644 index 00000000..3e746228 --- /dev/null +++ b/tests/filters/test_generated_field_filtering.py @@ -0,0 +1,38 @@ +import unittest, os + +from django.test import TestCase, Client +from django.contrib.auth.models import User + +from binder.json import jsonloads + +from ..testapp.models import Zoo + + +@unittest.skipIf( + 'DJANGO_VERSION' in os.environ and tuple(map(int, os.environ['DJANGO_VERSION'].split('.'))) < (5, 0, 0), + 'Only available in Django 5+' +) +class GeneratedFieldFiltersTest(TestCase): + + def setUp(self): + super().setUp() + u = User(username='testuser', is_active=True, is_superuser=True) + u.set_password('test') + u.save() + + self.client = Client() + r = self.client.login(username='testuser', password='test') + self.assertTrue(r) + + Zoo(name='Burgers Zoo').save() + Zoo(name='Artis').save() + Zoo(name='Apenheul').save() + Zoo(name='Ouwehand Zoo').save() + + def test_filter(self): + response = self.client.get('/zoo/', data={'.upper_name:contains': 'BURGER'}) + self.assertEqual(response.status_code, 200) + result = jsonloads(response.content) + self.assertEqual(1, len(result['data'])) + self.assertEqual('Burgers Zoo', result['data'][0]['name']) + self.assertEqual('BURGERS ZOO', result['data'][0]['upper_name']) diff --git a/tests/test_composite_primary_key.py b/tests/test_composite_primary_key.py new file mode 100644 index 00000000..92af13f7 --- /dev/null +++ b/tests/test_composite_primary_key.py @@ -0,0 +1,68 @@ +from datetime import date +from django.contrib.auth.models import User +from django.test import Client, TestCase +import json +import os +import unittest +from .testapp.models import ContactPerson, ContactPersonRating, Zoo + + +@unittest.skipIf( + 'DJANGO_VERSION' in os.environ and tuple(map(int, os.environ['DJANGO_VERSION'].split('.'))) < (5, 2, 0), + 'Only available from Django >= 5.2' +) +class CompositePrimaryKeyTest(TestCase): + + def setUp(self): + super().setUp() + u = User(username='testuser', is_active=True, is_superuser=True) + u.set_password('test') + u.save() + self.client = Client() + r = self.client.login(username='testuser', password='test') + self.assertTrue(r) + + + def test_create(self): + ContactPerson.objects.create(name='Jantje') + creation_data = { + 'contact_person': 'Jantje', + 'date': '2026-07-08', + 'rating': 8, + } + response = self.client.post('/contact_person_rating/', data=json.dumps(creation_data), content_type='application/json') + self.assertEqual(response.status_code, 200) + self.assertEqual(8, ContactPersonRating.objects.get().rating) + + def test_list(self): + jantje = ContactPerson.objects.create(name='Jantje') + ContactPersonRating.objects.create(contact_person=jantje, date=date(2026, 1, 1), rating=6) + ContactPersonRating.objects.create(contact_person=jantje, date=date(2026, 1, 2), rating=7) + zoo = Zoo.objects.create(name='Dorpje') + zoo.contacts.add(jantje) + + # Test whether we can list the ratings directly + response = self.client.get('/contact_person_rating/') + self.assertEqual(response.status_code, 200) + data = json.loads(response.content)['data'] + self.assertEqual('Jantje', data[0]['contact_person']) + self.assertEqual('2026-01-01', data[0]['date']) + self.assertEqual(6, data[0]['rating']) + self.assertEqual('Jantje', data[1]['contact_person']) + self.assertEqual('2026-01-02', data[1]['date']) + self.assertEqual(7, data[1]['rating']) + + # Test whether we can list the ratings indirectly + response = self.client.get('/contact_person/?with=ratings') + self.assertEqual(response.status_code, 200) + content = json.loads(response.content) + self.assertEqual(1, len(content['data'])) + person = content['data'][0] + self.assertEqual('Jantje', person['name']) + self.assertEqual([['Jantje', '2026-01-01'], ['Jantje', '2026-01-02']], person['ratings']) + rating = content['with']['contact_person_rating'][0] + self.assertEqual(['Jantje', '2026-01-01'], rating['pk']) + self.assertEqual(['Jantje', '2026-01-01'], rating['id']) + self.assertEqual('Jantje', rating['contact_person']) + self.assertEqual('2026-01-01', rating['date']) + self.assertEqual(6, rating['rating']) diff --git a/tests/test_filter_alternative_filters.py b/tests/test_filter_alternative_filters.py index 68c5e710..83f913fe 100644 --- a/tests/test_filter_alternative_filters.py +++ b/tests/test_filter_alternative_filters.py @@ -16,20 +16,24 @@ def setUp(self): r = self.client.login(username='testuser', password='test') self.assertTrue(r) - ContactPerson(id=1, name='contact1').save() - ContactPerson(id=2, name='contact2').save() - ContactPerson(id=3, name='zoo3').save() + ContactPerson(name='contact1').save() + ContactPerson(name='contact2').save() + + # Yes... we are really creating a contact person with name zoo3; NOT a zoo with name zoo3. + # This is needed for the `test_alt_filters_not_all` test. + ContactPerson(name='zoo3').save() + z1 = Zoo(id=1, name='zoo1') z1.save() - z1.contacts.set([1, 2]) + z1.contacts.set(['contact1', 'contact2']) z2 = Zoo(id=2, name='zoo2') z2.save() - z2.contacts.set([2]) + z2.contacts.set(['contact2']) z3 = Zoo(id=3, name='zoo3') z3.save() - z3.contacts.set([3]) + z3.contacts.set(['zoo3']) def test_filter_alternative_contacts_one_foreign_field(self): response = self.client.get('/zoo/?.all_contact_name:icontains=contact1') diff --git a/tests/test_filter_manyrels.py b/tests/test_filter_manyrels.py index a40349a8..b8a66b7c 100644 --- a/tests/test_filter_manyrels.py +++ b/tests/test_filter_manyrels.py @@ -18,10 +18,10 @@ def setUp(self): r = self.client.login(username='testuser', password='test') self.assertTrue(r) - ContactPerson(id=1, name='contact1').save() + ContactPerson(name='contact1').save() z = Zoo(id=1, name='zoo') z.save() - z.contacts.set([1]) + z.contacts.set(['contact1']) Animal(id=1, name='animal', zoo_id=1).save() Animal(id=2, name='animal2', zoo_id=1).save() @@ -87,7 +87,7 @@ def test_filter_fk_backward_distinct(self): def test_filter_m2m_forward(self): - response = self.client.get('/zoo/?.contacts=1') + response = self.client.get('/zoo/?.contacts=contact1') self.assertEqual(response.status_code, 200) returned_data = jsonloads(response.content) @@ -95,7 +95,7 @@ def test_filter_m2m_forward(self): 'data': [ { 'id': 1, - 'contacts': [1], + 'contacts': ['contact1'], EXTRA(): None, # Other fields are dontcare } ], @@ -120,13 +120,13 @@ def test_m2m_with_multiple_relations(self): 'zoo': [ { 'id': 1, - 'contacts': [1], + 'contacts': ['contact1'], EXTRA(): None, }, ], 'contact_person': [ { - 'id': 1, + 'name': 'contact1', EXTRA(): None, } ] @@ -147,10 +147,10 @@ def test_m2m_with_multiple_relations(self): # Now ensure we don't put both contact person lists on the same pile - ContactPerson(id=2, name='contact2').save() + ContactPerson(name='contact2').save() z2 = Zoo(id=2, name='zoo2') z2.save() - z2.contacts.set([2]) + z2.contacts.set(['contact2']) animal = Animal.objects.get(pk=1) animal.zoo_of_birth = z2 animal.save() @@ -167,22 +167,22 @@ def test_m2m_with_multiple_relations(self): 'zoo': [ { 'id': 1, - 'contacts': [1], + 'contacts': ['contact1'], EXTRA(): None, }, { 'id': 2, - 'contacts': [2], + 'contacts': ['contact2'], EXTRA(): None, }, ], 'contact_person': [ { - 'id': 1, + 'name': 'contact1', EXTRA(): None, }, { - 'id': 2, + 'name': 'contact2', EXTRA(): None, }, ] @@ -210,7 +210,7 @@ def test_filter_m2m_backward(self): assert_json(returned_data, { 'data': [ { - 'id': 1, + 'name': 'contact1', 'zoos': [1], EXTRA(): None, # Other fields are dontcare } diff --git a/tests/test_filterable_relations.py b/tests/test_filterable_relations.py index 1e17fec5..e0305ea8 100644 --- a/tests/test_filterable_relations.py +++ b/tests/test_filterable_relations.py @@ -273,7 +273,7 @@ def test_m2m(self): cp3 = ContactPerson(name='hans') cp3.save() - zoo.contacts.set([cp1.id, cp2.id, cp3.id]) + zoo.contacts.set([cp1.pk, cp2.pk, cp3.pk]) res = self.client.get('/zoo/', data={ 'with': 'contacts', @@ -286,18 +286,18 @@ def test_m2m(self): 'data': [ { 'id': zoo.id, - 'contacts': [cp1.id, cp2.id], + 'contacts': [cp1.pk, cp2.pk], EXTRA(): None, } ], 'with': { 'contact_person': [ { - 'id': cp1.id, + 'name': cp1.pk, EXTRA(): None, }, { - 'id': cp2.id, + 'name': cp2.pk, EXTRA(): None, }, ] @@ -317,7 +317,7 @@ def test_where_filter_on_m2m_field_causes_no_duplication_of_main_record(self): cp3 = ContactPerson(name='hans') cp3.save() - zoo.contacts.set([cp1.id, cp2.id, cp3.id]) + zoo.contacts.set([cp1.pk, cp2.pk, cp3.pk]) res = self.client.get('/zoo/', data={ '.contacts.name:startswith': 'he' }) self.assertEqual(res.status_code, 200) @@ -381,14 +381,14 @@ def test_where_complains_on_non_relation_field(self): def test_where_complains_on_invalid_value(self): res = self.client.get('/zoo/', data={ - 'with': 'contacts', - 'where': 'contacts(id=foo)' + 'with': 'most_popular_animals', + 'where': 'most_popular_animals(id=foo)' }) self.assertEqual(res.status_code, 418) res = jsonloads(res.content) assert_json(res, { - 'message': 'Invalid value {foo} for AutoField {ContactPerson}.{id}.', + 'message': 'Invalid value {foo} for AutoField {Animal}.{id}.', EXTRA(): None, }) diff --git a/tests/test_history.py b/tests/test_history.py index 33d3b3eb..6d95a317 100644 --- a/tests/test_history.py +++ b/tests/test_history.py @@ -1,5 +1,5 @@ from datetime import datetime, timedelta, timezone -import json +import os, json, unittest from django.test import TestCase, Client from django.contrib.auth.models import User @@ -7,7 +7,7 @@ from binder import history from binder.history import Change, Changeset -from .testapp.models import Animal, Caretaker, ContactPerson, Zoo +from .testapp.models import Animal, Caretaker, ContactPerson, ContactPersonRating, Zoo class HistoryTest(TestCase): @@ -66,13 +66,15 @@ def test_m2m_using_binder_through_history_doesnt_crash(self): self.assertEqual(1, len(data[0]['changes'])) add_rene = data[0]['changes'][0] self.assertEqual('contacts', add_rene['field']) - self.assertEqual('Burhan, Rene', add_rene['after']) - self.assertEqual('Burhan', add_rene['before']) + self.assertEqual('BURHAN, RENE', add_rene['after']) + self.assertEqual('BURHAN', add_rene['before']) - self.assertEqual(13, len(data[1]['changes'])) + # The exact number of changes depends on the upper_name field of Zoo, which depends on the Django version. + self.assertGreaterEqual(len(data[1]['changes']), 13) + self.assertLessEqual(len(data[1]['changes']), 14) add_burhan = data[1]['changes'][4] self.assertEqual('contacts', add_burhan['field']) - self.assertEqual('Burhan', add_burhan['after']) + self.assertEqual('BURHAN', add_burhan['after']) self.assertEqual('', add_burhan['before']) def test_basic_relation_formatting(self): @@ -96,8 +98,8 @@ def test_basic_relation_formatting(self): changes = data[0]['changes'] set_director = changes[4] self.assertEqual('director', set_director['field']) - self.assertEqual('null', set_director['before']) - self.assertEqual('Burhan', set_director['after']) + self.assertIsNone(set_director['before']) + self.assertEqual('BURHAN', set_director['after']) model_data = { 'name': 'Code Yellow', @@ -114,31 +116,31 @@ def test_basic_relation_formatting(self): changes = data[0]['changes'] set_director = changes[0] self.assertEqual('director', set_director['field']) - self.assertEqual('Burhan', set_director['before']) - self.assertEqual('null', set_director['after']) + self.assertEqual('BURHAN', set_director['before']) + self.assertIsNone(set_director['after']) def test_m2m_relation_formatting(self): - burhan = ContactPerson.objects.create(name='Burhan') - rene = ContactPerson.objects.create(name='Rene') - tim = ContactPerson.objects.create(name='Tim') - nuria = ContactPerson.objects.create(name='Nuria') + atari = Animal.objects.create(name='Atari') + yuzu = Animal.objects.create(name='Yuzu') + zuki = Animal.objects.create(name='Zuki') + moxy = Animal.objects.create(name='Moxy') model_data = { 'name': 'Code Yellow', - 'contacts': [burhan.id, rene.id] + 'most_popular_animals': [atari.pk, yuzu.pk] } response = self.client.post('/zoo/', data=json.dumps(model_data), content_type='application/json') self.assertEqual(response.status_code, 200) zoo_id = json.loads(response.content)['id'] model_data = { - 'contacts': [rene.id, tim.id, nuria.id] + 'most_popular_animals': [yuzu.pk, zuki.pk, moxy.pk] } response = self.client.put(f'/zoo/{zoo_id}/', data=json.dumps(model_data), content_type='application/json') self.assertEqual(response.status_code, 200) model_data = { - 'contacts': [burhan.id, nuria.id] + 'most_popular_animals': [atari.pk, moxy.pk] } response = self.client.put(f'/zoo/{zoo_id}/', data=json.dumps(model_data), content_type='application/json') self.assertEqual(response.status_code, 200) @@ -150,21 +152,21 @@ def test_m2m_relation_formatting(self): remove_all = data[0]['changes'] self.assertEqual(1, len(remove_all)) - self.assertEqual('contacts', remove_all[0]['field']) - self.assertEqual('Burhan, Nuria', remove_all[0]['after']) - self.assertEqual('Rene, Tim, Nuria', remove_all[0]['before']) + self.assertEqual('most_popular_animals', remove_all[0]['field']) + self.assertEqual(f'Atari, {moxy.pk}', remove_all[0]['after']) + self.assertEqual(f'Yuzu, Zuki, {moxy.pk}', remove_all[0]['before']) replace = data[1]['changes'] self.assertEqual(1, len(replace)) - self.assertEqual('contacts', replace[0]['field']) - self.assertEqual('Rene, Tim, Nuria', replace[0]['after']) - self.assertEqual('Burhan, Rene', replace[0]['before']) + self.assertEqual('most_popular_animals', replace[0]['field']) + self.assertEqual(f'Yuzu, Zuki, {moxy.pk}', replace[0]['after']) + self.assertEqual('Atari, Yuzu', replace[0]['before']) initial = data[2]['changes'] - first_contacts = initial[4] - self.assertEqual('contacts', first_contacts['field']) - self.assertEqual('Burhan, Rene', first_contacts['after']) - self.assertEqual('', first_contacts['before']) + first_animals = initial[10] + self.assertEqual('most_popular_animals', first_animals['field']) + self.assertEqual('Atari, Yuzu', first_animals['after']) + self.assertEqual('', first_animals['before']) def test_model_with_history_creates_changes_on_creation(self): model_data = { @@ -351,7 +353,7 @@ def test_exclude_history_fields_prevents_tracking(self): """Test that fields listed in exclude_history_fields are not tracked in history""" original_exclude_fields = getattr(Animal.Binder, 'exclude_history_fields', []) Animal.Binder.exclude_history_fields = ['name'] - + try: model_data = { 'name': 'Test Animal', @@ -359,30 +361,30 @@ def test_exclude_history_fields_prevents_tracking(self): response = self.client.post('/animal/', data=json.dumps(model_data), content_type='application/json') self.assertEqual(response.status_code, 200) animal_id = json.loads(response.content)['id'] - + # Check that we have a changeset for the creation self.assertEqual(1, Changeset.objects.count()) cs = Changeset.objects.get() - + # Check that changes exist for normal fields but not for excluded fields changes = Change.objects.filter(changeset=cs) field_names = [change.field for change in changes] - + # The 'name' field should be excluded from history self.assertNotIn('name', field_names) # Other fields should still be tracked self.assertIn('id', field_names) self.assertIn('deleted', field_names) - + model_data = { 'name': 'Updated Animal Name', } response = self.client.patch(f'/animal/{animal_id}/', data=json.dumps(model_data), content_type='application/json') self.assertEqual(response.status_code, 200) - + # Should still only have 1 changeset (no new one created for excluded field) self.assertEqual(1, Changeset.objects.count()) - + # Now update both an excluded field AND a non-excluded field zoo = Zoo.objects.create(name='Test Zoo') model_data = { @@ -391,20 +393,52 @@ def test_exclude_history_fields_prevents_tracking(self): } response = self.client.patch(f'/animal/{animal_id}/', data=json.dumps(model_data), content_type='application/json') self.assertEqual(response.status_code, 200) - + # Now we should have 2 changesets (original creation + this mixed update) self.assertEqual(2, Changeset.objects.count()) - + # Check the latest changeset (for the mixed update) latest_cs = Changeset.objects.order_by('-id').first() latest_changes = Change.objects.filter(changeset=latest_cs) latest_field_names = [change.field for change in latest_changes] - + # Only the zoo field should be recorded, not the name self.assertEqual(1, len(latest_field_names)) self.assertIn('zoo', latest_field_names) self.assertNotIn('name', latest_field_names) - + finally: # Restore original exclude_history_fields setting - Animal.Binder.exclude_history_fields = original_exclude_fields \ No newline at end of file + Animal.Binder.exclude_history_fields = original_exclude_fields + + @unittest.skipIf( + 'DJANGO_VERSION' in os.environ and tuple(map(int, os.environ['DJANGO_VERSION'].split('.'))) < (5, 2, 0), + 'Only available from Django >= 5.2' + ) + def test_non_integer_primary_keys(self): + self.assertEqual(200, self.client.post( + '/contact_person/', + data=json.dumps({ 'name': 'Tim' }), + content_type='application/json' + ).status_code) + self.assertEqual(200, self.client.post( + '/contact_person_rating/', + data=json.dumps({ 'contact_person': 'Tim', 'rating': 9, 'date': '2026-07-09' }), + content_type='application/json' + ).status_code) + self.assertEqual(200, self.client.post( + '/contact_person_rating/', + data=json.dumps({ 'contact_person': 'Tim', 'rating': 10, 'date': '2026-07-10' }), + content_type='application/json' + ).status_code) + response = self.client.get('/contact_person/Tim/history/') + self.assertEqual(200, response.status_code) + + data = json.loads(response.content)['data'] + self.assertEqual(1, len(data)) + changes = data[0]['changes'] + self.assertEqual(6, len(changes)) + for change in changes: + self.assertEqual('Tim', change['oid']) + + # FIXME Also query the history of the ratings, but that's not yet supported diff --git a/tests/test_masquerade.py b/tests/test_masquerade.py index 3529224d..98bc7da0 100644 --- a/tests/test_masquerade.py +++ b/tests/test_masquerade.py @@ -50,6 +50,10 @@ def logout(self): res = self.client.post('/user/logout/') self.assertEqual(res.status_code, 204) + def endMasquerade(self): + res = self.client.post('/user/endmasquerade/') + self.assertEqual(res.status_code, 200) + # Asserts def assertLoggedIn(self, username=None): res = self.client.get('/user/identify/') @@ -97,3 +101,18 @@ def test_masquerade_non_superuser(self): self.assertLoggedIn('user3') self.logout() self.assertLoggedOut() + + def test_endmasquerade(self): + self.assertLoggedOut() + self.login('user1') + self.assertLoggedIn('user1') + self.masquerade('user2') + self.assertLoggedIn('user2') + self.masquerade('user3') + self.assertLoggedIn('user3') + self.endMasquerade() + self.assertLoggedIn('user2') + self.endMasquerade() + self.assertLoggedIn('user1') + res = self.client.post('/user/endmasquerade/') + self.assertEqual(403, res.status_code) diff --git a/tests/test_multi_put.py b/tests/test_multi_put.py index 533d5d51..19a44ae7 100644 --- a/tests/test_multi_put.py +++ b/tests/test_multi_put.py @@ -509,7 +509,7 @@ def test_update_model_with_m2m_field_causes_no_error(self): # Now from the other end model_data = { 'data': [{ - 'id': contact1.id, + 'name': contact1.pk, 'zoos': [], }] } diff --git a/tests/test_non_integer_primary_keys.py b/tests/test_non_integer_primary_keys.py new file mode 100644 index 00000000..12f8de78 --- /dev/null +++ b/tests/test_non_integer_primary_keys.py @@ -0,0 +1,118 @@ +from django.contrib.auth.models import User +from django.test import Client, TestCase +import json +from .testapp.models import ContactPerson, Zoo + + +class StringPrimaryKeyTest(TestCase): + + def setUp(self): + super().setUp() + u = User(username='testuser', is_active=True, is_superuser=True) + u.set_password('test') + u.save() + self.client = Client() + r = self.client.login(username='testuser', password='test') + self.assertTrue(r) + + def test_create_update_delete(self): + creation_data = { + 'name': 'Jantje', + } + response = self.client.post('/contact_person/', data=json.dumps(creation_data), content_type='application/json') + self.assertEqual(response.status_code, 200) + self.assertEqual('Jantje', ContactPerson.objects.get().name) + + update_data = { + 'name': 'Jantje', + 'nick_name': 'Pietje', + } + response = self.client.patch('/contact_person/Jantje/', data=json.dumps(update_data), content_type='application/json') + self.assertEqual(response.status_code, 200) + self.assertEqual('Jantje', ContactPerson.objects.get().name) + self.assertEqual('Pietje', ContactPerson.objects.get().nick_name) + + response = self.client.delete('/contact_person/Jantje/', data={}, content_type='application/json') + self.assertEqual(response.status_code, 204) + self.assertEqual(0, ContactPerson.objects.all().count()) + + def test_attach_in_multi_put1(self): + ContactPerson.objects.create(name='Tim') + ContactPerson.objects.create(name='Pieter') + creation_data = { + 'data': [ + { + 'id': -1, + 'name': 'Beekse Bergen', + 'director': 'Tim', + 'contacts': ['Tim', 'Pieter'], + }, + ] + } + response = self.client.put('/zoo/', data=json.dumps(creation_data)) + self.assertEqual(response.status_code, 200) + self.assertEqual('Tim', Zoo.objects.get().director.name) + self.assertEqual(['Pieter', 'Tim'], list(Zoo.objects.get().contacts.all().values_list('name', flat=True))) + + def test_attach_in_multi_put2(self): + ContactPerson.objects.create(name='Tim') + ContactPerson.objects.create(name='Pieter') + Zoo.objects.create(id=1, name='Beekse Bergen') + creation_data = { + 'data': [ + { + 'id': 1, + 'director': 'Tim', + 'contacts': ['Tim', 'Pieter'], + }, + ] + } + response = self.client.put('/zoo/', data=json.dumps(creation_data)) + self.assertEqual(response.status_code, 200) + self.assertEqual('Tim', Zoo.objects.get().director.name) + self.assertEqual(['Pieter', 'Tim'], list(Zoo.objects.get().contacts.all().values_list('name', flat=True))) + + def test_create_and_edit_in_multi_put(self): + ContactPerson.objects.create(name='Tim') + ContactPerson.objects.create(name='Bob') + other_zoo = Zoo.objects.create(name='lalala') + creation_data = { + 'data': [ + { + 'id': -1, + 'name': 'Beekse Bergen', + 'director': 'Tim', + 'contacts': ['Tim', 'Pieter'], + 'originals': ['Nuria', 'Bob'], + }, + ], + 'with': { + 'contact_person': [ + { 'name': 'Tim', 'nick_name': 'knokko', 'successor': 'Pieter', 'ratings': [] }, + { 'name': 'Pieter', 'nick_name': 'Pietje', 'first_zoo': other_zoo.id }, + { 'name': 'Bob', 'nick_name': 'Gigolo Bob' }, + { 'name': 'Nuria', 'successor': 'Tim' }, + ] + } + } + response = self.client.put('/zoo/', data=json.dumps(creation_data)) + self.assertEqual(response.status_code, 200) + self.assertEqual('Tim', Zoo.objects.get(name='Beekse Bergen').director.name) + self.assertEqual(['Pieter', 'Tim'], list(Zoo.objects.get(name='Beekse Bergen').contacts.all().values_list('name', flat=True))) + self.assertEqual('knokko', ContactPerson.objects.get(name='Tim').nick_name) + self.assertEqual('Pietje', ContactPerson.objects.get(name='Pieter').nick_name) + id_map = json.loads(response.content)['idmap'] + self.assertEqual(1, len(id_map)) + self.assertEqual(1, len(id_map['zoo'])) + self.assertEqual(Zoo.objects.get(name='Beekse Bergen').id, id_map['zoo'][0][-1]) + self.assertEqual('lalala', ContactPerson.objects.get(name='Pieter').first_zoo.name) + + def test_searches(self): + tim = ContactPerson.objects.create(name='Tim') + Zoo.objects.create(name='Beekse Bergen', director=tim) + Zoo.objects.create(name='Dolfinarium') + response = self.client.get('/zoo/?search=Tim') + self.assertEqual(200, response.status_code) + data = json.loads(response.content)['data'] + self.assertEqual(1, len(data)) + self.assertEqual('Beekse Bergen', data[0]['name']) diff --git a/tests/test_pagination.py b/tests/test_pagination.py index 0694d6c2..ad0c93b0 100644 --- a/tests/test_pagination.py +++ b/tests/test_pagination.py @@ -287,7 +287,7 @@ def test_limit_offset_related_filtering(self): self.assertEqual(1, data['meta']['total_records']) self.assertEqual(1, len(data['data'])) - self.assertEqual(self.director.id, data['data'][0]['id']) + self.assertEqual(self.director.pk, data['data'][0]['name']) response = self.client.get('/contact_person/', data={'order_by': 'name', 'limit': 2, 'offset': 1, '.zoos.name': 'Wildlands Adventure Zoo Emmen'}) @@ -304,8 +304,8 @@ def test_limit_offset_related_filtering(self): self.assertEqual(3, data['meta']['total_records']) self.assertEqual(2, len(data['data'])) - self.assertEqual(self.director.id, data['data'][0]['id']) - self.assertEqual(self.janitor.id, data['data'][1]['id']) + self.assertEqual(self.director.pk, data['data'][0]['name']) + self.assertEqual(self.janitor.pk, data['data'][1]['name']) # Same set, but deeper filtering @@ -315,8 +315,8 @@ def test_limit_offset_related_filtering(self): self.assertEqual(3, data['meta']['total_records']) self.assertEqual(2, len(data['data'])) - self.assertEqual(self.director.id, data['data'][0]['id']) - self.assertEqual(self.janitor.id, data['data'][1]['id']) + self.assertEqual(self.director.pk, data['data'][0]['name']) + self.assertEqual(self.janitor.pk, data['data'][1]['name']) def test_limit_offset_filtering_on_annotations(self): @@ -326,7 +326,7 @@ def test_limit_offset_filtering_on_annotations(self): self.assertEqual(2, data['meta']['total_records']) self.assertEqual(1, len(data['data'])) - self.assertEqual(self.caretaker1.id, data['data'][0]['id']) + self.assertEqual(self.caretaker1.pk, data['data'][0]['id']) response = self.client.get('/caretaker/', data={'order_by': 'name', 'limit': 1, 'offset': 1, '.animal_count:gt': 1}) @@ -335,7 +335,7 @@ def test_limit_offset_filtering_on_annotations(self): self.assertEqual(2, data['meta']['total_records']) self.assertEqual(1, len(data['data'])) - self.assertEqual(self.caretaker2.id, data['data'][0]['id']) + self.assertEqual(self.caretaker2.pk, data['data'][0]['id']) response = self.client.get('/caretaker/', data={'order_by': 'name', 'limit': 1, 'offset': 0, '.animal_count:gt': 2}) @@ -344,7 +344,7 @@ def test_limit_offset_filtering_on_annotations(self): self.assertEqual(1, data['meta']['total_records']) self.assertEqual(1, len(data['data'])) - self.assertEqual(self.caretaker1.id, data['data'][0]['id']) + self.assertEqual(self.caretaker1.pk, data['data'][0]['id']) # This is a bit of a hack to ensure that people aren't using Q() diff --git a/tests/test_permission_view.py b/tests/test_permission_view.py index edff733a..9d23717c 100644 --- a/tests/test_permission_view.py +++ b/tests/test_permission_view.py @@ -858,6 +858,5 @@ def test_for_update_bug_not_occurs_on_put(self): res = self.client.put(f'/city_state/{city_state.id}/', data=jsondumps({ })) - print(res.json()) self.assertEqual(200, res.status_code) diff --git a/tests/test_router.py b/tests/test_router.py index b5a7fdd7..ce57252a 100644 --- a/tests/test_router.py +++ b/tests/test_router.py @@ -1,20 +1,21 @@ from django.test import TestCase from binder.exceptions import BinderNotFound -from binder.json import jsondumps +from binder.json import jsondumps, jsonloads from binder.models import BinderModel from binder.router import Router, Route, detail_route from binder.views import ModelView +from django.contrib.auth.models import User from django.urls.base import is_valid_path, clear_url_caches from django.urls import re_path, include from . import urls_module -# Two unique local models, to use for view registration -from .testapp.models import Country +from .testapp.models import ContactPerson, Country +# Two unique local models, to use for view registration class FooModel(BinderModel): class Meta(BinderModel.Meta): app_label = 'test' @@ -131,7 +132,7 @@ class BarView(ParentView): class TestFetchObj(TestCase): - + def test_get_obj_turns_pk_in_object(self): that = self @@ -183,3 +184,34 @@ def foo(self, request, obj): with self.assertRaises(BinderNotFound): Foo().foo(RequestMock(), 5) + +class TestWeirdPrimaryKeys(TestCase): + + WEIRD_PK = '"Such a... \\weird/annoying name!\'' + + def setUp(self): + u = User(username='testuser', is_active=True, is_superuser=True) + u.set_password('test') + u.save() + self.assertTrue(self.client.login(username='testuser', password='test')) + ContactPerson.objects.create(name=self.WEIRD_PK) + + def test_detail_route(self): + response = self.client.get(f'/contact_person/{self.WEIRD_PK}/upper-name/') + self.assertEqual(response.status_code, 200) + self.assertEqual('"SUCH A... \\WEIRD/ANNOYING NAME!\'', response.content.decode('utf-8')) + + def test_list_route(self): + response = self.client.get('/contact_person/counter/') + self.assertEqual(response.status_code, 200) + self.assertEqual('1', response.content.decode('utf-8')) + + def test_detail_endpoint(self): + response = self.client.get(f'/contact_person/{self.WEIRD_PK}/') + self.assertEqual(response.status_code, 200) + self.assertEqual(self.WEIRD_PK, jsonloads(response.content)['data']['name']) + + def test_list_endpoint(self): + response = self.client.get(f'/contact_person/') + self.assertEqual(response.status_code, 200) + self.assertEqual(self.WEIRD_PK, jsonloads(response.content)['data'][0]['name']) diff --git a/tests/test_validation_errors.py b/tests/test_validation_errors.py index 45e41ea6..7b511edd 100644 --- a/tests/test_validation_errors.py +++ b/tests/test_validation_errors.py @@ -257,13 +257,13 @@ def test_multiput_validate_unique_constraint_fail(self): 'with': {} } - response = self.client.put('/contact_person/', data=json.dumps(model_data), content_type='application/json') + response = self.client.put('/country/', data=json.dumps(model_data), content_type='application/json') self.assertEqual(response.status_code, 200) - response = self.client.put('/contact_person/', data=json.dumps(model_data), content_type='application/json') + response = self.client.put('/country/', data=json.dumps(model_data), content_type='application/json') returned_data = jsonloads(response.content) self.assertEqual(len(returned_data['errors']), 1) - self.assertEqual(len(returned_data['errors']['contact_person']), 1) - self.assertIn('name', returned_data['errors']['contact_person']['-1']) - self.assertEqual('unique', returned_data['errors']['contact_person']['-1']['name'][0]['code']) + self.assertEqual(len(returned_data['errors']['country']), 1) + self.assertIn('name', returned_data['errors']['country']['-1']) + self.assertEqual('unique', returned_data['errors']['country']['-1']['name'][0]['code']) diff --git a/tests/test_virtual_relation.py b/tests/test_virtual_relation.py index 89b6709f..52ff33d9 100644 --- a/tests/test_virtual_relation.py +++ b/tests/test_virtual_relation.py @@ -53,5 +53,3 @@ def test_can_include_relation_with_and_without_virtual_relation(self): response = self.client.get('/animal/', data={'.name': 'Peter', 'with': 'nickname.source,zoo.animals.nickname'}) - - print(response.content) diff --git a/tests/testapp/models/__init__.py b/tests/testapp/models/__init__.py index f944a1ca..9d322f70 100644 --- a/tests/testapp/models/__init__.py +++ b/tests/testapp/models/__init__.py @@ -6,6 +6,7 @@ from .donor import Donor from .caretaker import Caretaker from .contact_person import ContactPerson +from .contact_person_rating import ContactPersonRating from .costume import Costume from .gate import Gate from .nickname import Nickname, NullableNickname diff --git a/tests/testapp/models/animal.py b/tests/testapp/models/animal.py index fde79d7b..ad421b3d 100644 --- a/tests/testapp/models/animal.py +++ b/tests/testapp/models/animal.py @@ -18,6 +18,18 @@ class Animal(LoadedValuesMixin, BinderModel): deleted = models.BooleanField(default=False) # Softdelete birth_date = models.DateField(blank=True, null=True) + @classmethod + def format_instance_for_history(cls, pk): + if pk is not None: + try: + name = Animal.objects.get(pk=pk).name + # This looks silly, but is a good opportunity to test how we handle lists containing both names and integer IDs + if name != 'Moxy': + return name + except: + pass + return pk + def __str__(self): return 'animal %d: %s' % (self.pk, self.name) diff --git a/tests/testapp/models/contact_person.py b/tests/testapp/models/contact_person.py index baa1abcb..1df7f19e 100644 --- a/tests/testapp/models/contact_person.py +++ b/tests/testapp/models/contact_person.py @@ -3,19 +3,24 @@ from binder.models import BinderModel class ContactPerson(BinderModel): - name = models.CharField(unique=True, max_length=50) + name = models.CharField(primary_key=True, max_length=50) + nick_name = models.TextField(blank=True) created_at = models.DateTimeField(auto_now_add=True) updated_at = models.DateTimeField(auto_now=True) + first_zoo = models.ForeignKey('Zoo', on_delete=models.SET_NULL, blank=True, null=True, related_name='originals') + successor = models.ForeignKey('ContactPerson', on_delete=models.SET_NULL, blank=True, null=True, related_name='succeeds') + + pk_regex = '.*' @classmethod - def format_instance_for_history(cls, id: int): - try: - return ContactPerson.objects.get(id=id).name - except: - return 'deleted? ' + str(id) + def format_instance_for_history(cls, pk): + if isinstance(pk, str): + return pk.upper() + else: + return pk def __str__(self): - return 'contact_person %d: %s' % (self.pk, self.name) + return 'contact_person %s' % (self.pk) def clean(self): if self.name == 'very_special_forbidden_contact_person_name': @@ -23,3 +28,6 @@ def clean(self): code='invalid', message='Very special validation check that we need in `tests.M2MStoreErrorsTest`.' ) + + class Binder: + history = True diff --git a/tests/testapp/models/contact_person_rating.py b/tests/testapp/models/contact_person_rating.py new file mode 100644 index 00000000..1d805e9b --- /dev/null +++ b/tests/testapp/models/contact_person_rating.py @@ -0,0 +1,19 @@ +from django.db import models +from binder.models import BinderModel +import os + + +def choose_pk_field(): + if 'DJANGO_VERSION' in os.environ and tuple(map(int, os.environ['DJANGO_VERSION'].split('.'))) < (5, 2, 0): + return models.AutoField(primary_key=True) + else: + return models.CompositePrimaryKey('contact_person', 'date') + +class ContactPersonRating(BinderModel): + pk = choose_pk_field() + contact_person = models.ForeignKey('ContactPerson', on_delete=models.CASCADE, related_name='ratings') + date = models.DateField() + rating = models.IntegerField() + + class Binder: + history = True diff --git a/tests/testapp/models/zoo.py b/tests/testapp/models/zoo.py index 0aff9c2a..945ff71a 100644 --- a/tests/testapp/models/zoo.py +++ b/tests/testapp/models/zoo.py @@ -21,11 +21,23 @@ class ZooContactPerson(BinderModel): zoo = models.ForeignKey('Zoo', on_delete=models.CASCADE) contact_person = models.ForeignKey('ContactPerson', on_delete=models.CASCADE) +def choose_upper_name_field(): + if 'DJANGO_VERSION' in os.environ and tuple(map(int, os.environ['DJANGO_VERSION'].split('.'))) < (5, 0, 0): + return models.TextField(blank=True) + else: + return models.GeneratedField( + expression=models.functions.Upper('name'), + output_field=models.TextField(blank=True), + db_persist=True, + ) + + class Zoo(BinderModel): class Binder: history = True name = models.TextField() + upper_name = choose_upper_name_field() founding_date = models.DateField(null=True, blank=True) floor_plan = models.ImageField(upload_to='floor-plans', null=True, blank=True) # It is important that this m2m relationship is defined on this model, one of the tests depends on this fact diff --git a/tests/testapp/views/__init__.py b/tests/testapp/views/__init__.py index 31fb1049..41a09529 100644 --- a/tests/testapp/views/__init__.py +++ b/tests/testapp/views/__init__.py @@ -5,6 +5,7 @@ from .animal import AnimalView from .caretaker import CaretakerView from .contact_person import ContactPersonView +from .contact_person_rating import ContactPersonRatingView from .costume import CostumeView from .city import CityView from .country import CountryView diff --git a/tests/testapp/views/contact_person.py b/tests/testapp/views/contact_person.py index c1e90c5b..c4f136db 100644 --- a/tests/testapp/views/contact_person.py +++ b/tests/testapp/views/contact_person.py @@ -1,4 +1,6 @@ +from binder.router import detail_route, list_route from binder.views import ModelView +from django.http import HttpResponse from ..models import ContactPerson @@ -6,3 +8,11 @@ class ContactPersonView(ModelView): model = ContactPerson m2m_fields = ['zoos'] unwritable_fields = ['created_at', 'updated_at'] + + @detail_route(name='upper-name', methods=['GET'], fetch_obj=True) + def upper_name(self, request, obj: ContactPerson): + return HttpResponse(obj.name.upper()) + + @list_route(name='counter', methods=['GET']) + def counter(self, request): + return HttpResponse(str(ContactPerson.objects.all().count())) diff --git a/tests/testapp/views/contact_person_rating.py b/tests/testapp/views/contact_person_rating.py new file mode 100644 index 00000000..62021067 --- /dev/null +++ b/tests/testapp/views/contact_person_rating.py @@ -0,0 +1,6 @@ +from binder.permissions.views import PermissionView +from ..models import ContactPersonRating + + +class ContactPersonRatingView(PermissionView): + model = ContactPersonRating diff --git a/tests/testapp/views/zoo.py b/tests/testapp/views/zoo.py index 1f79917e..73546fee 100644 --- a/tests/testapp/views/zoo.py +++ b/tests/testapp/views/zoo.py @@ -8,6 +8,7 @@ # From the api docs class ZooView(PermissionView): + searches = ['name__icontains', 'director__pk__icontains'] m2m_fields = ['contacts', 'zoo_employees', 'most_popular_animals'] model = Zoo file_fields = ['floor_plan', 'django_picture', 'binder_picture', 'django_picture_not_null',