search

archesproject / arches

Arches is a web platform for creating, managing, & visualizing geospatial data. Arches was inspired by the needs of the Cultural Heritage community, particularly the widespread need of organizations to build & manage cultural heritage inventories
GNU Affero General Public License v3.0
219 stars 144 forks source link

Permissions framework classes aren't stateful #11553

Open jacobtylerwalls opened 4 weeks ago

jacobtylerwalls commented 4 weeks ago

The permissions framework classes inheriting from ArchesPermissionBase don't have any state, e.g. instance attributes. They get instantiated as singletons and attached as a module-level constant. They're really just a collection of pure functions, probably to rhyme with the pure function shortcuts from django-guardian.

What this means in practice is that there's no way to avoid repetitive queries like User.objects.all() inside get_restricted_users(), a pure function. @chiatt profiled indexing and found that almost all of the time was spent inside get_restricted_users(). Much of that in turn is due to design flaws in django-guardian, but some of it is avoidable on our side. The time spent iterating the Users actually takes longer than iterating the ResourceInstances!

https://github.com/archesproject/arches/blob/a8e97834ff8e6e9658e8e82d1bc7c8871258cc3d/arches/app/permissions/arches_default_allow.py#L134

For 1 million resources, that means 1 million avoidable User.objects.all() queries if we could just provide the users somehow to get_users_with_perms().

If we can't provide state at the instance attribute level because we want to continue treating the instances as singletons, maybe we can provide some sort of dictionary holding state and adjust the signatures of all the pure functions to take that dict as an optional keyword argument.

jacobtylerwalls commented 2 weeks ago

/cc @whatisgalen, may be relevant to the work you mentioned in standup today?