django-multiselectfield icon indicating copy to clipboard operation
django-multiselectfield copied to clipboard

Fix when selecting with a MSFList

Open karolyi opened this issue 3 years ago • 4 comments

karolyi avatar Nov 28 '22 20:11 karolyi

Django==4.1.3, this is a serious error:

When selecting a model that has a multiselectfield, with a result from a former multiselectfield (MSFList), the DB conversion will fail, resulting in false negatives.

This patch fixes it. I use it in one of my projects. Until this PR gets merged, my work is available at https://gitea.ksol.io/karolyi/django-multiselectfield/commits/branch/master

Elaborating on how to reproduce:


>>> obj = SomeModel.objects.first()
>>> type(obj.some_multiselect_field)
multiselectfield.db.fields.MSFList
>>> SomeModel.objects.filter(pk=obj.pk, some_multiselect_field=obj.some_multiselect_field).count()
0

The result should be 1 instead of 0, obviously. Tracking the bug down, the DB conversion fails. It is very circumstantial to go into the ORM code to see what happens, but after hours of investigation, this PR fixes the bug.

karolyi avatar Nov 28 '22 20:11 karolyi

@goinnn, please merge this.

karolyi avatar Dec 05 '22 16:12 karolyi

guys, can we pick up the pace here? @tomasgarzon ?

karolyi avatar Mar 19 '23 12:03 karolyi

bumperino

karolyi avatar Apr 26 '23 20:04 karolyi

@blag ?

karolyi avatar May 23 '24 16:05 karolyi

Should be good to go now.

karolyi avatar May 23 '24 17:05 karolyi

I pushed a commit with the test code you provided:

def test_filter(self):
    self.assertEqual(Book.objects.filter(tags__contains='sex').count(), 1)
    self.assertEqual(Book.objects.filter(tags__contains='boring').count(), 0)

    book = Book.objects.first()
    self.assertQuerySetEqual(
        Book.objects.filter(pk=book.pk),
        Book.objects.filter(pk=book.pk, published_in=book.published_in),
    )

but it's still failing:

$ (cd example; python manage.py test app.test_msf.MultiSelectTestCase.test_filter)
Found 1 test(s).
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
F
======================================================================
FAIL: test_filter (app.test_msf.MultiSelectTestCase.test_filter)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "example/app/test_msf.py", line 91, in test_filter
    self.assertQuerySetEqual(
  File ".../django/test/testcases.py", line 1169, in assertQuerySetEqual
    return self.assertEqual(list(items), values, msg=msg)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: Lists differ: [<Book: My book 1>] != []

First list contains 1 additional elements.
First extra element 0:
<Book: My book 1>

- [<Book: My book 1>]
+ []

----------------------------------------------------------------------
Ran 1 test in 0.012s

FAILED (failures=1)
Destroying test database for alias 'default'...

blag avatar May 23 '24 22:05 blag

When I change the filter to:

Book.objects.filter(pk=book.pk, published_in__contains=book.published_in)

the test passes.

¯\_(ツ)_/¯

blag avatar May 23 '24 22:05 blag

contains ?!

I've got a test for this change in one of my projects that passes. Maybe I should port that over here?

karolyi avatar May 23 '24 22:05 karolyi

Tests for this would be great, thank you.

blag avatar May 23 '24 22:05 blag

I'll see what I can do - tomorrow.

Just make sure that when the test passes, you'll merge this in a timely fashion :) I've been waiting for this for almost 2 years now.

karolyi avatar May 23 '24 22:05 karolyi

I make no promises - I do this all in my own free time and I have plenty of other things going on, as we all do.

But tests for anything and everything are much appreciated. 😄

blag avatar May 23 '24 22:05 blag

On it now, it seems the changes introduced since my first patch broke the patch itself.

Need to figure out what's up here.

karolyi avatar May 24 '24 13:05 karolyi

The culprit was https://github.com/goinnn/django-multiselectfield/commit/eadd5731b76918e46d45460f2efa655b7e62ae31, fixed that.

Some jobs failed to complete, looking into that.

karolyi avatar May 24 '24 13:05 karolyi

The build for 4.1 has failed.

Support for 4.1 ended at April 5, 2023, removing from the tests.

karolyi avatar May 24 '24 13:05 karolyi

Good to go now: https://github.com/karolyi/django-multiselectfield/actions/runs/9224962236

karolyi avatar May 24 '24 13:05 karolyi

Thanks for your contribution!

blag avatar May 24 '24 19:05 blag

Thanks. Good to see this module is taken care of by someone who still cares. :)

karolyi avatar May 24 '24 23:05 karolyi