WordPress / openverse

Openverse is a search engine for openly-licensed media. This monorepo includes all application code.
https://openverse.org
MIT License
234 stars 183 forks source link

Replace function-local dict media type -> model relationship configurations with static module dicts or an otherwise shared approach #4553

Open sarayourfriend opened 2 months ago

sarayourfriend commented 2 months ago

Current Situation

See this thread for context: https://github.com/WordPress/openverse/pull/4544#discussion_r1653696199

This pattern exists widely in our code, and does not need to, and has the following current and potential problems:

Suggested Improvement

Either:

To me the latter is preferred because it creates a much cleaner and more consistent code, without the need to manually maintain dicts of these relationships at all.

Benefit

Avoid the anti-pattern qualities of the function-local inline dict approach.

dhruvkb commented 2 months ago

Adding a note here that the approach described in suggested improvement point 1 would be similar to this test fixture.

https://github.com/WordPress/openverse/blob/73c0ad5726f289fcb8b5d556fe014a1dd76b37e8/api/test/fixtures/media_type_config.py#L72-L111

I would also favour the second approach, but that's because I expect a number of circular dependency issues to come up using the first approach.

sarayourfriend commented 3 weeks ago

I'm unassigning myself this for now and closing https://github.com/WordPress/openverse/pull/4599.

I need to focus on some other work at the moment and do not have time to rebase and get that PR reviewable. There is some good feedback and discussion in the PR, and @krysal shared a preference for the cached property approach, though I personally don't see how it's that much less meta-programming than the metaclass approach, and it does not completely solve the issue of fiddly configuration required on all of these support models, namely the related fields, which are arguably one of the most important things to get right here, and the most annoying to deal with when they aren't. In fact, if just the related names were unified, at least you can get the model from most of these relations on the class.

I think the meta-class approach works well, and don't fully understand the trepidation towards hooking into Django in this way, but do understand that not everyone is comfortable with this kind of approach and would prefer verbose manual configuration of these generic relations rather than programatically handling it as I've done in the PR.

Regardless, I'm happy to review and give help if someone else takes this up and solves it. Hopefully it's clear from the PR how much of an improvement it is in all the code working on these relationships.