nshafer / django-hashid-field

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

Using invalid hashid on related field throws ValueError even with HASHID_FIELD_LOOKUP_EXCEPTION=False #48

Closed rvdrijst closed 3 years ago

rvdrijst commented 4 years ago

This is related to the closed issue #17 that deals only with direct lookups.

I have the default HASHID_FIELD_LOOKUP_EXCEPTION = False, which works for as expected if I look up an object by primary key directly.

However, if I try to filter a queryset by hashid to a related field (e.g. a foreign key) the queryset throws a ValueError if the hashid is not valid. I understand why, but for my purposes it shouldn't really matter whether I pass in a valid but non-existent key, or an invalid (and equally non-existent) key.

Example:

class Author(models.Model):
    id = HashidAutoField(primary_key=True)
    name = models.CharField(max_length=40)

class Book(models.Model):
    reference_id = HashidField()
    author = models.ForeignKey('Author')

# now, if I try to do the following lookups:

# valid ID: returns the books for given author
Book.objects.filter(author_id='OwLxW8D')
# valid ID but no author with given ID, returns empty queryset
Book.objects.filter(author_id='r8636LO')
# invalid ID throws ValueError
Book.objects.filter(author_id='OwLxW8X')

Because it's quite tricky to catch invalid hashids before they are passed into the queryset (a simple regex is clearly not enough and in my case the salt is different for each model) this leads to having to handle two cases: empty results and ValueErrors. Especially inside Django Rest Frameworks internals (like filters) this is quite cumbersome, and actually I don't care whether the hashid is invalid or just doesn't exist, as long as it matches basic sanity checks with a regex.

So, my solution (a bit hacky so I'm sure there is a cleaner solution, but this seems to work at least on Postgres):

class AutoSaltHashidFieldMixin(HashidFieldMixin):
    # see my comment in #31 for automatic salt based on model, that's in this same mixin
    def get_prep_value(self, value):
        try:
            return super().get_prep_value(value)
        except ValueError:
            # don't expose errors, simply let it go through to empty results (since -1 never exists)
            return -1

# use these in your models    
class HashidAutoField(AutoSaltHashidFieldMixin, models.AutoField):
    description = "A Hashids obscured AutoField"

class HashidBigAutoField(AutoSaltHashidFieldMixin, models.BigAutoField):
    description = "A Hashids obscured BigAutoField"

Also this makes sure that invalid hashids don't trigger a different error than non-existent hashids, which might expose some information on how the hashids are built up.

nshafer commented 3 years ago

This is unfortunately not possible to fix without causing other issues, and cutting against the grain of Django's intent. The logic for this is buried deep in RelatedLookupMixin which is used by the ForeignKey field. It calls get_prep_lookup() on the field to validate the value. It has this comment:

# We need to run the related field's get_prep_value(). Consider case
# ForeignKey to IntegerField given value 'abc'. The ForeignKey itself
# doesn't have validation for non-integers, so we must run validation
# using the target field.

Your solution is a possible one to workaround this particular problem, but like I said, I'm worried about it being too "magic" and/or causing issues elsewhere. The current behavior mimics that of other built-in fields. If you try to do a filter on a ForeignKey field that points to an AutoField or IntegerField with an invalid int, such as a string, it will throw a ValueError as well.