Mixed PK types (UUID / Integer Incremental ID) Django queryset Bug

[stygmate]

@jerch

Hi,

edit: This issue now have two thumbs down because without thinking I made the mistake of tagging all contributors (please excuse me if I bothered you). Despite this an improvement can be made if it is taken into consideration. unfortunately I have absolutely no time to produce a pull request with unit tests.

I use models with id and uuid in a Django App. I discovered that when filtering on related field, Django may cast id type... id :10 can become 00000000-0000-0000-0000-00000000000a

I made a patch for this (resolver.py file). All tests pass with it. Can you make a commit and add it to code base ? (don't have the time to make a pull request)

@@ -392,7 +392,12 @@
             for fieldname in update_fields:
                 if fieldname in modeldata:
                     updates.add(fieldname)
-        subquery = '__in' if isinstance(instance, QuerySet) else ''
+        if isinstance(instance, QuerySet):
+            subquery = '__in'
+            instance_for_filter = instance
+        else:
+            subquery = '__pk'
+            instance_for_filter = instance.pk
         model_updates = OrderedDict()
         for update in updates:
             # first aggregate fields and paths to cover
@@ -406,7 +411,7 @@
             fields, paths = data
             queryset = model.objects.none()
             for path in paths:
-                queryset |= model.objects.filter(**{path+subquery: instance})
+                queryset |= model.objects.filter(**{path+subquery: instance_for_filter})
             if pk_list:
                 # need pks for post_delete since the real queryset will be empty
                 # after deleting the instance in question

[simkimsia]

I am not an active maintainer and I just checked my commit over the past 6 months have only been to update the README example once.

For you to spam all the authors because you have no time is incredibly selfish.

We also lead busy lives. Is our time worth less than yours?

There might be a legitimate request in your issue but the way this is delivered leaves a bad taste.

And I feel sorry for the actual maintainer @jerch

[stygmate]

@simkimsia hi ! You are right. Excuse me ! In a hurry of too much things. I didn’t think before posting.

[simkimsia]

@stygmate

1. don't just apologize. actually do something. You have 3 choices:
   1. delete this issue, maintain your own fork, and let it go
   2. modify this issue so that the other authors are no longer spammed and wait
   3. delete this issue then do a proper pull request with a unit test to cover your use case and wait to see if the PR is merged or not
2. Please do not @ me to reply. I no longer want to know what you choose.

[jerch]

Well, lets not heat up things. @stygmate If you find time to do a PR, that would be awesome. If not, it is no biggy either, but then I need a bit more context to grasp things:

• When exactly happens the "cast" and by what precondition?
• Is this only filter related?
• Can this be pointed back to django code, where the cast happens?
• Other things affected as well? (like complicated pk construction, whatsoever)

As you see, I've never experienced that behavior myself, thus have to understand first whats going on.

[stygmate]

@jerch Hi ! If i remember correctly my debug session. when 'model' variable contain a class of a model using UUIDs and 'instance' is a single instance of a related model using incremental integer IDs: the integer ID is casted to UUID (id type of parent object). seems to be more a Django things but passing directly 'instance.pk' instead of 'instance' to filter function resolved the problem. I stopped debugging when i've seen that it can be a Django's bug (the SQL and Query code of Django is very difficult to follow because there's a lot of metacode in it) (I launched all your unit tests and this patch don't seem to alter any behaviour.)

[jerch]

@stygmate Well, what you describe basically means, that instance and instance.pk are not treated as object identical anymore at the query interface. This identity was guaranteed in all django versions so far (in fact the query methods refer to the identity via the pk). If they changed that - thats huge and would impose much bigger issues. So questions are:

• What django version you are on?
• Minimal model repro possible?

[stygmate]

@jerch I have take one hour to make a test project: https://github.com/stygmate/dcftest all is in. ps: if you're not familiar with Poetry, check the pyproject.toml file. if you want precise versions of components used, check the poetry.lock file.

[jerch]

Thx for the repro project. This is what I've found - when adding a ModelB instance, the query interface creates this SQL query to get the records for updating the computed fields:

SELECT DISTINCT 
    "dcfapp_modela"."id", "dcfapp_modela"."name", "dcfapp_modela"."number_of_model_a_objects"
FROM
    "dcfapp_modela"
INNER JOIN
    "dcfapp_modelb"
ON
    ("dcfapp_modela"."id" = "dcfapp_modelb"."model_a_id")
WHERE
    "dcfapp_modelb"."id" = '00000000-0000-0000-0000-000000000001'::uuid
LIMIT
  21;
args=(UUID('00000000-0000-0000-0000-000000000001'),)

Now look at this in detail - it is all wrong. For some reason the fact, that ModelA.id is a UUID field, leaked into the WHERE clause on ModelB.id, which ofc cannot work, as it is just a simple integer id. This is a bug in Django 2.2, it does not happen in Django 3.2 anymore. Not going to add your fix, as it is clearly wrong behavior in the ORM, and I cannot guarantee anything, if those basic assumptions are not met anymore.

Suggestion: Either keep your local fix with Django 2.2, or upgrade to Django 3.2. If you care enough, you also might want to send a bug report to the django repo (not sure if they would backport a fix for this, 2.2 is not in mainstream support phase anymore).

[stygmate]

@jerch thanks for the test. Will do a Quick report to django team.

[jerch]

If you need more context - this is the actual call, that goes bonkers:

>>> ModelA.objects.filter(modelb=some_b)

while this works:

>>> ModelA.objects.filter(modelb__pk=some_b.pk)

It is the filtering on reverse fk relations with plain instance objects, where the pk type gets messed up.

[stygmate]

@jerch i've received informations from Django team: It was patched in a beta of Django 3. It will never be patched in 2.2 . https://code.djangoproject.com/ticket/33010 https://code.djangoproject.com/ticket/30477 So I have to switch to Django 3 . (big work... i use a lot of complex lib that are incompatible)