Add user-based quota enforcement #133
Conversation
dkc/core/models/quota.py
Outdated
| allowed = models.PositiveBigIntegerField(default=_default_user_quota) | ||
|
|
||
| # OneToOneField ensures that only one Quota per User exists | ||
| user = models.OneToOneField(User, on_delete=models.CASCADE, related_name='quota') |
There was a problem hiding this comment.
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 on user.quota always existing for now.
There was a problem hiding this comment.
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.
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.
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.
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 just UserFactory and ProfileFactory) with @factory.django.mute_signals(post_save), which will disable any post_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 to Quota 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 the ForeignKey reference points to Quota).
Given the downside of suppressing all post_save signals, I think we should only proceed with Profile 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.
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.
| # local instance, which might need to be rolled back on a failure | ||
| Quota.objects.filter(pk=self.pk).update(used=(models.F('used') + amount)) | ||
| except IntegrityError as e: | ||
| if '"used_lte_allowed"' in str(e): |
There was a problem hiding this comment.
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.
(Not to hold up this PR or anything, just an idle thought.)
There was a problem hiding this comment.
Here's where the IntegrityError is generated.
For some types of errors, the server reports the name of a database object (a table, table column, data type, or constraint) associated with the error; for example, the name of the unique constraint that caused a unique_violation error. Such names are supplied in separate fields of the error report message so that applications need not try to extract them from the possibly-localized human-readable text of the message. As of PostgreSQL 9.3, complete coverage for this feature exists only for errors in SQLSTATE class 23 (integrity constraint violation), but this is likely to be expanded in future.
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.
Co-authored-by: Zach Mullen <[email protected]>
This does not yet support "project-based" quotas. That will be added in a future PR, as it requires a UI to manage it.
This supersedes PRs #41, #49, #50.
Fixes #76.