MAINT: Address feedbacks

Author: GibranAlfa

ARCHITECTURE.md

@@ -165,9 +165,9 @@ model declares its current schema version, and each resource manager has a
 `MigrationManager` with the migration registry for that model.
 
 Migration is forward-only. Data without `schema_version` is treated as version 1.
-Each registered function increments the schema version by one. The manager rejects a
-newer schema, a missing migration step, an invalid version, or data that does not
-validate as the current model.
+Each registered migration function increments the schema version by one. The manager
+rejects a newer schema, a missing migration step, an invalid version, or data that
+does not validate as the current model.
 
 The migration boundary depends on the resource operation:
 
@@ -192,8 +192,9 @@ The migration boundary depends on the resource operation:
 There is no backward migration. After a current-schema resource is saved, an older
 `fmu-settings` release can reject it as newer than supported. If the pre-migration
 cache revision is still retained, restore it with the older release. Migration
-backups are not loaded or restored by the library. Users must copy one back manually
-when the cache revision is no longer available.
+backups are not loaded or restored by the library. If the cache revision is no longer
+available, we should help users copy the appropriate migration backup back to the
+resource file before running the older release.
 
 See the
 [schema migration guide](src/fmu/settings/_migrations/README.md)

src/fmu/settings/_migrations/README.md

@@ -146,9 +146,9 @@ The returned data must:
 
 - Preserve all relevant stored values.
 - Set `schema_version` to the next version.
-- Be valid input for the next migration or the current model.
+- Be valid input for the next migration function or the current model.
 
-### 3. Register the migration
+### 3. Register the migration function
 
 Update `project_config/__init__.py`:
 
@@ -162,10 +162,10 @@ PROJECT_CONFIG_MIGRATIONS: dict[int, MigrationFunction] = {
 }
 ```
 
-The registry key is the source schema version. Key `1` registers the migration from
-version 1 to version 2.
+The registry key is the source schema version. Key `1` registers the migration
+function from version 1 to version 2.
 
-Keep every migration when later versions are added:
+Keep every migration function when later versions are added:
 
 ```python
 PROJECT_CONFIG_MIGRATIONS: dict[int, MigrationFunction] = {
@@ -185,7 +185,7 @@ When you add a migration:
 - Add resource-specific tests under `tests/test_migrations/`. For example, a
   `ProjectConfig` migration can use `test_project_config_migration.py`.
 
-The resource-specific tests must cover conversion, load, save, cache restore, and
+The resource-specific tests must cover migration, load, save, cache restore, and
 invalid data. Update the complete current version fixture used by
 `tests/test_resources/test_migratable_models_up_to_date.py`. Keep the previous
 version input with the resource-specific migration tests.
@@ -205,7 +205,7 @@ uv run mypy src tests
 Migration is automatic during normal use:
 
 1. The resource manager reads stored data from an older schema.
-2. The migration manager converts it in memory.
+2. The migration manager migrates it in memory.
 3. The resource manager returns the current validated model.
 4. The stored file remains unchanged until a save occurs.
 
@@ -223,21 +223,27 @@ On the first save:
 Loading a resource does not create a changelog entry. A later user update or
 restore uses the existing changelog behavior.
 
+### Cache restore
+
 When an old cache revision is restored by the current release, it is migrated before
 it is written. The resource file therefore uses the current schema after the restore.
-If you roll back to an older release while the pre-migration revision is retained,
-restore that revision with the older release to return the resource file to the older
-schema. Migration backups are not read or restored by the library, copy one back
-manually when the cache revision is no longer available.
+
+### Rollback to an older release
 
 Migrations are forward-only. After current-schema data is saved, an older
-`fmu-settings` release can reject it as newer than its supported schema.
+`fmu-settings` release can reject it as newer than its supported schema. If the
+pre-migration revision is retained, restore that revision with the older release to
+return the resource file to the older schema. Migration backups are not read or
+restored by the library. If the cache revision is no longer available, we should help
+users copy the appropriate migration backup back to the resource file before running
+the older release.
 
 ## Release checklist for an `fmu-settings` schema version
 
-Use this checklist when releasing an `fmu-settings` package that contains a new
-stored schema version. First prepare and publish `fmu-settings`. Then update the
-downstream applications so that users receive the new package.
+Use this checklist when releasing an `fmu-settings` package with an updated
+schema version in one of the migratable models. First prepare and publish
+`fmu-settings`. Then update the downstream applications so that users receive
+the new package.
 
 ### 1. Prepare the `fmu-settings` package
 

src/fmu/settings/_migrations/manager.py

@@ -60,7 +60,7 @@ def migrate_resource(self, data: Any) -> MigratableResource:
         """
         if not isinstance(data, dict):
             raise MigrationError(
-                f"{self.model_class.__name__} resource must be a JSON object"
+                f"Stored {self.model_class.__name__} data must be a JSON object"
             )
         source_schema_version = self._get_source_schema_version(data)
         migration_steps = self._get_migration_steps(source_schema_version)
@@ -71,37 +71,37 @@ def migrate_resource(self, data: Any) -> MigratableResource:
                 migrated_data = migration_function(migrated_data)
             except Exception as e:
                 raise MigrationError(
-                    f"{self.model_class.__name__} migration from schema version "
-                    f"{version} to {version + 1} failed"
+                    f"Failed to migrate stored {self.model_class.__name__} data "
+                    f"from schema version {version} to {version + 1}"
                 ) from e
 
             result_version = self._validate_version(migrated_data.get("schema_version"))
             expected_version = version + 1
             if result_version != expected_version:
                 raise MigrationError(
-                    f"{self.model_class.__name__} migration from schema version "
-                    f"{version} must set schema_version to {expected_version}, "
-                    f"but set it to {result_version}"
+                    f"Migration of stored {self.model_class.__name__} data from "
+                    f"schema version {version} must set schema_version to "
+                    f"{expected_version}, but set it to {result_version}"
                 )
 
         try:
             validated_model = self.model_class.model_validate(migrated_data)
         except ValidationError as e:
             raise MigrationError(
-                f"{self.model_class.__name__} data does not validate against current "
-                f"schema version {self.current_version}"
+                f"Stored {self.model_class.__name__} data does not validate against "
+                f"current schema version {self.current_version}"
             ) from e
 
         return validated_model
 
     def requires_migration(self, data: dict[str, Any]) -> bool:
-        """Return whether stored data needs migration before a write.
+        """Return whether stored data needs migration.
 
         This check verifies that all required forward migration functions exist and
         that the stored schema version is older than the current schema version.
 
         Args:
-            data: Existing stored data that a write would replace.
+            data: Existing stored data to inspect.
 
         Returns:
             Whether the stored data requires migration.
@@ -117,11 +117,11 @@ def requires_migration(self, data: dict[str, Any]) -> bool:
     def _get_migration_steps(
         self, source_schema_version: int
     ) -> list[tuple[int, MigrationFunction]]:
-        """Return and validate migration steps needed by a source schema version."""
+        """Return migration functions from source to current schema version."""
         if source_schema_version > self.current_version:
             raise MigrationError(
-                f"{self.model_class.__name__} schema version {source_schema_version} "
-                "is newer than supported version "
+                f"Stored {self.model_class.__name__} data has schema version "
+                f"{source_schema_version}, which is newer than supported version "
                 f"{self.current_version}; downgrade migration "
                 "is not supported"
             )
@@ -131,9 +131,8 @@ def _get_migration_steps(
             migration_function = self.migrations.get(version)
             if migration_function is None:
                 raise MigrationError(
-                    f"Missing {self.model_class.__name__} migration function from "
-                    "schema "
-                    f"version {version} to {version + 1}"
+                    f"Missing {self.model_class.__name__} migration function for "
+                    f"stored data from schema version {version} to {version + 1}"
                 )
             steps.append((version, migration_function))
         return steps

src/fmu/settings/_resources/cache_manager.py

@@ -172,11 +172,11 @@ def get_revision_content(
         content_str = self._fmu_dir.read_text_file(cache_relative)
 
         try:
-            return self._migrate_and_validate_content(
-                content_str,
-                model_class,
-                migration_manager,
-            )
+            if migration_manager is not None and migration_manager.requires_migration(
+                json.loads(content_str)
+            ):
+                return migration_manager.migrate_resource(json.loads(content_str))
+            return model_class.model_validate_json(content_str)
         except MigrationError as e:
             raise MigrationError(
                 f"Cannot migrate cached content for '{resource_file_path}': {e}"
@@ -227,11 +227,15 @@ def restore_revision(
             current_content = self._fmu_dir.read_text_file(resource_file_path)
 
             try:
-                self._migrate_and_validate_content(
-                    current_content,
-                    model_class,
-                    migration_manager,
-                )
+                if (
+                    migration_manager is not None
+                    and migration_manager.requires_migration(
+                        json.loads(current_content)
+                    )
+                ):
+                    migration_manager.migrate_resource(json.loads(current_content))
+                else:
+                    model_class.model_validate_json(current_content)
             except (json.JSONDecodeError, MigrationError, ValidationError) as e:
                 logger.warning(
                     "Skipped caching current state of "
@@ -246,20 +250,6 @@ def restore_revision(
 
         logger.info(f"Restored {resource_file_path} from cache revision {revision_id}")
 
-    @staticmethod
-    def _migrate_and_validate_content(
-        content: str,
-        model_class: type[RequestedModel],
-        migration_manager: MigrationManager[RequestedModel] | None,
-    ) -> RequestedModel:
-        """Migrate versioned content and validate it, or validate current content."""
-        if migration_manager is None:
-            return model_class.model_validate_json(content)
-        if migration_manager.model_class is not model_class:
-            raise TypeError("Migration manager model must match the requested model")
-
-        return migration_manager.migrate_resource(json.loads(content))
-
     def _ensure_resource_cache_dir(self: Self, resource_file_path: Path) -> Path:
         """Create (if needed) and return the cache directory for resource file."""
         self._cache_root_path(create=True)

src/fmu/settings/_resources/pydantic_resource_manager.py

@@ -145,7 +145,13 @@ def load(
             try:
                 content = self.fmu_dir.read_text_file(self.relative_path)
                 data = json.loads(content)
-                validated_model = self._migrate_and_validate_data(data)
+                if (
+                    self.migration_manager is not None
+                    and self.migration_manager.requires_migration(data)
+                ):
+                    validated_model = self.migration_manager.migrate_resource(data)
+                else:
+                    validated_model = self.model_class.model_validate(data)
                 if store_cache:
                     self._cache = validated_model
                 else:
@@ -184,11 +190,11 @@ def save(
         self._cache = model
 
     def _store_migration_backup(self: Self) -> None:
-        """Try to back up older data before saving the migrated version.
+        """Try to back up older stored data.
 
         The backup is best effort. Invalid JSON and non-object content are skipped.
-        File-system errors while writing the backup are logged, but do not stop the
-        save.
+        File-system errors while writing the backup are logged without interrupting
+        the caller.
         """
         migration_manager = self.migration_manager
         if migration_manager is None:
@@ -214,12 +220,14 @@ def _store_migration_backup(self: Self) -> None:
         if not requires_migration:
             return
 
-        source_version = data.get("schema_version", 1)
-        self._write_migration_backup(content, source_version)
+        source_schema_version = data.get("schema_version", 1)
+        self._write_migration_backup(content, source_schema_version)
         if self.automatic_caching:
             self.fmu_dir.cache.store_revision(self.relative_path, content)
 
-    def _write_migration_backup(self: Self, content: str, source_version: int) -> None:
+    def _write_migration_backup(
+        self: Self, content: str, source_schema_version: int
+    ) -> None:
         """Write a best-effort migration backup of the original content.
 
         For example, a ``config.json`` backup can be stored under
@@ -234,7 +242,7 @@ def _write_migration_backup(self: Self, content: str, source_version: int) -> No
             / self.relative_path.stem
         )
         backup_filename = (
-            f"{timestamp}-{token}-{self.model_class.__name__}-v{source_version}"
+            f"{timestamp}-{token}-{self.model_class.__name__}-v{source_schema_version}"
             f"{self.relative_path.suffix}"
         )
         backup_path = backup_directory / backup_filename
@@ -248,18 +256,6 @@ def _write_migration_backup(self: Self, content: str, source_version: int) -> No
                 f"Continuing without it: {e}"
             )
 
-    def _migrate_and_validate_data(self: Self, data: Any) -> PydanticResource:
-        """Migrate decoded data with registered functions and validate the result."""
-        if self.migration_manager is None:
-            return self.model_class.model_validate(data)
-        try:
-            return self.migration_manager.migrate_resource(data)
-        except MigrationError as e:
-            raise MigrationError(
-                f"Failed to migrate resource file for "
-                f"'{self.__class__.__name__}' at '{self.path}': {e}"
-            ) from e
-
     def get_model_diff(
         self: Self,
         current_model: BaseModel,

tests/test_migrations/test_migration_manager.py

@@ -117,7 +117,10 @@ def test_migration_manager_rejects_missing_step() -> None:
 
     with pytest.raises(
         MigrationError,
-        match="Missing VersionThreeModel migration function from schema version 1 to 2",
+        match=(
+            "Missing VersionThreeModel migration function for stored data from "
+            "schema version 1 to 2"
+        ),
     ):
         manager.migrate_resource(
             {
@@ -134,7 +137,10 @@ def test_migration_manager_rejects_newer_schema() -> None:
 
     with pytest.raises(
         MigrationError,
-        match=("VersionThreeModel schema version 4 is newer than supported version 3"),
+        match=(
+            "Stored VersionThreeModel data has schema version 4, which is newer "
+            "than supported version 3"
+        ),
     ):
         manager.migrate_resource(
             {
@@ -206,7 +212,9 @@ def failing_migration(data: dict[str, Any]) -> dict[str, Any]:
 
     with pytest.raises(
         MigrationError,
-        match=("VersionThreeModel migration from schema version 1 to 2 failed"),
+        match=(
+            "Failed to migrate stored VersionThreeModel data from schema version 1 to 2"
+        ),
     ) as error:
         manager.migrate_resource(
             {
@@ -237,7 +245,8 @@ def remove_required_value(data: dict[str, Any]) -> dict[str, Any]:
     with pytest.raises(
         MigrationError,
         match=(
-            "VersionThreeModel data does not validate against current schema version 3"
+            "Stored VersionThreeModel data does not validate against current schema "
+            "version 3"
         ),
     ):
         manager.migrate_resource(