nshafer / django-hashid-field

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

Django: Instantiated objects in TestCase.setUpTestData() that use HashidAutoField get a different pk in the test methods #63

Closed dmasson closed 3 years ago

dmasson commented 3 years ago

Asuming Foo has a HashidAutoField if used in a TestCase, and following the standard django test pattern:

from django.db import models
from hashid_field import HashidAutoField

class Foo(models.Model):
    id = HashidAutoField(
        primary_key=True, alphabet="0123456789abcdefghijklmnopqrstuvwxyz")
from django.test import TestCase

class MyTests(TestCase):
    @classmethod
    def setUpTestData(cls):
        # Set up data for the whole TestCase
        cls.foo = Foo.objects.create(bar="Test")
        print (cls.foo.id) # prints a hash like 'jmvnjz7' which is pk

    def test1(self):
        # Some test using self.foo
        print (self.foo.id) # prints a DIFFERENT hash like '4rqramj'

the versions I'm using:

django-hashid-field==3.2.1
Django==3.2.5
francis-de-ladu commented 3 years ago

Same, very annoying issue.

django-hashid-field==3.2.1 Django==3.2.3

dmasson commented 3 years ago

@francis-de-ladu how do you workaround ?

francis-de-ladu commented 3 years ago

I moved the setUp code from class method setUpTestData() to method setUp(), but it makes running tests a lot slower.

nshafer commented 3 years ago

I am unable to duplicate this issue with either SQLite nor Postgres. What database are you using? I ask because it sounds like whatever you're using doesn't support transactions, or sequences are not included in the transaction. What happens if you use a regular AutoField, not Hashid*Field?

My code:

class Foo(models.Model):
    id = HashidAutoField(primary_key=True, alphabet="0123456789abcdefghijklmnopqrstuvwxyz")

class MyTests(TestCase):
    @classmethod
    def setUpTestData(cls):
        # Set up data for the whole TestCase
        cls.foo = Foo.objects.create()
        print("setupTestData", cls.foo.id, cls.foo.id.id, cls.foo.id.hashid)

    def test1(self):
        # Some test using self.foo
        print("test1", self.foo.id, self.foo.id.id, self.foo.id.hashid)

My results with SQLite:

setupTestData j4k5xgx 1 j4k5xgx
test1 j4k5xgx 1 j4k5xgx

My results with Postgres:

setupTestData j4k5xgx 1 j4k5xgx
test1 j4k5xgx 1 j4k5xgx
francis-de-ladu commented 3 years ago

Thanks for your fast reply

I'm using Postgres and everything works fine if I switch for an AutoField instead of a HashidAutoField. Don't know if it matters, but the model with the hashid field is a custom user model.

nshafer commented 3 years ago

I don't think having it as a custom User model would make a difference, but you could try to duplicate the behavior with a different model and see if that isolates the issue. But otherwise, I have nothing to go on as to how or why this is happening. Perhaps if you can isolate it to a test project or case that would allow me to duplicate?

dmasson commented 3 years ago

Here's a code that reproduces the error, I'm adding all the imports that I have in the models file, I have some more 3 models that I'm not showing here, I'm using sqlite, I hope this information helps !! :

models.py

import inspect
import logging
from datetime import date, datetime, timedelta
from email.mime.text import MIMEText

from django.conf import settings
from django.core.exceptions import ValidationError
from django.db import models
from django.db.models import Count
from django.template.defaultfilters import date as date_filter
from django.utils.crypto import get_random_string
from django.utils.translation import ugettext_lazy as _
from hashid_field import HashidAutoField
from ics import Calendar, Event

logger = logging.getLogger(__name__)

class Foo(models.Model):
    id = HashidAutoField(primary_key=True, alphabet="0123456789abcdefghijklmnopqrstuvwxyz")

tests.py

from django.test import TestCase

from .models import Foo

class MyTests(TestCase):
    @classmethod
    def setUpTestData(cls):
        # Set up data for the whole TestCase
        cls.foo = Foo.objects.create()
        print("setupTestData", cls.foo.id, cls.foo.id.id, cls.foo.id.hashid)

    def test1(self):
        # Some test using self.foo
        print("test1", self.foo.id, self.foo.id.id, self.foo.id.hashid)

Test output:

./manage.py test
[DEBUG] 2021-07-27 16:19:35,381 :: Using selector: KqueueSelector
System check identified no issues (0 silenced).
setupTestData jmvnjz7 1 jmvnjz7
test1 4rqramj 1 4rqramj
.
----------------------------------------------------------------------
Ran 1 test in 0.001s

OK
Destroying test database for alias 'default'...

setupTestData jmvnjz7 1 jmvnjz7
test1 4rqramj 1 4rqramj

Pipfile

[[source]]
name = "pypi"
url = "https://pypi.org/simple"
verify_ssl = true

[dev-packages]
rope = "*"
colorlog = "*"
django-debug-toolbar = "*"
coverage = "*"
pylint-django = "*"

[packages]
django = "*"
autopep8 = "*"
django-bootstrap4 = "*"
pylint-django = "*"
django-hashid-field = "*"
python-dateutil = "*"
django-ses = "*"
gunicorn = "*"
pillow = "*"
ics = "*"
dj-database-url = "*"
django-storages = "*"
boto3 = "*"
sorl-thumbnail = "*"
django-crontab = "*"
tatsu = "*"

[requires]
python_version = "3.9"
nshafer commented 3 years ago

OK, so the underlying integer isn't changing, but something with either the field or the Hashid object is for some reason. What that reason is is eluding me with this information. Debugging time. Replace MyTests class with this and run ./manage.py test again, then paste the output back.

class MyTests(TestCase):
    @classmethod
    def setUpTestData(cls):
        # Set up data for the whole TestCase
        cls.foo = Foo.objects.create()
        print("setupTestData", cls.foo.id, cls.foo.id.id, cls.foo.id.hashid)
        cls.dumpHashidSettings("setupTestData Foo", Foo._meta.get_field('id'))
        cls.dumpHashidSettings("setupTestData foo", cls.foo.id)

    def test1(self):
        # Some test using self.foo
        print("test1", self.foo.id, self.foo.id.id, self.foo.id.hashid)
        self.dumpHashidSettings("test1 Foo", Foo._meta.get_field('id'))
        self.dumpHashidSettings("test1 foo", self.foo.id)

    @classmethod
    def dumpHashidSettings(cls, prefix, field):
        import hashlib
        for k, v in field.__dict__.items():
            if "salt" in k:
                v = hashlib.sha256(v.encode()).hexdigest()
            print("{} - {}: {}".format(prefix, k, v))

This will one-way hash your salt, so should be safe to paste back here.

dmasson commented 3 years ago

Here it is:

setupTestData jmvnjz7 1 jmvnjz7
setupTestData Foo - salt: c202554767f774fdaf3a3ef6fddd1a8b91bfa82fc58a517ff8154c1739fec6e0
setupTestData Foo - min_length: 7
setupTestData Foo - alphabet: 0123456789abcdefghijklmnopqrstuvwxyz
setupTestData Foo - _hashids: <hashids.Hashids object at 0x104445340>
setupTestData Foo - allow_int_lookup: False
setupTestData Foo - enable_hashid_object: True
setupTestData Foo - enable_descriptor: True
setupTestData Foo - prefix: 
setupTestData Foo - name: id
setupTestData Foo - verbose_name: id
setupTestData Foo - _verbose_name: None
setupTestData Foo - primary_key: True
setupTestData Foo - max_length: None
setupTestData Foo - _unique: False
setupTestData Foo - blank: True
setupTestData Foo - null: False
setupTestData Foo - remote_field: None
setupTestData Foo - is_relation: False
setupTestData Foo - default: <class 'django.db.models.fields.NOT_PROVIDED'>
setupTestData Foo - editable: True
setupTestData Foo - serialize: False
setupTestData Foo - unique_for_date: None
setupTestData Foo - unique_for_month: None
setupTestData Foo - unique_for_year: None
setupTestData Foo - choices: None
setupTestData Foo - help_text: 
setupTestData Foo - db_index: False
setupTestData Foo - db_column: None
setupTestData Foo - _db_tablespace: None
setupTestData Foo - auto_created: False
setupTestData Foo - creation_counter: 53
setupTestData Foo - _validators: []
setupTestData Foo - _error_messages: None
setupTestData Foo - error_messages: {'invalid_choice': 'Valor %(value)r no es una opción válida.', 'null': 'Este campo no puede ser nulo.', 'blank': 'Este campo no puede estar vacío.', 'unique': 'Ya existe %(model_name)s con este %(field_label)s.', 'unique_for_date': '%(field_label)s debe ser único para %(date_field_label)s %(lookup_type)s.', 'invalid': "'%(value)s' value must be a positive integer or a valid Hashids string.", 'invalid_hashid': "'%(value)s' value must be a valid Hashids string."}
setupTestData Foo - attname: id
setupTestData Foo - column: id
setupTestData Foo - concrete: True
setupTestData Foo - model: <class 'local_apps.inventory.models.Foo'>
setupTestData Foo - cached_col: Col(inventory_foo, inventory.Foo.id)
setupTestData Foo - validators: []
setupTestData Foo - _get_default: <function return_None at 0x1037b8310>
setupTestData foo - _hashids: <hashids.Hashids object at 0x104445340>
setupTestData foo - _salt: c202554767f774fdaf3a3ef6fddd1a8b91bfa82fc58a517ff8154c1739fec6e0
setupTestData foo - _min_length: 7
setupTestData foo - _alphabet: wne37x9gl4madoj58p26byrq
setupTestData foo - _prefix: 
setupTestData foo - _id: 1
setupTestData foo - _hashid: jmvnjz7
test1 4rqramj 1 4rqramj
test1 Foo - salt: c202554767f774fdaf3a3ef6fddd1a8b91bfa82fc58a517ff8154c1739fec6e0
test1 Foo - min_length: 7
test1 Foo - alphabet: 0123456789abcdefghijklmnopqrstuvwxyz
test1 Foo - _hashids: <hashids.Hashids object at 0x104445340>
test1 Foo - allow_int_lookup: False
test1 Foo - enable_hashid_object: True
test1 Foo - enable_descriptor: True
test1 Foo - prefix: 
test1 Foo - name: id
test1 Foo - verbose_name: id
test1 Foo - _verbose_name: None
test1 Foo - primary_key: True
test1 Foo - max_length: None
test1 Foo - _unique: False
test1 Foo - blank: True
test1 Foo - null: False
test1 Foo - remote_field: None
test1 Foo - is_relation: False
test1 Foo - default: <class 'django.db.models.fields.NOT_PROVIDED'>
test1 Foo - editable: True
test1 Foo - serialize: False
test1 Foo - unique_for_date: None
test1 Foo - unique_for_month: None
test1 Foo - unique_for_year: None
test1 Foo - choices: None
test1 Foo - help_text: 
test1 Foo - db_index: False
test1 Foo - db_column: None
test1 Foo - _db_tablespace: None
test1 Foo - auto_created: False
test1 Foo - creation_counter: 53
test1 Foo - _validators: []
test1 Foo - _error_messages: None
test1 Foo - error_messages: {'invalid_choice': 'Valor %(value)r no es una opción válida.', 'null': 'Este campo no puede ser nulo.', 'blank': 'Este campo no puede estar vacío.', 'unique': 'Ya existe %(model_name)s con este %(field_label)s.', 'unique_for_date': '%(field_label)s debe ser único para %(date_field_label)s %(lookup_type)s.', 'invalid': "'%(value)s' value must be a positive integer or a valid Hashids string.", 'invalid_hashid': "'%(value)s' value must be a valid Hashids string."}
test1 Foo - attname: id
test1 Foo - column: id
test1 Foo - concrete: True
test1 Foo - model: <class 'local_apps.inventory.models.Foo'>
test1 Foo - cached_col: Col(inventory_foo, inventory.Foo.id)
test1 Foo - validators: []
test1 Foo - _get_default: <function return_None at 0x1037b8310>
test1 foo - _salt: c202554767f774fdaf3a3ef6fddd1a8b91bfa82fc58a517ff8154c1739fec6e0
test1 foo - _min_length: 7
test1 foo - _alphabet: wne37x9gl4madoj58p26byrq
test1 foo - _hashids: <hashids.Hashids object at 0x1049ad910>
test1 foo - _prefix: 
test1 foo - _id: 1
test1 foo - _hashid: 4rqramj
.
----------------------------------------------------------------------
Ran 1 test in 0.001s

OK
nshafer commented 3 years ago

Oh, seeing the alphabet changing and how it was changing made me suddenly realize that you're on an old version of django-hashids-field. I reread your comment earlier that you're on 3.2.1. 3.3.0 was released on 5/11/2021, and fixes a regression from before in how the module interacts with the Hashids library. It should be perfectly safe to upgrade as long as you're not doing anything really deep and funky with the Hashid class instances, which is the only reason I incremented the minor version.

I think what's happening here is a combination with Django 3.2, where it does a copy.deepcopy of any objects added in setupTestData (as outline in the docs). When that happens, it's getting the scrambled version of the alphabet, which then changes the results of hashing. I tested the copy.deepcopy thing earlier today, but I was of course testing against django-hashids-field 3.3.0 and django 3.2, not django-hashids-field 3.2.1.

So try upgrading to django-hashids-field=3.3.0 and test again.

dmasson commented 3 years ago

Problem solved !! @nshafer Thank you !!! Should I close the issue ?

nshafer commented 3 years ago

No problem. Have a good one.

francis-de-ladu commented 3 years ago

@nshafer Could you to publish version 3.3.0 to pypi?

nshafer commented 3 years ago

Oh my, well there's the real problem. Git has the tag, which happens as part of the publish, but there must have been an error or something on upload and I never checked it. Done now. A thousand apologies.

francis-de-ladu commented 3 years ago

Well, that was quick! Upgrading to 3.3.0 also fixed the issue for me.

Thanks @nshafer :)