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
211 stars 142 forks source link

Consider retiring proxy models #11367

Open jacobtylerwalls opened 3 weeks ago

jacobtylerwalls commented 3 weeks ago

https://github.com/archesproject/arches/pull/11362/commits/95f928dcc6bb6925a2d5bf6678b105a629cf0f32 exposed a scenario where Django failed to discover the proxy models. (See comment there for more information and a workaround.)

But over the medium term, is it worth investigating whether we can just retire the proxy models?

Proxy models are doc'd as useful for multi-table inheritance, or altering ORM-level behavior (overriding default ordering, or .objects, etc). Instead, we're using them mostly for business logic and for loading data from a python dict.

If we reimplemented that functionality on the original models or as pure functions, I wonder if it would ease maintaining Arches and onboarding new developers. (I still find the proxy models a bit forbidding and avoid using them if I can manage.)

chrabyrd commented 3 weeks ago

Spicy! 🌶️

I like the idea of bringing Arches more inline with documented use. But proxy models are largely are pretty embedded and I imagine it will take serious effort to undo the pattern

whatisgalen commented 3 weeks ago

one wrinkle is that in some cases the proxy models import settings via from arches.app.models.system_settings import settings and there's also this comment at the top of models.py:

# can't use "arches.app.models.system_settings.SystemSettings" because of circular refernce issue
# so make sure the only settings we use in this file are ones that are static (fixed at run time)
from django.conf import settings

...however, at first glance it appears all the settings kwargs proxy models call are the static ones hard-coded onto settings.py (rather than dynamically imported via the system settings resource instance) so perhaps this may not be a blocker