Opened 2 years ago

Last modified 2 years ago

#33945 assigned Bug

get_previous_in_order and get_next_in_order return incorrect data when objects is stored in non-default database

Reported by: François Granade Owned by: Aryan Amish
Component: Database layer (models, ORM) Version: 4.0
Severity: Normal Keywords:
Cc: Wael Ramadan Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Using the a simple model with an order_with_respect_to (slight variation of the models in https://github.com/django/django/blob/main/tests/multiple_database/models.py):

class Person(models.Model):
    name = models.CharField(max_length=100, unique=True)


class Pet(models.Model):
    name = models.CharField(max_length=100)
    owner = models.ForeignKey(Person, models.CASCADE)

    class Meta:
        order_with_respect_to = "owner"
       

then adding some object, and declaring an order for the pets:

 human = Person.objects.using("other").create(name="Human")
        dog = Pet.objects.using("other").create(name="Dog", owner=human)
        cat = Pet.objects.using("other").create(name="Cat", owner=human)
        duck = Pet.objects.using("other").create(name="Duck", owner=human)
        human.set_pet_order([duck.pk, dog.pk, cat.pk], "other")
       

Then simply trying to get the previous or next pet will fail:

>>> dog.get_next_in_order()  # would expect `cat`
models.Pet.DoesNotExist: Pet matching query does not exist.
>>> dog.get_previous_in_order()  # would expect `duck`
models.Pet.DoesNotExist: Pet matching query does not exist.       

Change History (11)

comment:1 by Wael Ramadan, 2 years ago

Cc: Wael Ramadan added
Owner: changed from nobody to Wael Ramadan
Status: newassigned

comment:2 by Carlton Gibson, 2 years ago

Triage Stage: UnreviewedAccepted

OK, I think this is valid. Model._get_next_or_previous_in_order() doesn't apply using() at all. src

Reproduces with the following diff:

diff --git a/tests/multiple_database/models.py b/tests/multiple_database/models.py
index 7de784e149..4c8bdffb96 100644
--- a/tests/multiple_database/models.py
+++ b/tests/multiple_database/models.py
@@ -79,3 +79,18 @@ class UserProfile(models.Model):
 
     class Meta:
         ordering = ("flavor",)
+
+
+class Question(models.Model):
+    text = models.CharField(max_length=200)
+
+
+class Answer(models.Model):
+    text = models.CharField(max_length=200)
+    question = models.ForeignKey(Question, models.CASCADE)
+
+    class Meta:
+        order_with_respect_to = "question"
+
+    def __str__(self):
+        return self.text
diff --git a/tests/multiple_database/tests.py b/tests/multiple_database/tests.py
index 7a9ff4da4e..8bf30c6fd9 100644
--- a/tests/multiple_database/tests.py
+++ b/tests/multiple_database/tests.py
@@ -12,7 +12,7 @@ from django.db.models import signals
 from django.db.utils import ConnectionRouter
 from django.test import SimpleTestCase, TestCase, override_settings
 
-from .models import Book, Person, Pet, Review, UserProfile
+from .models import Book, Person, Pet, Review, UserProfile, Question, Answer
 from .routers import AuthRouter, TestRouter, WriteRouter
 
 
@@ -2503,6 +2503,17 @@ class RouteForWriteTestCase(TestCase):
         self.assertEqual(e.model, Book)
         self.assertEqual(e.hints, {"instance": auth})
 
+    def test_order_with_respect_to(self):
+        q = Question.objects.using("other").create(text="The question")
+        a1 = Answer.objects.using("other").create(text="Answer 1", question=q)
+        a2 = Answer.objects.using("other").create(text="Answer 2", question=q)
+        a3 = Answer.objects.using("other").create(text="Answer 3", question=q)
+        q.set_answer_order([a1.pk, a2.pk, a3.pk], "other")
+
+        a2.refresh_from_db()
+        self.assertIs(a2.get_next_in_order(), a3)
+        self.assertIs(a2.get_previous_in_order(), a1)
+
 
 class NoRelationRouter:
     """Disallow all relations."""

Let's accept to investigate.

comment:3 by Wael Ramadan, 2 years ago

This seems to be working correctly in Django 4.1.2

Version 0, edited 2 years ago by Wael Ramadan (next)

comment:4 by Wael Ramadan, 2 years ago

Has patch: set
Resolution: fixed
Status: assignedclosed

comment:5 by Tim Graham, 2 years ago

Has patch: unset
Resolution: fixed
Status: closednew

If that's the case, you should identify the commit that fixed the issue. The test that Carlton wrote in comment 2 still fails on the main and stable/4.1.x branches, so I have my doubts this is fixed.

comment:6 by Wael Ramadan, 2 years ago

Resolution: worksforme
Status: newclosed

Using Django 4.1.1 this does actually fail, although when using Django 4.1.2 this works perfectly. Yet looking at the Release notes (https://docs.djangoproject.com/en/4.1/releases/4.1.2/) there seems to be no mention of this ticket.

This has to be investigated as to what fixed the issue!

Last edited 2 years ago by Wael Ramadan (previous) (diff)

comment:7 by Wael Ramadan, 2 years ago

Resolution: worksforme
Status: closednew

in reply to:  6 ; comment:8 by Mariusz Felisiak, 2 years ago

Replying to Wael Ramadan:

This has to be investigated as to what fixed the issue!

It fails for me on the main branch, stable/4.1.x branch, and with 4.1.2 version so it's definitely not fixed.

in reply to:  8 ; comment:9 by Wael Ramadan, 2 years ago

Replying to Mariusz Felisiak:

Replying to Wael Ramadan:

This has to be investigated as to what fixed the issue!

It fails for me on the main branch, stable/4.1.x branch, and with 4.1.2 version so it's definitely not fixed.

How is that possible that it fails on 4.1.2 what type of database are you using?

in reply to:  9 comment:10 by Wael Ramadan, 2 years ago

Replying to Wael Ramadan:

Replying to Mariusz Felisiak:

Replying to Wael Ramadan:

This has to be investigated as to what fixed the issue!

It fails for me on the main branch, stable/4.1.x branch, and with 4.1.2 version so it's definitely not fixed.

How is that possible that it fails on 4.1.2 what type of database are you using?

OK, oddly enough purged everything and started from scratch using 4.1.2 and it does fail.

comment:11 by Aryan Amish, 2 years ago

Owner: changed from Wael Ramadan to Aryan Amish
Status: newassigned
Note: See TracTickets for help on using tickets.
Back to Top