Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changing any variables of a User nullifies their history (I think) #4

Open
DreamShark-Bytes opened this issue Sep 25, 2022 · 2 comments
Assignees

Comments

@DreamShark-Bytes
Copy link

DreamShark-Bytes commented Sep 25, 2022

Sorry but I couldn't get the code to run so I can't test this (probably just a result of my ignorance on all this, still super green when it comes to GitHub), but in my attempt to learn from your code, I think I found a potential bug.

I'm seeing that when it performs save_history, it's using the custom hash function you laid out in the Person class, which is this:

    def __hash__(self):
        return int(hashlib.md5(
            f'{self.name}:{self.dob}:{self.sex}:{self.lat_lng}'.encode('utf-8')
        ).hexdigest()[:8], 16)

Does this combine all of those fields (name, dob, sex, and location) into a unique identifier?
Meaning, any changes to those items would break the user's link to their gift exchange history? I feel like name and location are especially susceptible to change given that the scope your wife gave you was "3 years" of history. The people could move locations, change their names from getting married, or change genders/pronouns.

All this means a person with a change could more easily be assigned someone they've been assigned to previously.

here was my attempt to re-create the bug:
possible example of bug

@sethblack
Copy link
Owner

It does! Good catch! Especially on the gender/pronouns, names, and locations; totally legit. The hashing algo mixes everything.

Hmm. 🤔 I'll have to give this one some thought, but I think I can fix it.

@sethblack sethblack self-assigned this Nov 25, 2022
@cwkingjr
Copy link

It seems like the simplest fix is to require unique names (which is likely an implicit requirement anyhow). However, to cover all bases, you'd still have to do something like include a history updater routine that accepts old and new names and rebuilds the history hashes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants