refactor!: custom fields now require dictionary access

Custom fields must now be accessed via `item.custom["my_custom_field"]` i.e. normal dictionary access.

I changed this from normal attribute access as overriding `__getattr__` and `__setattr__` (required for transparent attribute access) had some finicky conflicts with sqlalchemy. Also, it prevented type hinting the custom attribute dictionary as well as integration with data serializers such as pydantic.

Overall, the more I used them, the more issues I found, and the more it felt like a hack. I believe the new explicit dictionary access for custom attributes will prove to be more bulletproof. It also explicitly delineates normal and custom attributes which can be useful in some cases.

alembic/versions/ab5db9861d30_rename_custom_fields.py

@@ -0,0 +1,36 @@
+"""Rename custom fields.
+
+Revision ID: ab5db9861d30
+Revises: 880c5a2d80ed
+Create Date: 2022-12-11 15:24:10.227924
+
+"""
+from alembic import op
+
+# revision identifiers, used by Alembic.
+revision = "ab5db9861d30"
+down_revision = "880c5a2d80ed"
+branch_labels = None
+depends_on = None
+
+
+def upgrade():
+    with op.batch_alter_table("album", schema=None) as batch_op:
+        batch_op.alter_column("_custom_fields", new_column_name="custom")
+
+    with op.batch_alter_table("extra", schema=None) as batch_op:
+        batch_op.alter_column("_custom_fields", new_column_name="custom")
+
+    with op.batch_alter_table("track", schema=None) as batch_op:
+        batch_op.alter_column("_custom_fields", new_column_name="custom")
+
+
+def downgrade():
+    with op.batch_alter_table("track", schema=None) as batch_op:
+        batch_op.alter_column("custom", new_column_name="_custom_fields")
+
+    with op.batch_alter_table("extra", schema=None) as batch_op:
+        batch_op.alter_column("custom", new_column_name="_custom_fields")
+
+    with op.batch_alter_table("album", schema=None) as batch_op:
+        batch_op.alter_column("custom", new_column_name="_custom_fields")

docs/developers/api/core.rst

@@ -28,7 +28,7 @@ Library
 
 .. automodule:: moe.library
    :members:
-   :exclude-members: cache_ok, path
+   :exclude-members: cache_ok, path, custom
    :show-inheritance:
    :special-members: __init__
 

moe/library/album.py

@@ -28,29 +28,6 @@
 class Hooks:
     """Album hook specifications."""
 
-    @staticmethod
-    @moe.hookspec
-    def create_custom_album_fields() -> dict[str, Any]:  # type: ignore
-        """Creates new custom fields for an Album.
-
-        Returns:
-            Dict of the field names to their default values or ``None`` for no default.
-
-        Example:
-            .. code:: python
-
-                return {"my_new_field": "default value", "other_field": None}
-
-            You can then access your new field as if it were a normal field::
-
-                album.my_new_field = "awesome new value"
-
-        Important:
-            Your custom field should follow the same naming rules as any other python
-            variable i.e. no spaces, starts with a letter, and consists solely of
-            alpha-numeric and underscore characters.
-        """  # noqa: DAR202
-
     @staticmethod
     @moe.hookspec
     def is_unique_album(album: "Album", other: "Album") -> bool:  # type: ignore
@@ -90,6 +67,7 @@ class MetaAlbum(MetaLibItem):
         catalog_nums (Optional[set[str]]): Set of all catalog numbers.
         country (Optional[str]): Country the album was released in
             (two character identifier).
+        custom (dict[str, Any]): Dictionary of custom fields.
         date (Optional[datetime.date]): Album release date.
         disc_total (Optional[int]): Number of discs in the album.
         label (Optional[str]): Album release label.
@@ -119,8 +97,7 @@ def __init__(
         **kwargs,
     ):
         """Creates a MetaAlbum object with any additional custom fields as kwargs."""
-        self._custom_fields = self._get_default_custom_fields()
-        self._custom_fields_set = set(self._custom_fields)
+        self.custom = kwargs
 
         self.artist = artist
         self.barcode = barcode
@@ -137,9 +114,6 @@ def __init__(
         if not tracks:
             self.tracks = []
 
-        for key, value in kwargs.items():
-            setattr(self, key, value)
-
         if config.CONFIG.settings.original_date and self.original_date:
             self.date = self.original_date
 
@@ -183,7 +157,7 @@ def fields(self) -> set[str]:
             "original_date",
             "title",
             "track_total",
-        }.union(self._custom_fields)
+        }
 
     def get_track(self, track_num: int, disc: int = 1) -> Optional["MetaTrack"]:
         """Gets a MetaTrack by its track number."""
@@ -222,6 +196,13 @@ def merge(self, other: "MetaAlbum", overwrite: bool = False) -> None:
             if other_value and (overwrite or (not overwrite and not self_value)):
                 setattr(self, field, other_value)
 
+        for custom_field in self.custom:
+            other_value = other.custom.get(custom_field)
+            if other_value and (
+                overwrite or (not overwrite and not self.custom[custom_field])
+            ):
+                self.custom[custom_field] = other_value
+
         log.debug(
             f"MetaAlbums merged. [album_a={self!r}, album_b={other!r}, {overwrite=!r}]"
         )
@@ -279,7 +260,7 @@ def __repr__(self):
         repr_str = "AlbumInfo(" + ", ".join(field_reprs)
 
         custom_field_reprs = []
-        for custom_field, value in self._custom_fields.items():
+        for custom_field, value in self.custom.items():
             custom_field_reprs.append(f"{custom_field}={value}")
         if custom_field_reprs:
             repr_str += ", custom_fields=[" + ", ".join(custom_field_reprs) + "]"
@@ -292,14 +273,6 @@ def __repr__(self):
         repr_str += ")"
         return repr_str
 
-    def _get_default_custom_fields(self) -> dict[str, Any]:
-        """Returns the default custom album fields."""
-        return {
-            field: default_val
-            for plugin_fields in config.CONFIG.pm.hook.create_custom_album_fields()
-            for field, default_val in plugin_fields.items()
-        }
-
 
 # Album generic, used for typing classmethod
 A = TypeVar("A", bound="Album")
@@ -316,6 +289,7 @@ class Album(LibItem, SABase, MetaAlbum):
         catalog_nums (Optional[set[str]]): Set of all catalog numbers.
         country (Optional[str]): Country the album was released in
             (two character identifier).
+        custom (dict[str, Any]): Dictionary of custom fields.
         date (datetime.date): Album release date.
         disc_total (int): Number of discs in the album.
         extras (list[Extra]): Extra non-track files associated with the album.
@@ -347,7 +321,7 @@ class Album(LibItem, SABase, MetaAlbum):
     )
     title: str = cast(str, Column(String, nullable=False))
     track_total: Optional[int] = cast(Optional[int], Column(Integer, nullable=True))
-    _custom_fields: dict[str, Any] = cast(
+    custom: dict[str, Any] = cast(
         dict[str, Any],
         Column(
             MutableDict.as_mutable(JSON(none_as_null=True)),
@@ -375,30 +349,31 @@ def __init__(
         artist: str,
         title: str,
         date: datetime.date,
+        barcode: Optional[str] = None,
+        catalog_nums: Optional[set[str]] = None,
+        country: Optional[str] = None,
         disc_total=1,
+        label: Optional[str] = None,
+        media: Optional[str] = None,
+        original_date: Optional[datetime.date] = None,
+        track_total: Optional[int] = None,
         **kwargs,
     ):
-        """Creates an Album.
-
-        Args:
-            path: Filesystem path of the album directory.
-            artist: Album artist.
-            title: Album title.
-            date: Album release date.
-            disc_total: Number of discs in the album.
-            **kwargs: Any other fields to assign to the album.
-        """
-        self._custom_fields = self._get_default_custom_fields()
-        self._custom_fields_set = set(self._custom_fields)
+        """Creates an Album object with any additional custom fields as kwargs."""
+        self.custom = kwargs
 
         self.path = path
         self.artist = artist
-        self.title = title
+        self.barcode = barcode
+        self.catalog_nums = catalog_nums
+        self.country = country
         self.date = date
         self.disc_total = disc_total
-
-        for key, value in kwargs.items():
-            setattr(self, key, value)
+        self.label = label
+        self.media = media
+        self.original_date = original_date
+        self.track_total = track_total
+        self.title = title
 
         if config.CONFIG.settings.original_date and self.original_date:
             self.date = self.original_date
@@ -483,6 +458,30 @@ def merge(self, other: Union["Album", MetaAlbum], overwrite: bool = False) -> No
         """
         log.debug(f"Merging albums. [album_a={self!r}, album_b={other!r}")
 
+        self._merge_tracks(other, overwrite)
+        self._merge_extras(other, overwrite)
+
+        for field in self.fields:
+            other_value = getattr(other, field, None)
+            self_value = getattr(self, field, None)
+            if other_value and (overwrite or (not overwrite and not self_value)):
+                setattr(self, field, other_value)
+
+        for custom_field in self.custom:
+            other_value = other.custom.get(custom_field)
+            if other_value and (
+                overwrite or (not overwrite and not self.custom[custom_field])
+            ):
+                self.custom[custom_field] = other_value
+
+        log.debug(
+            f"Albums merged. [album_a={self!r}, album_b={other!r}, {overwrite=!r}]"
+        )
+
+    def _merge_tracks(
+        self, other: Union["Album", MetaAlbum], overwrite: bool = False
+    ) -> None:
+        """Merges the tracks of another album into this one."""
         new_tracks: list["Track"] = []
         for other_track in other.tracks:
             conflict_track = None
@@ -494,6 +493,10 @@ def merge(self, other: Union["Album", MetaAlbum], overwrite: bool = False) -> No
                 new_tracks.append(other_track)
         self.tracks.extend(new_tracks)
 
+    def _merge_extras(
+        self, other: Union["Album", MetaAlbum], overwrite: bool = False
+    ) -> None:
+        """Merges the extras of another album into this one."""
         if isinstance(other, Album):
             new_extras: list["Extra"] = []
             for other_extra in other.extras:
@@ -504,16 +507,6 @@ def merge(self, other: Union["Album", MetaAlbum], overwrite: bool = False) -> No
                     new_extras.append(other_extra)
             self.extras.extend(new_extras)
 
-        for field in self.fields:
-            other_value = getattr(other, field, None)
-            self_value = getattr(self, field, None)
-            if other_value and (overwrite or (not overwrite and not self_value)):
-                setattr(self, field, other_value)
-
-        log.debug(
-            f"Albums merged. [album_a={self!r}, album_b={other!r}, {overwrite=!r}]"
-        )
-
     @hybrid_property
     def original_year(self) -> Optional[int]:  # type: ignore
         """Gets an Album's year."""
@@ -546,7 +539,7 @@ def __repr__(self):
         repr_str = "Album(" + ", ".join(field_reprs)
 
         custom_field_reprs = []
-        for custom_field, value in self._custom_fields.items():
+        for custom_field, value in self.custom.items():
             custom_field_reprs.append(f"{custom_field}={value}")
         if custom_field_reprs:
             repr_str += ", custom_fields=[" + ", ".join(custom_field_reprs) + "]"

moe/library/extra.py

@@ -70,12 +70,13 @@ class Extra(LibItem, SABase):
 
     Attributes:
         album (Album): Album the extra file belongs to.
+        custom (dict[str, Any]): Dictionary of custom fields.
         path (pathlib.Path): Filesystem path of the extra file.
     """
 
     __tablename__ = "extra"
 
-    _custom_fields: dict[str, Any] = cast(
+    custom: dict[str, Any] = cast(
         dict[str, Any],
         Column(
             MutableDict.as_mutable(JSON(none_as_null=True)),
@@ -93,23 +94,19 @@ def __init__(self, album: Album, path: Path, **kwargs):
         Args:
             album: Album the extra file belongs to.
             path: Filesystem path of the extra file.
-            **kwargs: Any other fields to assign to the extra.
+            **kwargs: Any custom fields to assign to the extra.
         """
-        self._custom_fields = self._get_default_custom_fields()
-        self._custom_fields_set = set(self._custom_fields)
+        self.custom = kwargs
 
         album.extras.append(self)
         self.path = path
 
-        for key, value in kwargs.items():
-            setattr(self, key, value)
-
         log.debug(f"Extra created. [extra={self!r}]")
 
     @property
     def fields(self) -> set[str]:
         """Returns any editable, extra fields."""
-        return {"album", "path"}.union(self._custom_fields)
+        return {"album", "path"}
 
     @property
     def rel_path(self) -> PurePath:
@@ -147,6 +144,13 @@ def merge(self, other: "Extra", overwrite: bool = False):
             if other_value and (overwrite or (not overwrite and not self_value)):
                 setattr(self, field, other_value)
 
+        for custom_field in self.custom:
+            other_value = other.custom.get(custom_field)
+            if other_value and (
+                overwrite or (not overwrite and not self.custom[custom_field])
+            ):
+                self.custom[custom_field] = other_value
+
         log.debug(
             f"Extras merged. [extra_a={self!r}, extra_b={other!r}, {overwrite=!r}]"
         )
@@ -181,7 +185,7 @@ def __repr__(self):
         repr_str = "Extra(" + ", ".join(field_reprs) + f", album='{self.album}'"
 
         custom_field_reprs = []
-        for custom_field, value in self._custom_fields.items():
+        for custom_field, value in self.custom.items():
             custom_field_reprs.append(f"{custom_field}={value}")
         if custom_field_reprs:
             repr_str += ", custom_fields=[" + ", ".join(custom_field_reprs) + "]"
@@ -192,11 +196,3 @@ def __repr__(self):
     def __str__(self):
         """String representation of an Extra."""
         return f"{self.album}: {self.rel_path}"
-
-    def _get_default_custom_fields(self) -> dict[str, Any]:
-        """Returns the default custom extra fields."""
-        return {
-            field: default_val
-            for plugin_fields in config.CONFIG.pm.hook.create_custom_extra_fields()
-            for field, default_val in plugin_fields.items()
-        }