Structured errors 2: more error types (#24)

* Structured errors 2: more error types

* PR fixes

* fix tests

---------

Co-authored-by: Tim Pellissier <tpellissier@microsoft.com>

Author: tpellissier-msft

src/dataverse_sdk/error_codes.py

@@ -26,3 +26,46 @@
     HTTP_503,
     HTTP_504,
 }
+
+# Validation subcodes
+VALIDATION_SQL_NOT_STRING = "validation_sql_not_string"
+VALIDATION_SQL_EMPTY = "validation_sql_empty"
+VALIDATION_ENUM_NO_MEMBERS = "validation_enum_no_members"
+VALIDATION_ENUM_NON_INT_VALUE = "validation_enum_non_int_value"
+VALIDATION_UNSUPPORTED_COLUMN_TYPE = "validation_unsupported_column_type"
+VALIDATION_UNSUPPORTED_CACHE_KIND = "validation_unsupported_cache_kind"
+
+# SQL parse subcodes
+SQL_PARSE_TABLE_NOT_FOUND = "sql_parse_table_not_found"
+
+# Metadata subcodes
+METADATA_ENTITYSET_NOT_FOUND = "metadata_entityset_not_found"
+METADATA_ENTITYSET_NAME_MISSING = "metadata_entityset_name_missing"
+METADATA_TABLE_NOT_FOUND = "metadata_table_not_found"
+METADATA_TABLE_ALREADY_EXISTS = "metadata_table_already_exists"
+METADATA_ATTRIBUTE_RETRY_EXHAUSTED = "metadata_attribute_retry_exhausted"
+METADATA_PICKLIST_RETRY_EXHAUSTED = "metadata_picklist_retry_exhausted"
+
+# Mapping from status code -> subcode
+HTTP_STATUS_TO_SUBCODE: dict[int, str] = {
+    400: HTTP_400,
+    401: HTTP_401,
+    403: HTTP_403,
+    404: HTTP_404,
+    409: HTTP_409,
+    412: HTTP_412,
+    415: HTTP_415,
+    429: HTTP_429,
+    500: HTTP_500,
+    502: HTTP_502,
+    503: HTTP_503,
+    504: HTTP_504,
+}
+
+TRANSIENT_STATUS = {429, 502, 503, 504}
+
+def http_subcode(status: int) -> str:
+    return HTTP_STATUS_TO_SUBCODE.get(status, f"http_{status}")
+
+def is_transient_status(status: int) -> bool:
+    return status in TRANSIENT_STATUS

src/dataverse_sdk/errors.py

@@ -12,16 +12,16 @@ def __init__(
         subcode: Optional[str] = None,
         status_code: Optional[int] = None,
         details: Optional[Dict[str, Any]] = None,
-        source: Optional[Dict[str, Any]] = None,
-        is_transient: Optional[bool] = None,
+        source: Optional[str] = None,
+        is_transient: bool = False,
     ) -> None:
         super().__init__(message)
         self.message = message
         self.code = code
         self.subcode = subcode
         self.status_code = status_code
         self.details = details or {}
-        self.source = source or {}
+        self.source = source or "client"
         self.is_transient = is_transient
         self.timestamp = _dt.datetime.utcnow().isoformat() + "Z"
 
@@ -40,25 +40,54 @@ def to_dict(self) -> Dict[str, Any]:
     def __repr__(self) -> str:  # pragma: no cover
         return f"{self.__class__.__name__}(code={self.code!r}, subcode={self.subcode!r}, message={self.message!r})"
 
+class ValidationError(DataverseError):
+    def __init__(self, message: str, *, subcode: Optional[str] = None, details: Optional[Dict[str, Any]] = None):
+        super().__init__(message, code="validation_error", subcode=subcode, details=details, source="client")
+
+class MetadataError(DataverseError):
+    def __init__(self, message: str, *, subcode: Optional[str] = None, details: Optional[Dict[str, Any]] = None):
+        super().__init__(message, code="metadata_error", subcode=subcode, details=details, source="client")
+
+class SQLParseError(DataverseError):
+    def __init__(self, message: str, *, subcode: Optional[str] = None, details: Optional[Dict[str, Any]] = None):
+        super().__init__(message, code="sql_parse_error", subcode=subcode, details=details, source="client")
+
 class HttpError(DataverseError):
     def __init__(
         self,
         message: str,
-        *,
+        status_code: int,
+        is_transient: bool = False,
         subcode: Optional[str] = None,
-        status_code: Optional[int] = None,
-        details: Optional[Dict[str, Any]] = None,
-        source: Optional[Dict[str, Any]] = None,
-        is_transient: Optional[bool] = None,
+        service_error_code: Optional[str] = None,
+        correlation_id: Optional[str] = None,
+        request_id: Optional[str] = None,
+        traceparent: Optional[str] = None,
+        body_excerpt: Optional[str] = None,
+        retry_after: Optional[int] = None,
+        details: Optional[Dict[str, Any]] = None
     ) -> None:
+        d = details or {}
+        if service_error_code is not None:
+            d["service_error_code"] = service_error_code
+        if correlation_id is not None:
+            d["correlation_id"] = correlation_id
+        if request_id is not None:
+            d["request_id"] = request_id
+        if traceparent is not None:
+            d["traceparent"] = traceparent
+        if body_excerpt is not None:
+            d["body_excerpt"] = body_excerpt
+        if retry_after is not None:
+            d["retry_after"] = retry_after
         super().__init__(
             message,
-            code="http",
+            code="http_error",
             subcode=subcode,
             status_code=status_code,
-            details=details,
-            source=source,
+            details=d,
+            source="server",
             is_transient=is_transient,
         )
 
-__all__ = ["DataverseError", "HttpError"]
+__all__ = ["DataverseError", "HttpError", "ValidationError", "MetadataError", "SQLParseError"]

src/dataverse_sdk/odata.py

@@ -10,7 +10,7 @@
 
 from .http import HttpClient
 from .odata_upload_files import ODataFileUpload
-from .errors import HttpError
+from .errors import *
 from . import error_codes as ec
 
 
@@ -76,45 +76,53 @@ def _raw_request(self, method: str, url: str, **kwargs):
         return self._http.request(method, url, **kwargs)
 
     def _request(self, method: str, url: str, *, expected: tuple[int, ...] = (200, 201, 202, 204), **kwargs):
-        """Execute HTTP request; raise HttpError with structured details on failure.
-
-        Returns the raw response for success codes; raises HttpError with extracted
-        Dataverse error payload fields and correlation identifiers otherwise.
-        """
-        headers = kwargs.pop("headers", None)
-        kwargs["headers"] = self._merge_headers(headers)
+        headers_in = kwargs.pop("headers", None)
+        kwargs["headers"] = self._merge_headers(headers_in)
         r = self._raw_request(method, url, **kwargs)
         if r.status_code in expected:
             return r
-        payload = {}
+        headers = getattr(r, "headers", {}) or {}
+        body_excerpt = (getattr(r, "text", "") or "")[:200]
+        svc_code = None
+        msg = f"HTTP {r.status_code}"
         try:
-            payload = r.json() if getattr(r, 'text', None) else {}
+            data = r.json() if getattr(r, "text", None) else {}
+            if isinstance(data, dict):
+                inner = data.get("error")
+                if isinstance(inner, dict):
+                    svc_code = inner.get("code")
+                    imsg = inner.get("message")
+                    if isinstance(imsg, str) and imsg.strip():
+                        msg = imsg.strip()
+                else:
+                    imsg2 = data.get("message")
+                    if isinstance(imsg2, str) and imsg2.strip():
+                        msg = imsg2.strip()
         except Exception:
-            payload = {}
-        svc_err = payload.get("error") if isinstance(payload, dict) else None
-        svc_code = svc_err.get("code") if isinstance(svc_err, dict) else None
-        svc_msg = svc_err.get("message") if isinstance(svc_err, dict) else None
-        message = svc_msg or f"HTTP {r.status_code}"
-        subcode = f"http_{r.status_code}"
-
-        headers = getattr(r, 'headers', {}) or {}
-        details = {
-            "service_error_code": svc_code,
-            "body_excerpt": (getattr(r, 'text', '') or '')[:200],
-            "correlation_id": headers.get("x-ms-correlation-request-id") or headers.get("x-ms-correlation-id"),
-            "request_id": headers.get("x-ms-client-request-id") or headers.get("request-id"),
-            "traceparent": headers.get("traceparent"),
-        }
+            pass
+        sc = r.status_code
+        subcode = ec.http_subcode(sc)
+        correlation_id = headers.get("x-ms-correlation-request-id") or headers.get("x-ms-correlation-id")
+        request_id = headers.get("x-ms-client-request-id") or headers.get("request-id") or headers.get("x-ms-request-id")
+        traceparent = headers.get("traceparent")
         ra = headers.get("Retry-After")
+        retry_after = None
         if ra:
-            details["retry_after"] = ra
-        is_transient = r.status_code in (429, 502, 503, 504)
+            try:
+                retry_after = int(ra)
+            except Exception:
+                retry_after = None
+        is_transient = ec.is_transient_status(sc)
         raise HttpError(
-            message,
+            msg,
+            status_code=sc,
             subcode=subcode,
-            status_code=r.status_code,
-            details=details,
-            source={"method": method, "url": url},
+            service_error_code=svc_code,
+            correlation_id=correlation_id,
+            request_id=request_id,
+            traceparent=traceparent,
+            body_excerpt=body_excerpt,
+            retry_after=retry_after,
             is_transient=is_transient,
         )
 
@@ -500,8 +508,10 @@ def _query_sql(self, sql: str) -> list[dict[str, Any]]:
         RuntimeError
             If metadata lookup for the logical name fails.
         """
-        if not isinstance(sql, str) or not sql.strip():
-            raise ValueError("sql must be a non-empty string")
+        if not isinstance(sql, str):
+            raise ValidationError("sql must be a string", subcode=ec.VALIDATION_SQL_NOT_STRING)
+        if not sql.strip():
+            raise ValidationError("sql must be a non-empty string", subcode=ec.VALIDATION_SQL_EMPTY)
         sql = sql.strip()
 
         # Extract logical table name via helper (robust to identifiers ending with 'from')
@@ -570,11 +580,17 @@ def _entity_set_from_logical(self, logical: str) -> str:
             items = []
         if not items:
             plural_hint = " (did you pass a plural entity set name instead of the singular logical name?)" if logical.endswith("s") and not logical.endswith("ss") else ""
-            raise RuntimeError(f"Unable to resolve entity set for logical name '{logical}'. Provide the singular logical name.{plural_hint}")
+            raise MetadataError(
+                f"Unable to resolve entity set for logical name '{logical}'. Provide the singular logical name.{plural_hint}",
+                subcode=ec.METADATA_ENTITYSET_NOT_FOUND,
+            )
         md = items[0]
         es = md.get("EntitySetName")
         if not es:
-            raise RuntimeError(f"Metadata response missing EntitySetName for logical '{logical}'.")
+            raise MetadataError(
+                f"Metadata response missing EntitySetName for logical '{logical}'.",
+                subcode=ec.METADATA_ENTITYSET_NAME_MISSING,
+            )
         self._logical_to_entityset_cache[logical] = es
         primary_id_attr = md.get("PrimaryIdAttribute")
         if isinstance(primary_id_attr, str) and primary_id_attr:
@@ -1014,7 +1030,10 @@ def _delete_table(self, tablename: str) -> None:
         entity_schema = schema_name
         ent = self._get_entity_by_schema(entity_schema)
         if not ent or not ent.get("MetadataId"):
-            raise RuntimeError(f"Table '{entity_schema}' not found.")
+            raise MetadataError(
+                f"Table '{entity_schema}' not found.",
+                subcode=ec.METADATA_TABLE_NOT_FOUND,
+            )
         metadata_id = ent["MetadataId"]
         url = f"{self.api}/EntityDefinitions({metadata_id})"
         r = self._request("delete", url)
@@ -1026,7 +1045,10 @@ def _create_table(self, tablename: str, schema: Dict[str, Any]) -> Dict[str, Any
 
         ent = self._get_entity_by_schema(entity_schema)
         if ent:
-            raise RuntimeError(f"Table '{entity_schema}' already exists. No update performed.")
+            raise MetadataError(
+                f"Table '{entity_schema}' already exists.",
+                subcode=ec.METADATA_TABLE_ALREADY_EXISTS,
+            )
 
         created_cols: List[str] = []
         primary_attr_schema = "new_Name" if "_" not in entity_schema else f"{entity_schema.split('_',1)[0]}_Name"
@@ -1080,7 +1102,10 @@ def _flush_cache(
         """
         k = (kind or "").strip().lower()
         if k != "picklist":
-            raise ValueError(f"Unsupported cache kind '{kind}' (only 'picklist' is implemented)")
+            raise ValidationError(
+                f"Unsupported cache kind '{kind}' (only 'picklist' is implemented)",
+                subcode=ec.VALIDATION_UNSUPPORTED_CACHE_KIND,
+            )
 
         removed = len(self._picklist_label_cache)
         self._picklist_label_cache.clear()