nshafer / django-hashid-field

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

implement optional prefix to the field #49

Closed toudi closed 3 years ago

toudi commented 4 years ago

I stumbled upon the issue #31 however instead of adding a custom salt I thought that it'd be nice to have a custom prefix. The reason would be that, if somebody would pass an invalid ID as an URL parameter, I'd be able to detect that even prior to hitting the database. As an additional benefit, if you'd have two models, encoding a primary key with a value 1 and on two separate models (with two prefixes) would yield two different hashes.

this change is backwards compatible - the prefix is optional.

not sure if I made the correct unit test scenarios :)

cheers

nshafer commented 4 years ago

Thanks for the patch, I'll take a look as soon as I can. I have been absolutely slammed with work, and I am also moving this month, so it might be a little bit. I'll try to take a look ASAP though!

toudi commented 4 years ago

no problem. the DRF-framework related stuff actually took me most of the time as I didn't quite understood why the serializer / mixin was creating another instance of Hashid class under the hood. Maybe if it would be possible to remove that then a custom salt could be implemented as well?. I wouldn't mind contributing some more, rewriting some of the tests and what not.

take some rest and stay safe. cheers

bjmc commented 3 years ago

This looks like a good PR @toudi!

I'd be very interested in this feature because we'd like to prefix our IDs with a tag URI scheme to make globally-unique identifiers.

If I could make one suggestion / extra feature request, would it be possible to support optionally having prefix be a callable that gets executed in a context where it can reference the model the field is attached to?

We have a common base class for all our models, and it would be fantastic to only have to override the ID once, on the base class, rather than on every individual model.

As an example of what I'm talking about, I'm imagining something like this:

def generate_tag(model):
    return 'tag:example.com,2021:' + model._meta.verbose_name

class CustomBaseClass(models.Model):
    id = HashidAutoField(primary_key=True, prefix=generate_tag)

    class Meta:
        abstract = True

class OtherModel(CustomBaseClass):
    somefield = models.TextField()

I don't know how difficult this would be to support, but I wonder if the __init__() could check whether prefix is a string or a callable, and act accordingly. (Or we could support a separate keyword, dynamic_prefix= or something, but then the interface starts to get wordy)

nshafer commented 3 years ago

@toudi Thanks for the PR. In review, I have found that this encodes the prefix as an additional integer in every hashid created. So that, for example, Hashids(salt="abcd", min_length=7).encode(2147483647, 1) == "QBQaa9OFD".

  1. This means that every hashids created encodes this extra integer. Not a big deal with the default min_length of 7 for integers up to a point, but after that, they start to grow in size up to 9 digits long for the maximum for an IntegerField. Minor issue, but worth noting.
  2. In my testing (timeit.timeit('h1.encode(1, 2147483647)', setup='from hashids import Hashids; h1 = Hashids(salt="asdf")', number=1_000_000), this resulted in the Hashids library taking almost twice as long to encode numbers. I don't know what it's doing internally, but each extra number you pass to encode() seems to add a linear amount of time to the algorithm (3 numbers takes 3 times longer).
  3. The named parameter 'prefix' is confusing, as in other PRs it's been used to mean a prefix of the generated hashid, like "user_h6jIl92". This is illustrated by @bjmc's recent confusion. I also thought that was what this PR was about at first.
  4. The desired behavior can already be attained without these drawbacks by setting a custom field salt in the existing library. e.g. id = HashIdField(salt=settings.HASHID_FIELD_SALT + "_user")

Due to those issues, I won't be merging this PR in at this time. But please let me know if there are any misconceptions or inaccuracies in this assessment.

Thanks! Nate

Arti3DPlayer commented 3 months ago

@nshafer Hello, what about callable prefix as @bjmc mentioned here .I faced the same issue where I need to have dynamic prefix based on model type field

bjmc commented 3 months ago

@Arti3DPlayer This library has been retired, so I doubt the callable prefix feature will be returning.

See here for details