fix: ensure to_model called with update operation for all data types (#575)

Fixes #555

This PR fixes a bug in the Service layer's `update()` method where dict, Pydantic, msgspec, and attrs data bypassed the `to_model()` operation map, preventing custom `to_model()` implementations from being invoked during update operations.

Author: cofin

advanced_alchemy/service/_async.py

@@ -10,6 +10,7 @@
 from typing import Any, ClassVar, Generic, Optional, Union, cast
 
 from sqlalchemy import Select
+from sqlalchemy import inspect as sa_inspect
 from sqlalchemy.ext.asyncio import AsyncSession
 from sqlalchemy.ext.asyncio.scoping import async_scoped_session
 from sqlalchemy.orm import InstrumentedAttribute
@@ -38,7 +39,6 @@
     is_dto_data,
     is_msgspec_struct,
     is_pydantic_model,
-    schema_dump,
 )
 from advanced_alchemy.utils.dataclass import Empty, EmptyType
 
@@ -738,38 +738,42 @@ async def update(
         Returns:
             Updated representation.
         """
-        if (
-            is_dict(data) or is_pydantic_model(data) or is_msgspec_struct(data) or is_attrs_instance(data)
-        ) and item_id is not None:
+        # ALWAYS convert data through to_model first to ensure operation hooks are called
+        # This ensures custom to_model() implementations receive the operation="update" parameter
+        # and that to_model_on_update() is properly invoked via the operation_map
+        data = await self.to_model(data, "update")
+
+        if item_id is not None:
+            # When item_id is provided, update existing instance rather than replacing it
+            # This preserves relationships and database-managed fields
             existing_instance = await self.repository.get(
                 item_id, id_attribute=id_attribute, load=load, execution_options=execution_options
             )
-            update_data = (
-                await self.to_model_on_update(data) if is_dict(data) else schema_dump(data, exclude_unset=True)
-            )
 
-            if is_dict(update_data):
-                for key, value in update_data.items():
-                    if getattr(existing_instance, key, MISSING) is not MISSING:
-                        setattr(existing_instance, key, value)
+            # Extract attributes from converted model to update existing instance
+            # Only copy attributes that were explicitly set (present in instance state)
+            instance_state = sa_inspect(data)  # pyright: ignore[reportOptionalMemberAccess]
+            for attr in instance_state.mapper.attrs:  # type: ignore[union-attr]  # pyright: ignore[reportOptionalMemberAccess]
+                # Check if attribute was explicitly set in the instance
+                if attr.key in instance_state.dict:  # type: ignore[union-attr]  # pyright: ignore[reportOptionalMemberAccess]
+                    value = getattr(data, attr.key, MISSING)
+                    if value is not MISSING and hasattr(existing_instance, attr.key):
+                        setattr(existing_instance, attr.key, value)
+
             data = existing_instance
-        else:
-            data = await self.to_model(data, "update")
-            if (
-                item_id is None
-                and self.repository.get_id_attribute_value(  # pyright: ignore[reportUnknownMemberType]
-                    item=data,
-                    id_attribute=id_attribute,
-                )
-                is None
-            ):
-                msg = (
-                    "Could not identify ID attribute value.  One of the following is required: "
-                    f"``item_id`` or ``data.{id_attribute or self.repository.id_attribute}``"
-                )
-                raise RepositoryError(msg)
-            if item_id is not None:
-                data = self.repository.set_id_attribute_value(item_id=item_id, item=data, id_attribute=id_attribute)  # pyright: ignore[reportUnknownMemberType]
+        elif (
+            self.repository.get_id_attribute_value(  # pyright: ignore[reportUnknownMemberType]
+                item=data,
+                id_attribute=id_attribute,
+            )
+            is None
+        ):
+            # No item_id provided and no ID on model - error
+            msg = (
+                "Could not identify ID attribute value.  One of the following is required: "
+                f"``item_id`` or ``data.{id_attribute or self.repository.id_attribute}``"
+            )
+            raise RepositoryError(msg)
         return cast(
             "ModelT",
             await self.repository.update(

advanced_alchemy/service/_sync.py

@@ -12,6 +12,7 @@
 from typing import Any, ClassVar, Generic, Optional, Union, cast
 
 from sqlalchemy import Select
+from sqlalchemy import inspect as sa_inspect
 from sqlalchemy.orm import InstrumentedAttribute, Session
 from sqlalchemy.orm.scoping import scoped_session
 from sqlalchemy.sql import ColumnElement
@@ -37,7 +38,6 @@
     is_dto_data,
     is_msgspec_struct,
     is_pydantic_model,
-    schema_dump,
 )
 from advanced_alchemy.utils.dataclass import Empty, EmptyType
 
@@ -737,36 +737,42 @@ def update(
         Returns:
             Updated representation.
         """
-        if (
-            is_dict(data) or is_pydantic_model(data) or is_msgspec_struct(data) or is_attrs_instance(data)
-        ) and item_id is not None:
+        # ALWAYS convert data through to_model first to ensure operation hooks are called
+        # This ensures custom to_model() implementations receive the operation="update" parameter
+        # and that to_model_on_update() is properly invoked via the operation_map
+        data = self.to_model(data, "update")
+
+        if item_id is not None:
+            # When item_id is provided, update existing instance rather than replacing it
+            # This preserves relationships and database-managed fields
             existing_instance = self.repository.get(
                 item_id, id_attribute=id_attribute, load=load, execution_options=execution_options
             )
-            update_data = self.to_model_on_update(data) if is_dict(data) else schema_dump(data, exclude_unset=True)
 
-            if is_dict(update_data):
-                for key, value in update_data.items():
-                    if getattr(existing_instance, key, MISSING) is not MISSING:
-                        setattr(existing_instance, key, value)
+            # Extract attributes from converted model to update existing instance
+            # Only copy attributes that were explicitly set (present in instance state)
+            instance_state = sa_inspect(data)  # pyright: ignore[reportOptionalMemberAccess]
+            for attr in instance_state.mapper.attrs:  # type: ignore[union-attr]  # pyright: ignore[reportOptionalMemberAccess]
+                # Check if attribute was explicitly set in the instance
+                if attr.key in instance_state.dict:  # type: ignore[union-attr]  # pyright: ignore[reportOptionalMemberAccess]
+                    value = getattr(data, attr.key, MISSING)
+                    if value is not MISSING and hasattr(existing_instance, attr.key):
+                        setattr(existing_instance, attr.key, value)
+
             data = existing_instance
-        else:
-            data = self.to_model(data, "update")
-            if (
-                item_id is None
-                and self.repository.get_id_attribute_value(  # pyright: ignore[reportUnknownMemberType]
-                    item=data,
-                    id_attribute=id_attribute,
-                )
-                is None
-            ):
-                msg = (
-                    "Could not identify ID attribute value.  One of the following is required: "
-                    f"``item_id`` or ``data.{id_attribute or self.repository.id_attribute}``"
-                )
-                raise RepositoryError(msg)
-            if item_id is not None:
-                data = self.repository.set_id_attribute_value(item_id=item_id, item=data, id_attribute=id_attribute)  # pyright: ignore[reportUnknownMemberType]
+        elif (
+            self.repository.get_id_attribute_value(  # pyright: ignore[reportUnknownMemberType]
+                item=data,
+                id_attribute=id_attribute,
+            )
+            is None
+        ):
+            # No item_id provided and no ID on model - error
+            msg = (
+                "Could not identify ID attribute value.  One of the following is required: "
+                f"``item_id`` or ``data.{id_attribute or self.repository.id_attribute}``"
+            )
+            raise RepositoryError(msg)
         return cast(
             "ModelT",
             self.repository.update(

docs/guides/patterns/repository-service.md

@@ -33,27 +33,81 @@ class TagService(service.SQLAlchemyAsyncRepositoryService[m.Tag]):
 
 ## Service Hooks
 
+### Operation Flow
+
+All service CRUD operations follow a consistent pattern:
+
+```
+Operation (create/update/upsert/delete)
+    ↓
+to_model(data, operation="create"|"update"|"upsert"|"delete")
+    ↓
+operation_map routes to operation-specific hook:
+    ↓
+to_model_on_create()  # for create
+to_model_on_update()  # for update
+to_model_on_upsert()  # for upsert
+to_model_on_delete()  # for delete
+```
+
+This ensures custom `to_model()` implementations receive the operation parameter for all data types (dict, Pydantic, msgspec, attrs, model instances).
+
 ### Data Transformation Hooks
 
+**Operation-Specific Hooks** (recommended pattern):
+
 ```python
-# Called before create
+# Called before create (via to_model operation_map)
 async def to_model_on_create(self, data: service.ModelDictT[m.User]) -> service.ModelDictT[m.User]:
     data = service.schema_dump(data)
     # Hash passwords, generate slugs, set defaults
     return data
 
-# Called before update
+# Called before update (via to_model operation_map)
 async def to_model_on_update(self, data: service.ModelDictT[m.User]) -> service.ModelDictT[m.User]:
     data = service.schema_dump(data)
     # Update slugs, validate permissions
     return data
 
-# Called before upsert (insert or update based on match_fields)
+# Called before upsert (via to_model operation_map)
 async def to_model_on_upsert(self, data: service.ModelDictT[m.User]) -> service.ModelDictT[m.User]:
     data = service.schema_dump(data)
     return data
 ```
 
+**Custom to_model() with Operation Parameter** (for shared logic across operations):
+
+```python
+class UserService(service.SQLAlchemyAsyncRepositoryService[m.User]):
+    class Repo(repository.SQLAlchemyAsyncRepository[m.User]):
+        model_type = m.User
+
+    repository_type = Repo
+
+    async def to_model(
+        self,
+        data: service.ModelDictT[m.User],
+        operation: str | None = None,
+    ) -> m.User:
+        """Custom to_model with operation-aware logic."""
+        data = service.schema_dump(data)
+
+        # Shared preprocessing for all operations
+        if service.is_dict(data):
+            data["email"] = data.get("email", "").lower()
+
+        # Let parent call operation-specific hook via operation_map
+        return await super().to_model(data, operation)
+
+    async def to_model_on_update(self, data: service.ModelDictT[m.User]) -> service.ModelDictT[m.User]:
+        """Update-specific logic (called after to_model preprocessing)."""
+        data = service.schema_dump(data)
+        # Only update slug if name changed
+        if "name" in data and "slug" not in data:
+            data["slug"] = await self.repository.get_available_slug(data["name"])
+        return data
+```
+
 ## Service Patterns
 
 ### Password Hashing