deschler/django-modeltranslation

Iterable classes have incorrect inheritence

MooseV2 opened this issue · 1 comments

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.

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.