Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix when selecting with an MSFList #135

Merged
merged 7 commits into from
May 24, 2024
Merged

Fix when selecting with an MSFList #135

merged 7 commits into from
May 24, 2024

Conversation

karolyi
Copy link
Contributor

@karolyi karolyi commented Nov 28, 2022

No description provided.

@karolyi
Copy link
Contributor Author

karolyi commented Nov 28, 2022

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
Copy link
Contributor Author

karolyi commented Dec 5, 2022

@goinnn, please merge this.

@karolyi
Copy link
Contributor Author

karolyi commented Mar 19, 2023

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

@karolyi
Copy link
Contributor Author

karolyi commented Apr 26, 2023

bumperino

@karolyi
Copy link
Contributor Author

karolyi commented May 23, 2024

@blag ?

CHANGES.rst Outdated Show resolved Hide resolved
multiselectfield/utils.py Outdated Show resolved Hide resolved
multiselectfield/utils.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@karolyi
Copy link
Contributor Author

karolyi commented May 23, 2024

Should be good to go now.

@blag
Copy link
Collaborator

blag commented May 23, 2024

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
Copy link
Collaborator

blag commented May 23, 2024

When I change the filter to:

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

the test passes.

¯\_(ツ)_/¯

@karolyi
Copy link
Contributor Author

karolyi commented May 23, 2024

contains ?!

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

@blag
Copy link
Collaborator

blag commented May 23, 2024

Tests for this would be great, thank you.

@karolyi
Copy link
Contributor Author

karolyi commented May 23, 2024

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.

@blag
Copy link
Collaborator

blag commented May 23, 2024

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. 😄

@karolyi
Copy link
Contributor Author

karolyi commented May 24, 2024

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
Copy link
Contributor Author

karolyi commented May 24, 2024

The culprit was eadd573, fixed that.

Some jobs failed to complete, looking into that.

@karolyi
Copy link
Contributor Author

karolyi commented May 24, 2024

The build for 4.1 has failed.

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

@karolyi
Copy link
Contributor Author

karolyi commented May 24, 2024

@karolyi karolyi changed the title Fix when selecting with a MSFList Fix when selecting with an MSFList May 24, 2024
@blag blag merged commit 2ae2103 into goinnn:master May 24, 2024
4 checks passed
@blag
Copy link
Collaborator

blag commented May 24, 2024

Thanks for your contribution!

@karolyi
Copy link
Contributor Author

karolyi commented May 24, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants