nshafer / django-hashid-field

Django Model Field that uses Hashids to obscure the value
MIT License
370 stars 40 forks source link

`enable_hashid_object=False` breaks Django admin? #69

Closed ckcollab closed 2 years ago

ckcollab commented 2 years ago
order_id = HashidField(enable_hashid_object=False, null=True, blank=True)

Unless I set enable_hashid_object = True then I get the following error in Django admin attempting to save a model:

TypeError
'<' not supported between instances of 'str' and 'int'
image
nshafer commented 2 years ago

It looks like you have a MinValueValidator on that field, which doesn't work with a string, so either enable the hashid_object or remove the validator.

ckcollab commented 2 years ago

That's the confusing thing and why I shared the field definition: I didn't set this validator or anything?! Changing enable_hashid_object removes the error

Maybe something special django admin does?

nshafer commented 2 years ago

What version of Django? I just tested with 3.2 and had no problems

ckcollab commented 2 years ago

Django==3.2.10 and I just tried on the latest Django==3.2.12 as well, same problemo. Using django-hashid-field==3.3.3

I went to a completely different model, added this, and tried to save it in Django admin:

special_id = HashidField(enable_hashid_object=False, null=True, blank=True, default=1)

..same error! It could definitely be something silly i am doing..

I'm down to hop in a quick Google Meet or something, I can share my environment and maybe be more helpful?

EDIT: To remove Django Admin from the equation, here's a quick repro:

>>> u = User.objects.first()
>>> u.full_clean()
Traceback (most recent call last):
  File "<input>", line 1, in <module>
    u.full_clean()
  File "/usr/local/lib/python3.10/site-packages/django/db/models/base.py", line 1229, in full_clean
    self.clean_fields(exclude=exclude)
  File "/usr/local/lib/python3.10/site-packages/django/db/models/base.py", line 1271, in clean_fields
    setattr(self, f.attname, f.clean(raw_value, self))
  File "/usr/local/lib/python3.10/site-packages/django/db/models/fields/__init__.py", line 671, in clean
    self.run_validators(value)
  File "/usr/local/lib/python3.10/site-packages/django/db/models/fields/__init__.py", line 623, in run_validators
    v(value)
  File "/usr/local/lib/python3.10/site-packages/django/core/validators.py", line 358, in __call__
    if self.compare(cleaned, limit_value):
  File "/usr/local/lib/python3.10/site-packages/django/core/validators.py", line 392, in compare
    return a < b
TypeError: '<' not supported between instances of 'str' and 'int'
nshafer commented 2 years ago

My guess is you have somewhere in your code-base, or some library that is helpfully adding it. If you do something like this, what do you get?

In [1]: key = Book._meta.get_field("key")

In [2]: key
Out[2]: <hashid_field.field.HashidField: key>

In [3]: key.enable_hashid_object
Out[3]: False

In [4]: key.validators
Out[4]: []
ckcollab commented 2 years ago
>>> key = User._meta.get_field("special_id")
>>> key
<hashid_field.field.HashidField: special_id>
>>> key.enable_hashid_object
False
>>> key.validators
[<django.core.validators.MinValueValidator object at 0x7f7e2b8fa050>, <django.core.validators.MaxValueValidator object at 0x7f7e2b8fa0b0>]

you're right...

Something must be setting "default validators" somehow... ok..! probably not a problem with your project.

nshafer commented 2 years ago

What does your model inherit from? models.Model that's imported from django.db import models?

ckcollab commented 2 years ago

In one case:

class User(AbstractBaseUser, PermissionsMixin):
    ...
    special_id = HashidField(enable_hashid_object=False, null=True, blank=True, default=1)

In another case:

class Booking(SoftDeletableModel):
    ...
    order_id = HashidField(enable_hashid_object=False, null=True, blank=True)

# And here's soft deletable model:
class SoftDeletableModel(models.Model):
    deleted = models.BooleanField(default=False)

    objects = SoftDeleteModelManager()
    all_objects = models.Manager()

    class Meta:
        abstract = True

EDIT:

Looking at HashidField itself, the inherited IntegerField may be producing these validators for me!? not sure why it wouldn't for you..

class IntegerField(Field):

    ...

    @cached_property
    def validators(self):
        # These validators can't be added at field initialization time since
        # they're based on values retrieved from `connection`.
        validators_ = super().validators
        internal_type = self.get_internal_type()
        min_value, max_value = connection.ops.integer_field_range(internal_type)
        if min_value is not None and not any(
            (
                isinstance(validator, validators.MinValueValidator) and (
                    validator.limit_value()
                    if callable(validator.limit_value)
                    else validator.limit_value
                ) >= min_value
            ) for validator in validators_
        ):
            validators_.append(validators.MinValueValidator(min_value))
        if max_value is not None and not any(
            (
                isinstance(validator, validators.MaxValueValidator) and (
                    validator.limit_value()
                    if callable(validator.limit_value)
                    else validator.limit_value
                ) <= max_value
            ) for validator in validators_
        ):
            validators_.append(validators.MaxValueValidator(max_value))
        return validators_
nshafer commented 2 years ago

Yeah none of that is a clue of where it's coming from. I'm going to hazard a guess that something is doing a isinstance(field, models.IntegerField) check, which is true, but obviously doesn't work. A case where duck-typing is better in python/django. Why do you want to disable the Hashid object? Is leaving it on an option?

nshafer commented 2 years ago

Oh... what database?

nshafer commented 2 years ago

min_value, max_value = connection.ops.integer_field_range(internal_type) is getting something from your adapter that I've never seen, but I'm 100% postgres in my work.

ckcollab commented 2 years ago

Well this is a fun one! Yeah, Postgres here, too..

'ENGINE': 'django.db.backends.postgresql_psycopg2',
psycopg2-binary==2.9.3
nshafer commented 2 years ago

Ha! I lied! Both my tests and sandbox are using sqlite. Fix incoming.

nshafer commented 2 years ago

Version 3.3.4 is on pypi with a fix for this. Sorry about that! But thank you for working with me to a solution.

Nate

ckcollab commented 2 years ago

No problemo, was fun! Confirmed fixed.

Have a good one!