Improve support for non-integer primary keys

.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

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]

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

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,
 			))

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 = ['<html>', '<head>', '<style type="text/css">td {padding: 0px 20px;} th {padding: 0px 20px;}</style>', '</head>', '<body>']
 	for cs in changesets:
 		username = cs.user.username if cs.user else None
-		body.append('<h3>Changeset {} by {}: {} on {} {{{}}}'.format(cs.id, cs.source, username, cs.date.strftime('%Y-%m-%d %H:%M:%S'), cs.uuid))
+		body.append('<h3>Changeset {} by {}: {} on {} {{{}}}'.format(cs.pk, cs.source, username, cs.date.strftime('%Y-%m-%d %H:%M:%S'), cs.uuid))
 		body.append('<br><br>')
 		body.append('<table>')
 		body.append('<tr><th>model</th><th>object id</th><th>field</th><th><diff</th><th>before</th><th>after</th></tr>')
-		for c in cs.changes.order_by('model', 'oid', 'field'):
+		for c in cs.changes.order_by('model', 'oid', 'oid_string', 'field'):
 			body.append('<tr><td>{}</td><td>{}</td><td>{}</td><td>{}</td><td>{}</td><td>{}</td></tr>'.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('</table>')
 		body.append('<br><br>')
 	body.append('</body>')

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),
+        ),
+    ]

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,41 +495,41 @@ 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.
 
 		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)
 
 
 

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()
 

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()