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

Iterable classes have incorrect inheritence #725

Open
MooseV2 opened this issue Apr 10, 2024 · 1 comment
Open

Iterable classes have incorrect inheritence #725

MooseV2 opened this issue Apr 10, 2024 · 1 comment

Comments

@MooseV2
Copy link

MooseV2 commented Apr 10, 2024

By definition, a ValuesIterable should yield dicts for each row in the queryset:

Iterable returned by QuerySet.values() that yields a dict for each row.

Similary, ValuesListIterable yields tuples, and FlatValuesListIterable yields single values.

Therefore, you should be able to do something like this:

if issubclass(queryset_type, django.db.models.query.ValuesIterable):
    return [dict_to_json(row) for row in queryset]
if issubclass(queryset_type, django.db.models.query.ValuesListIterable):
    return [tuple_to_json(row) for row in queryset]
if issubclass(queryset_type, django.db.models.query.FlatValuesListIterable):
    return [value_to_json(value) for value in queryset]

This works correctly with base Django querysets and any managers that inherit from the default manager. However, with Django-modeltranslation, this fails because all of the iterable classes are incorrectly inheriting from ValuesIterable. If you are expecting to parse a dict with ValuesIterable but are given a str or list, things will break.

Minimal reproducible code:

from itertools import product
from modeltranslation.manager import (
    FallbackValuesIterable,
    FallbackValuesListIterable,
    FallbackFlatValuesListIterable,
)
from django.db.models.query import (
    ValuesIterable,
    ValuesListIterable,
    FlatValuesListIterable,
)

modeltrans_classes = [
    FallbackValuesIterable,
    FallbackValuesListIterable,
    FallbackFlatValuesListIterable,
]
django_classes = [ValuesIterable, ValuesListIterable, FlatValuesListIterable]


for a, b in product(modeltrans_classes + django_classes, django_classes):
    if a is b:
        continue
    subclass = issubclass(a, b)
    print(f"[{'✅' if subclass else '❌'}] {a.__name__} is a subclass of {b.__name__}")
[✅] FallbackValuesIterable is a subclass of ValuesIterable
[❌] FallbackValuesIterable is a subclass of ValuesListIterable
[❌] FallbackValuesIterable is a subclass of FlatValuesListIterable
[✅] FallbackValuesListIterable is a subclass of ValuesIterable
[❌] FallbackValuesListIterable is a subclass of ValuesListIterable
[❌] FallbackValuesListIterable is a subclass of FlatValuesListIterable
[✅] FallbackFlatValuesListIterable is a subclass of ValuesIterable
[❌] FallbackFlatValuesListIterable is a subclass of ValuesListIterable
[❌] FallbackFlatValuesListIterable is a subclass of FlatValuesListIterable
[❌] ValuesIterable is a subclass of ValuesListIterable
[❌] ValuesIterable is a subclass of FlatValuesListIterable
[❌] ValuesListIterable is a subclass of ValuesIterable
[❌] ValuesListIterable is a subclass of FlatValuesListIterable
[❌] FlatValuesListIterable is a subclass of ValuesIterable
[❌] FlatValuesListIterable is a subclass of ValuesListIterable

As you can see, the above Django-modeltrans iterable classes should inherit from their respective Django class (ie FallbackFlatValuesList -> FlatValuesList), but they are not.

@last-partizan
Copy link
Collaborator

Hello, yeah it would be nice to fix it.

Please, create PR with changes and new test-cases and we can merge it after review.

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

No branches or pull requests

2 participants