Improve support for non-integer primary keys by knokko · Pull Request #279 · CodeYellowBV/django-binder

knokko · 2026-05-07T15:55:35Z

This PR improves support for non-integer primary keys

Currently, binder sort of allows it, but:

You cannot invoke detail endpoints
You cannot invoke detail routes
The name of the primary key field is always sent as id over the network,
regardless of what the name of the primary key field is.
Any relations will fail miserably because binder accepts only integer IDs.

This PR fixes all of these shortcomings.
Furthermore, it fixes some history changeset inconsistencies.

knokko · 2026-05-08T14:26:26Z

Note that filtering on foreign key is now handled by this code:

# 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__

Prior to this PR, it was hardcoded as part of IntegerField.

BBooijLiewes

Lets also test this in a few internal project pipelines before merging.

BBooijLiewes · 2026-06-04T08:58:59Z

 			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})
+		data.append({'date': cs.date, 'uuid': cs.uuid, 'id': cs.pk, 'source': cs.source, 'user': cs.user_id, 'changes': changes})


Check serialization compisite PK, maybe add test case?

I just added a fix + test case for string (non-integer) primary keys.

I also tried to add a test case for composite primary keys, but this is currently impossible:

Binder currently doesn't support detail endpoints for models with composite pk's, so I can't use /api/model_with_composite_pk/pk_tuple/history/.

Django doesn't support forward relations to models with composite pk's

Our history endpoint ignores backward relations

BBooijLiewes · 2026-06-04T09:00:17Z



 class BinderModel(models.Model, metaclass=BinderModelBase):
+	pk_regex = '[0-9]+'


Now that PK's can be composite does this still hold? Or should we add check in places?

Currently, this PR does not support detail endpoints for models with composite primary keys. For instance, if I create an OrderAddition model with a composite primary key (order_id, addition_number), there is currently no way to address it. What would it even look like? /api/order_addition/(1234,5)/?

Also note that this pk_regex = '[0-9]+' is the default regex, which models with more complicated primary keys can override.

BBooijLiewes · 2026-06-04T09:02:06Z

+						if isinstance(entry, list) and len(entry) == 1:
+							entry = entry[0]


Add comment what this does

BBooijLiewes · 2026-06-04T09:06:06Z

+    else:
+        return models.CompositePrimaryKey('contact_person', 'date')


Can we also build a check in binder itself if you try to use a composite key on a binder model with binder < 5 we give a warning on migration?

I assume you mean django < 5 rather than binder < 5.

Django < 5 simply doesn't support CompositePrimaryKey, so it will already give an error.

If you use a CompositePrimaryKey on django 5 but with an old binder version, django won't give any errors, but binder will fail miserably. I could add a helpful error message, but older versions of binder wouldn't have that helpful error message, which makes it useless.

BBooijLiewes · 2026-06-04T09:07:23Z

+				# I would like to remove the next line completely, but that might break downstream projects
+				data['id'] = data[self.model._meta.pk.name]


No comment in first person

better now?

BBooijLiewes · 2026-06-04T09:08:00Z

 		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)) # TODO Check this?


Check ToDo @knokko

better now?

BBooijLiewes · 2026-06-04T09:16:18Z



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


think modern djanbo uses a big int field by default.

Possibly, but note that BigIntegerField extends IntegerField: https://github.com/django/django/blob/4bbc27c8686f10f9556cef02dbfa9f5157fbcf56/django/db/models/fields/__init__.py#L2241

BBooijLiewes · 2026-06-04T09:16:34Z

 					continue

-				if isinstance(data[field.name], int):
+				if isinstance(data[field.name], int): # TODO Check right type?


BBooijLiewes · 2026-06-04T09:16:40Z

 				elif isinstance(data[field.name], list):
 					for i, mid in enumerate(data[field.name]):
-						if isinstance(mid, int):
+						if isinstance(mid, int): # TODO Check right type?


BBooijLiewes · 2026-06-04T09:17:02Z

 						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): # TODO Check right type


This commit dramatically improves the way binder handles non-integer primary keys. Currently, binder sort of allows it, but: - You cannot invoke detail endpoints - You cannot invoke detail routes - You cannot invoke the history route - You cannot create, modify, or delete such objects - The name of the primary key field is always sent as `id` over the network, regardless of what the name of the primary key field is. - Any relations will fail miserably because binder accepts only integer IDs. This commit fixes all of these shortcomings.

Before updating django-hijack, the /user/endmasquerade/ endpoint would yield 200 when it succeeds, and yield 403 when it fails because the user is not masquerated. After updating django-hijack, it would always yield 200. This commit restores the old behavior of returning 403 for non-masquerated users.

This commit improves our support for `models.CompositePrimaryKey` by properly handling models with `CompositePrimaryKey`s in the `with`s of other objects. (Prior to this commit, it would fail miserably.)

Add instructions to reset test database

d8ab763

knokko force-pushed the non-integer-ids branch from 3871f4e to cce13d7 Compare May 8, 2026 10:26

BBooijLiewes self-requested a review May 8, 2026 13:13

BBooijLiewes assigned knokko May 8, 2026

knokko force-pushed the non-integer-ids branch 10 times, most recently from de3d364 to ce5801c Compare May 28, 2026 15:29

knokko changed the title ~~wip: non-integer primary keys~~ Improve support for non-integer primary keys Jun 4, 2026

knokko marked this pull request as ready for review June 4, 2026 07:25

BBooijLiewes requested changes Jun 4, 2026

View reviewed changes

knokko force-pushed the non-integer-ids branch 5 times, most recently from ac444e6 to c639c89 Compare June 4, 2026 13:23

knokko added 4 commits June 4, 2026 16:01

Improve support for composite primary keys

4f0cb88

This commit improves our support for `models.CompositePrimaryKey` by properly handling models with `CompositePrimaryKey`s in the `with`s of other objects. (Prior to this commit, it would fail miserably.)

Add support for filtering on GeneratedField

969f990

knokko force-pushed the non-integer-ids branch from e813ec7 to 969f990 Compare June 4, 2026 14:03



		class BinderModel(models.Model, metaclass=BinderModelBase):
		pk_regex = '[0-9]+'

		if isinstance(entry, list) and len(entry) == 1:
		entry = entry[0]

		else:
		return models.CompositePrimaryKey('contact_person', 'date')

		# I would like to remove the next line completely, but that might break downstream projects
		data['id'] = data[self.model._meta.pk.name]



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

Conversation

knokko commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knokko commented May 8, 2026

Uh oh!

BBooijLiewes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

knokko commented May 7, 2026 •

edited

Loading