refactor: improve typing of RESET (#1100)

Author: leafty

components/renku_data_services/base_models/core.py

@@ -6,8 +6,8 @@
 import unicodedata
 from dataclasses import dataclass, field
 from datetime import datetime
-from enum import Enum, StrEnum
-from typing import ClassVar, Never, NewType, Optional, Protocol, Self, TypeVar, overload
+from enum import Enum, StrEnum, auto
+from typing import ClassVar, Never, Optional, Protocol, Self, TypeVar, overload
 
 from sanic import Request
 
@@ -400,13 +400,31 @@ async def authenticate(self, access_token: str, request: Request) -> AnyAPIUser:
         ...
 
 
-ResetType = NewType("ResetType", object)
-"""This type represents that a value that may be None should be reset back to None or null.
-This type should have only one instance, defined in the same file as this type.
-"""
+class ResetType(Enum):
+    """This type represents that a value that may be None should be reset back to None.
 
-RESET: ResetType = ResetType(object())
-"""The single instance of the ResetType, can be compared to similar to None, i.e. `if value is RESET`"""
+    This type has a single instance, 'RESET', defined in the same file as this type.
+
+    Discussion about implementation: There is a in-progress PEP which once accepted would
+    make it simpler to define 'ResetType' and 'RESET'. Until that change is adopted,
+    using an enum with a single value comes with clear type-checking advantages.
+
+    See:
+    * PEP 661 - Sentinel Values: https://peps.python.org/pep-0661/
+    * Discussion about PEP 661: https://discuss.python.org/t/pep-661-sentinel-values/9126
+    """
+
+    Reset = auto()
+
+    def __repr__(self) -> str:
+        return "RESET"
+
+    def __str__(self) -> str:
+        return "RESET"
+
+
+RESET = ResetType.Reset
+"""The single instance of the ResetType, can be compared to similar to None, i.e. `if value is RESET`."""
 
 
 class ResourceType(StrEnum):

components/renku_data_services/crc/core.py

@@ -260,7 +260,7 @@ def validate_resource_pool_update(existing: models.ResourcePool, update: models.
     quota: models.Quota | models.UnsavedQuota | ResetType = existing.quota if existing.quota else RESET
     if update.quota is RESET:
         quota = RESET
-    elif isinstance(update.quota, models.QuotaPatch) and existing.quota is None:
+    elif update.quota is not None and existing.quota is None:
         # The quota patch needs to contain all required fields
         cpu = update.quota.cpu
         if cpu is None:
@@ -294,7 +294,7 @@ def validate_resource_pool_update(existing: models.ResourcePool, update: models.
     remote: models.RemoteConfigurationFirecrest | ResetType = existing.remote if existing.remote else RESET
     if update.remote is RESET:
         remote = RESET
-    elif isinstance(update.remote, models.RemoteConfigurationFirecrestPatch) and existing.remote is None:
+    elif update.remote is not None and existing.remote is None:
         # The remote patch needs to contain all required fields
         kind = update.remote.kind
         if kind is None:

components/renku_data_services/crc/db.py

@@ -426,7 +426,7 @@ async def update_resource_pool(
 
             if update.cluster_id is RESET:
                 rp.cluster_id = None
-            elif isinstance(update.cluster_id, ULID):
+            elif update.cluster_id is not None:
                 cluster = await self.__cluster_repo.select(update.cluster_id)
                 rp.cluster_id = cluster.id
 
@@ -471,7 +471,7 @@ async def update_resource_pool(
             if update.remote is RESET:
                 rp.remote_provider_id = None
                 rp.remote_json = None
-            elif isinstance(update.remote, models.RemoteConfigurationFirecrestPatch):
+            elif update.remote is not None:
                 rp.remote_provider_id = (
                     update.remote.provider_id if update.remote.provider_id is not None else rp.remote_provider_id
                 )

components/renku_data_services/project/db.py

@@ -7,7 +7,6 @@
 import string
 from collections.abc import AsyncGenerator, Awaitable, Callable
 from datetime import UTC, datetime
-from pathlib import PurePosixPath
 from typing import Concatenate, ParamSpec, TypeVar
 
 from cryptography.hazmat.primitives.asymmetric import rsa
@@ -368,9 +367,9 @@ async def update_project(
             project.template_id = None
         if patch.is_template is not None:
             project.is_template = patch.is_template
-        if patch.secrets_mount_directory is not None and patch.secrets_mount_directory is RESET:
+        if patch.secrets_mount_directory is RESET:
             project.secrets_mount_directory = constants.DEFAULT_SESSION_SECRETS_MOUNT_DIR
-        elif patch.secrets_mount_directory is not None and isinstance(patch.secrets_mount_directory, PurePosixPath):
+        elif patch.secrets_mount_directory is not None:
             project.secrets_mount_directory = patch.secrets_mount_directory
 
         await session.flush()

components/renku_data_services/session/db.py

@@ -5,7 +5,6 @@
 from collections.abc import Callable
 from contextlib import AbstractAsyncContextManager, nullcontext
 from datetime import UTC, datetime
-from pathlib import PurePosixPath
 from typing import TYPE_CHECKING
 
 from sqlalchemy import select
@@ -230,25 +229,25 @@ def __update_environment(
             environment.default_url = update.default_url
         if update.port is not None:
             environment.port = update.port
-        if update.working_directory is not None and update.working_directory is RESET:
+        if update.working_directory is RESET:
             environment.working_directory = None
-        elif update.working_directory is not None and isinstance(update.working_directory, PurePosixPath):
+        elif update.working_directory is not None:
             environment.working_directory = update.working_directory
-        if update.mount_directory is not None and update.mount_directory is RESET:
+        if update.mount_directory is RESET:
             environment.mount_directory = None
-        elif update.mount_directory is not None and isinstance(update.mount_directory, PurePosixPath):
+        elif update.mount_directory is not None:
             environment.mount_directory = update.mount_directory
         if update.uid is not None:
             environment.uid = update.uid
         if update.gid is not None:
             environment.gid = update.gid
         if update.args is RESET:
             environment.args = None
-        elif isinstance(update.args, list):
+        elif update.args is not None:
             environment.args = update.args
         if update.command is RESET:
             environment.command = None
-        elif isinstance(update.command, list):
+        elif update.command is not None:
             environment.command = update.command
         if update.is_archived is not None:
             environment.is_archived = update.is_archived