-
Notifications
You must be signed in to change notification settings - Fork 212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Unify names of generic relations to media classes #4599
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,116 @@ | ||
# Generated by Django 4.2.11 on 2024-07-04 07:01 | ||
|
||
from django.conf import settings | ||
from django.db import migrations, models | ||
import django.db.models.deletion | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
migrations.swappable_dependency(settings.AUTH_USER_MODEL), | ||
('api', '0068_remove_audioreport_status_remove_imagereport_status'), | ||
] | ||
|
||
operations = [ | ||
migrations.AlterModelOptions( | ||
name='audiodecision', | ||
options={'default_related_name': 'decision'}, | ||
), | ||
migrations.AlterModelOptions( | ||
name='audiodecisionthrough', | ||
options={'default_related_name': 'decision_through'}, | ||
), | ||
migrations.AlterModelOptions( | ||
name='audioreport', | ||
options={'default_related_name': 'report'}, | ||
), | ||
migrations.AlterModelOptions( | ||
name='deletedaudio', | ||
options={'default_related_name': 'deleted_media', 'verbose_name_plural': 'Deleted audio'}, | ||
), | ||
migrations.AlterModelOptions( | ||
name='deletedimage', | ||
options={'default_related_name': 'deleted_media'}, | ||
), | ||
migrations.AlterModelOptions( | ||
name='imagedecision', | ||
options={'default_related_name': 'decision'}, | ||
), | ||
migrations.AlterModelOptions( | ||
name='imagedecisionthrough', | ||
options={'default_related_name': 'decision_through'}, | ||
), | ||
migrations.AlterModelOptions( | ||
name='imagereport', | ||
options={'default_related_name': 'report'}, | ||
), | ||
migrations.AlterModelOptions( | ||
name='sensitiveaudio', | ||
options={'default_related_name': 'sensitive_media', 'verbose_name_plural': 'Sensitive audio'}, | ||
), | ||
migrations.AlterModelOptions( | ||
name='sensitiveimage', | ||
options={'default_related_name': 'sensitive_media'}, | ||
), | ||
migrations.AlterField( | ||
model_name='audiodecision', | ||
name='media_objs', | ||
field=models.ManyToManyField(help_text='The audio items being moderated.', related_name='decision', through='api.AudioDecisionThrough', to='api.audio'), | ||
), | ||
migrations.AlterField( | ||
model_name='audiodecision', | ||
name='moderator', | ||
field=models.ForeignKey(help_text='The moderator who undertook this decision.', on_delete=django.db.models.deletion.DO_NOTHING, related_name='audio_decision', to=settings.AUTH_USER_MODEL), | ||
), | ||
migrations.AlterField( | ||
model_name='audiodecisionthrough', | ||
name='media_obj', | ||
field=models.ForeignKey(db_column='identifier', db_constraint=False, on_delete=django.db.models.deletion.DO_NOTHING, related_name='decision_through', to='api.audio', to_field='identifier'), | ||
), | ||
migrations.AlterField( | ||
model_name='audioreport', | ||
name='media_obj', | ||
field=models.ForeignKey(db_column='identifier', db_constraint=False, help_text='The reference to the audio being reported.', on_delete=django.db.models.deletion.DO_NOTHING, related_name='report', to='api.audio', to_field='identifier'), | ||
), | ||
migrations.AlterField( | ||
model_name='deletedaudio', | ||
name='media_obj', | ||
field=models.OneToOneField(db_column='identifier', db_constraint=False, help_text='The reference to the deleted audio.', on_delete=django.db.models.deletion.DO_NOTHING, primary_key=True, related_name='deleted_media', serialize=False, to='api.audio', to_field='identifier'), | ||
), | ||
migrations.AlterField( | ||
model_name='deletedimage', | ||
name='media_obj', | ||
field=models.OneToOneField(db_column='identifier', db_constraint=False, help_text='The reference to the deleted image.', on_delete=django.db.models.deletion.DO_NOTHING, primary_key=True, related_name='deleted_media', serialize=False, to='api.image', to_field='identifier'), | ||
), | ||
migrations.AlterField( | ||
model_name='imagedecision', | ||
name='media_objs', | ||
field=models.ManyToManyField(help_text='The image items being moderated.', related_name='decision', through='api.ImageDecisionThrough', to='api.image'), | ||
), | ||
migrations.AlterField( | ||
model_name='imagedecision', | ||
name='moderator', | ||
field=models.ForeignKey(help_text='The moderator who undertook this decision.', on_delete=django.db.models.deletion.DO_NOTHING, related_name='image_decision', to=settings.AUTH_USER_MODEL), | ||
), | ||
migrations.AlterField( | ||
model_name='imagedecisionthrough', | ||
name='media_obj', | ||
field=models.ForeignKey(db_column='identifier', db_constraint=False, on_delete=django.db.models.deletion.DO_NOTHING, related_name='decision_through', to='api.image', to_field='identifier'), | ||
), | ||
migrations.AlterField( | ||
model_name='imagereport', | ||
name='media_obj', | ||
field=models.ForeignKey(db_column='identifier', db_constraint=False, help_text='The reference to the image being reported.', on_delete=django.db.models.deletion.DO_NOTHING, related_name='report', to='api.image', to_field='identifier'), | ||
), | ||
migrations.AlterField( | ||
model_name='sensitiveaudio', | ||
name='media_obj', | ||
field=models.OneToOneField(db_column='identifier', db_constraint=False, help_text='The reference to the sensitive audio.', on_delete=django.db.models.deletion.DO_NOTHING, primary_key=True, related_name='sensitive_media', serialize=False, to='api.audio', to_field='identifier'), | ||
), | ||
migrations.AlterField( | ||
model_name='sensitiveimage', | ||
name='media_obj', | ||
field=models.OneToOneField(db_column='identifier', db_constraint=False, help_text='The reference to the sensitive image.', on_delete=django.db.models.deletion.DO_NOTHING, primary_key=True, related_name='sensitive_media', serialize=False, to='api.image', to_field='identifier'), | ||
), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,9 @@ | ||
from typing import Protocol | ||
|
||
from django.db import models | ||
|
||
from api.constants.media_types import MediaType | ||
|
||
|
||
class OpenLedgerModel(models.Model): | ||
created_on = models.DateTimeField(auto_now_add=True) | ||
|
@@ -12,3 +16,74 @@ def __iter__(self): | |
for field_name in self._meta.get_fields(): | ||
value = getattr(self, field_name, None) | ||
yield field_name, value | ||
|
||
|
||
class OpenverseMediaModel(Protocol): | ||
media_type: MediaType | ||
Comment on lines
+21
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is unused, left over from another experimental approach I tried (one that didn't leverage metaclass kwargs and instead generated a new metaclass per-abstract related media model, or used a decorator, both of which were a little ugly or didn't work). Anyway, it's meant to be removed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you leave a comment in the code saying it is meant to be removed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It needs to be removed from this PR. I'll do so when I get back around to making updates to this based on feedback for the overall approach. |
||
|
||
|
||
class MediaRelatedBase(models.base.ModelBase): | ||
""" | ||
Metaclass for models related to any media type, for each each media type has its own subclass. | ||
|
||
The metaclass ensures media types use the same names to refer to related models | ||
of the same kind. Django will not inherit properties from parent the ``Meta`` | ||
of model base classes. As such, this class implements a "related_name" specific | ||
inheritance via base classes' ``Meta``. | ||
|
||
Additionally, it ensures base classes define the media class they are related to | ||
and that the media class has a back reference to the related model. The method | ||
used ensures all media classes have identical properties with which to reference | ||
shared related models. | ||
""" | ||
|
||
def _handle_abstract(cls, name, bases, attrs, related_name: str | None, **kwargs): | ||
if not related_name: | ||
raise TypeError( | ||
f"{name} missing 1 required keyword-only argument: 'related_name'" | ||
) | ||
|
||
setattr(attrs["Meta"], "default_related_name", related_name) | ||
|
||
return super().__new__(cls, name, bases, attrs, **kwargs) | ||
|
||
def _handle_final(cls, name, bases, attrs, **kwargs): | ||
if "media_class" not in attrs: | ||
raise TypeError(f"{name} missing required attribute: 'media_class'") | ||
|
||
media_class = attrs["media_class"] | ||
|
||
default_related_name = None | ||
for base in bases: | ||
if not hasattr(base, "Meta"): | ||
continue | ||
|
||
default_related_name = getattr(base.Meta, "default_related_name", None) | ||
if not default_related_name: | ||
continue | ||
|
||
if "Meta" not in attrs: | ||
attrs["Meta"] = type( | ||
"Meta", (), {"default_related_name": default_related_name} | ||
) | ||
else: | ||
setattr(attrs["Meta"], "default_related_name", default_related_name) | ||
|
||
if default_related_name is None: | ||
raise NotImplementedError( | ||
f"Media-related model {name} is not abstract, but has no base class that " | ||
"defines 'default_related_name'. Did you mean to mark this model abstract?" | ||
) | ||
|
||
new_model = super().__new__(cls, name, bases, attrs, **kwargs) | ||
|
||
setattr(media_class, f"{default_related_name}_class", new_model) | ||
|
||
return new_model | ||
|
||
def __new__(cls, name, bases, attrs, *, related_name: str | None = None, **kwargs): | ||
if "Meta" in attrs and getattr(attrs["Meta"], "abstract", False): | ||
# Abstract models are models with a ``Meta`` defined with ``abstract = True`` | ||
return cls._handle_abstract(cls, name, bases, attrs, related_name, **kwargs) | ||
else: | ||
return cls._handle_final(cls, name, bases, attrs, **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, these actually do not need to be defined anymore! I accidentally left these in when I was trying one of the other approaches I mentioned that still required manually configuring the related names. The metaclass approach intentionally removes the need for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the approach that I was actually preferring over the metaclass because it is much more explicit and clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't solve the back-references to the class though. You can go through the related set but if it's a one-to-one, there's no good way to do it, as far as I can tell, that won't cause an implicit query.
The complexity comes from trying to solve both problems: