This repository has been archived by the owner on Jan 29, 2022. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add user-based quota enforcement #133
Add user-based quota enforcement #133
Changes from 2 commits
fad61a6
88dec68
cb0e3ba
31dae79
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually, with "project-based" quotas, this can be nullable.
Ideally, the relationship would point the other way, but since
User
is practically not directly extensible, this is a reasonable substitute. Given the_create_user_quota
signal handler, we can rely onuser.quota
always existing for now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we pull this out of here and have a separate associative table to map users to quotas? Then, every user could have an entry in that table, but not every quota would have to have an entry there (i.e. for any non-user associated quotas).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would be a
Profile
model (Option 2 here), which would also store any other user-related info that we wanted to extend.Do you foresee any upcoming features where we want to store additional information (beyond just foreign key relationships) with the user? If so, I think it's worth a detour to add the
Profile
now.If not, I think it's worth merging this approach as-is, to start giving us working quota support. As we extend to non-user-associated ("project") quotas, we can rework the implementation internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add the profile model, it's not much code and should be straightforward. Just make it a commit on this same topic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started to create a
Profile
model. This is something that's supported by Factory Boy: https://factoryboy.readthedocs.io/en/latest/recipes.html#example-django-s-profile , but a significant complication is the fact that we'll have to annotate all of our factories (not justUserFactory
andProfileFactory
) with@factory.django.mute_signals(post_save)
, which will disable anypost_save
handlers during essentially all tests. Eventually, we might need this, but it will introduce some drag.IMO, the current implementation is adequate. At runtime, the reverse relation
user.quota
is adequate to support this feature and any lookup failures will immediately log 500 errors. If we can't depend on signals to support some behavior (instead requiring DB-level guarantees), I think we'd be giving up an important measure of power that Django provides.In some ways, attaching the
ForeignKey
toQuota
is superior, particularly once that field is nullable (to support project quotas) in the future, since we'll be able to easily filter user quotas from project quotas (I'm not seeing how to do this if theForeignKey
reference points toQuota
).Given the downside of suppressing all
post_save
signals, I think we should only proceed withProfile
if we are going to need it to store additional information on each user. Otherwise, I'd advocate to merge this as-is, and proceed with developing more advanced features on top.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just want to be clear that putting the user field here is a relational hack. We're misusing nullability to model what should be a relation. We can merge this as is, but I'd like to create an issue to address this. Insofar as this repository is meant to model best practices, one of the things I really want us to exemplify is proper use of relational databases with Django, which means we'll need to work through the warts. As nice as FactoryBoy is, we can't let it be the library driving our design choices.
Let's make the ticket, I'll take a crack at this myself later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish we didn't have to do this -- do you think we'd have any luck upstreaming a feature request to psycopg2 to provide more structured info on their IntegrityErrors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Not to hold up this PR or anything, just an idle thought.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's where the
IntegrityError
is generated.The PostgreSQL docs state:
So, it looks like it's possible in principle for only this particular case. Psycopg 2 supports >= PostgreSQL 7.4, so support would need to be conditional.
@zachmullen I would start by filing an issue in Psycopg 2, but if they decline it there, perhaps they'd consider it in (the apparently actively-developed) Psycopg 3. Let me know if you do so, so I can subscribe to it.