Quota representation and behavior in model layer #41

zachmullen · 2020-10-08T01:05:03Z

The scope of this PR is just to add the database representation of quotas, not to actually add behavior to enforce them.

I went with the existing strategy, and the one that seems to be recommended in Django land, of using a OneToOneField to effectively attach columns to user rows, but using a separate relation. The performance implications, as far as I can guess, are negligible in both directions simply due to how we use quotas. However, there are other tradeoffs that merit consideration:

Pros:

More sanctioned pattern at the Django level? This is a good article I was reading on the subject. Not sure if it's authoritative.
Separation of concerns in the application layer. Having the quota concept decoupled from the identity concept is definitely valuable in our Python code, though I'd still need to be convinced that it provides any value to separate it at the DBMS layer.

Cons:

Violates the principle of "make invalid states unrepresentable". If what we'd really like is to ensure that every user has a used count and allowed quota associated with them (which I think we do), it would be preferable to simply add these as columns to the existing user table and make them required and provide the appropriate constraints. As written, it's possible at the DBMS layer to get into a state where a user does not have associated quota columns. We can of course enforce this consistency at the application layer, but doing so is subverting some of the value of our wonderfully powerful DBMS, which should generally make us feel less confident.
This isn't to do with quotas specifically, but if we are never planning to extend the user model, we lose out on additional desirable capabilities, like adding extra methods or properties onto it. However, it looks like we could achieve this using a Proxy model as mentioned in the article above.
Any operation where quotas are required will require an additional database query, or at the very least a join. Again, this is probably negligible since we don't really do bulk operations on quotas.

FWIW I'm fine with the approach here, I just want us to make an informed decision. We're taking on a little bit of debt in this approach by forcing our app layer to have more consistency checks.

dkc/core/models/user_quota.py

dkc/core/apps.py

brianhelba · 2020-10-08T13:39:01Z

dkc/core/models/user_quota.py


    # The user is needed to stringify, so join it when directly querying for a quota
    objects = SelectRelatedManager('user')

    def __str__(self):
-        return f'Quota for <{self.user}>; {self.allocation} bytes'


I'm worried this might actually be a little long. Have you seen it rendered somewhere in the admin page yet?

It doesn't currently appear on the admin page. I may just strip some of these things out for now and let us deal with the admin page later. I'll probably also remove the SelectRelatedManager as I don't even think we'll query the quota tables directly, but rather always through FK lookups.

dkc/core/models/user_quota.py

dkc/core/tests/test_quotas.py

brianhelba

Finished my initial review. See inline comments.

zachmullen · 2020-10-08T19:23:27Z

PTAL, I've force pushed a new initial commit intended to capture the data modeling and quota resolving behavior. The approach as written is:

Every user, when created, gets a quota associated with their user record. A quota is a tuple of two strictly integer values: amount used, and amount allowed.
Every folder has an owner field, meaning each and every folder in the system is considered to be owned by one user.
Every root folder, when created, gets a quota associated with it. A folder quota record contains a strictly integer valued amount used column, and a nullable integer allowed column. A null allowed column on a folder means that the allowed amount is actually the owning user's allowed amount.
The process of converting a folder from counting against the user's quota to instead having its own quota involves setting a non-null allowed amount as the folder quota.

This approach means that we will need to specially handle when an admin user flips a folder quota from null to non-null, by transferring the used amount from the owning user's pile to the folder's custom allotment. In that case it's just a matter of subtracting from the user's used value, as the folder's own used should already be stored. I haven't written this logic yet -- I plan to put all quota computation and enforcement logic on future PRs, if this approach is approved.

zachmullen · 2020-10-09T01:24:47Z

I went ahead and stuck rudimentary quota accounting, as well as enforcement, on this branch.

zachmullen · 2020-10-09T01:32:13Z

dkc/core/models/folder.py

+                        f'{user_quota.used}B > {user_quota.allowed}B.'
+                    )
+        elif amount > 0:
+            folder_quota.refresh_from_db()


Please advise on best practices here -- because I'm using an F expression, refresh_from_db needs to be called before the updated attributes will have their normally-typed values. I chose to only call refresh_from_db internally here if it's strictly necessary, for the sake of performance, but callers of this function would then have to be concerned with refreshing the objects themselves if they need to use these properties again (which they probably won't).

zachmullen · 2020-10-09T01:33:56Z

dkc/core/models/file.py

    instance.size = instance.blob.size
+
+    if not instance.pk:  # this is a new file; reserve quota
+        instance.folder.increment_quota(instance.size)


This is a temporary place for incrementing the quota. Ultimately we'll want to do that when they first initiate an upload, before any bytes get stored anywhere.

This provides a workable interface for controlling system quotas.

zachmullen · 2020-10-09T20:24:31Z

This now has all the quota features we need for a MVP. The branch is ready for final review, however we shouldn't merge it until we solve one problem that it introduces.

This branch adds a new constraint: all folders must have an owning user. I like this constraint and want to keep it. However, since we don't currently have user authentication, the process of assigning an owner to a newly created folder doesn't exist. As such, on this branch right now, it's impossible to create folders via the REST API. Two paths forward would be:

Hack together something to make every folder have some arbitrary owner that we grab from the database at create time.
Wait to merge this branch until we have user auth in place, make the folder creation REST endpoint only open to authenticated users, and then attach it there.

Now, when a folder is granted its own specific quota, its usage is subtracted from its owners pool.

dkc/core/tests/factories.py

dkc/core/models/user_quota.py

dkc/core/models/folder_quota.py

* Use different db create method * Use SelfAttribute in factory Co-authored-by: Brian Helba <brian.helba@kitware.com>

zachmullen · 2020-10-17T01:42:34Z

Good news... I've worked around the lack of authentication in the REST API for now. This is ready to merge!

zachmullen commented Oct 8, 2020

View reviewed changes

dkc/core/models/user_quota.py Show resolved Hide resolved