fix: address PR review - PUT write-timeout, column-not-found subcode, full-definition metadata PUT + MergeLabels, pre-validate update_columns

Author: arorashivam96

src/PowerPlatform/Dataverse/aio/core/_async_http.py

@@ -115,13 +115,13 @@ async def _request(self, method: str, url: str, **kwargs: Any) -> _AsyncResponse
             raise RuntimeError("No aiohttp.ClientSession set. Set _session before making requests.")
 
         # If no timeout is provided, use the user-specified default timeout if set;
-        # otherwise, apply per-method defaults (120s for POST/PATCH/DELETE, 10s for others).
+        # otherwise, apply per-method defaults (120s for POST/PUT/PATCH/DELETE, 10s for others).
         if "timeout" not in kwargs:
             if self.default_timeout is not None:
                 t = self.default_timeout
             else:
                 m = (method or "").lower()
-                t = _TIMEOUT_WRITE_METHODS if m in ("post", "patch", "delete") else _TIMEOUT_READ_METHODS
+                t = _TIMEOUT_WRITE_METHODS if m in ("post", "put", "patch", "delete") else _TIMEOUT_READ_METHODS
             kwargs["timeout"] = aiohttp.ClientTimeout(total=t)
 
         # Log outbound request once (before retry loop).

src/PowerPlatform/Dataverse/aio/data/_async_odata.py

@@ -44,7 +44,6 @@
     _USER_AGENT,
     _DEFAULT_EXPECTED_STATUSES,
     _RequestContext,
-    _COLUMN_OVERRIDE_KEYS,
     _TYPED_COLUMN_PROPERTIES,
 )
 
@@ -1046,12 +1045,14 @@ async def _update_attribute(
         column_name: str,
         overrides: Dict[str, Any],
     ) -> str:
-        """Update constraints on an existing column: GET typed attr -> PUT + @odata.type (#202)."""
-        if not isinstance(overrides, dict) or not overrides:
-            raise TypeError("overrides must be a non-empty dict of column constraints")
-        unknown = set(overrides) - (_COLUMN_OVERRIDE_KEYS - {"type"})
-        if unknown:
-            raise ValueError(f"Unknown column constraint override(s) for '{column_name}': {sorted(unknown)}")
+        """Update constraints on an existing column: retrieve the full attr -> PUT it back (#202).
+
+        Follows the documented column-update contract -- ``PUT`` the *entire*
+        current attribute definition so untouched properties are preserved -- and
+        sends ``MSCRM.MergeLabels: true`` so localized labels in other languages
+        survive.
+        """
+        self._validate_column_overrides(column_name, overrides)
         ent = await self._get_entity_by_table_schema_name(table_schema_name)
         if not ent or not ent.get("MetadataId"):
             raise MetadataError(
@@ -1068,26 +1069,27 @@ async def _update_attribute(
             if getattr(err, "status_code", None) == 404:
                 raise MetadataError(
                     f"Column '{column_name}' not found on table '{table_schema_name}'.",
-                    subcode=METADATA_TABLE_NOT_FOUND,
+                    subcode=METADATA_COLUMN_NOT_FOUND,
                 ) from err
             raise
         attr_metadata_id = existing.get("MetadataId")
         odata_type = str(existing.get("@odata.type", "")).lstrip("#")
         if not attr_metadata_id or not odata_type:
             raise MetadataError(
                 f"Column '{column_name}' not found on table '{table_schema_name}'.",
-                subcode=METADATA_TABLE_NOT_FOUND,
+                subcode=METADATA_COLUMN_NOT_FOUND,
             )
-        body: Dict[str, Any] = {
-            "@odata.type": odata_type,
-            "MetadataId": attr_metadata_id,
-            "SchemaName": existing.get("SchemaName", column_name),
-        }
+        full = (await self._request("get", f"{attr_url}/{odata_type}")).json()
+        body: Dict[str, Any] = {k: v for k, v in full.items() if not str(k).startswith("@odata.")}
+        body["@odata.type"] = odata_type
+        body["MetadataId"] = attr_metadata_id
+        body.setdefault("SchemaName", existing.get("SchemaName", column_name))
         self._apply_column_overrides(body, overrides)
         req = _RawRequest(
             method="PUT",
             url=f"{self.api}/EntityDefinitions({metadata_id})/Attributes({attr_metadata_id})",
             body=json.dumps(body, ensure_ascii=False),
+            headers={"MSCRM.MergeLabels": "true"},
         )
         await self._execute_raw(req)
         return column_name

src/PowerPlatform/Dataverse/aio/operations/async_tables.py

@@ -798,6 +798,10 @@ async def update_columns(
             raise TypeError("columns must be a non-empty dict of {column: overrides}")
         updated: List[str] = []
         async with self._client._scoped_odata() as od:
+            # Validate every spec up front so a bad entry can't leave the table
+            # partially modified by earlier successful updates.
+            for col, spec in columns.items():
+                od._validate_column_overrides(col, spec)
             for col, spec in columns.items():
                 await od._update_attribute(table, col, spec)
                 updated.append(col)

src/PowerPlatform/Dataverse/core/_http.py

@@ -76,14 +76,14 @@ def _request(self, method: str, url: str, **kwargs: Any) -> requests.Response:
         :raises requests.exceptions.RequestException: If all retry attempts fail.
         """
         # If no timeout is provided, use the user-specified default timeout if set;
-        # otherwise, apply per-method defaults (120s for POST/PATCH/DELETE, 10s for others).
+        # otherwise, apply per-method defaults (120s for POST/PUT/PATCH/DELETE, 10s for others).
         if "timeout" not in kwargs:
             if self.default_timeout is not None:
                 kwargs["timeout"] = self.default_timeout
             else:
                 m = (method or "").lower()
                 kwargs["timeout"] = (
-                    _TIMEOUT_WRITE_METHODS if m in ("post", "patch", "delete") else _TIMEOUT_READ_METHODS
+                    _TIMEOUT_WRITE_METHODS if m in ("post", "put", "patch", "delete") else _TIMEOUT_READ_METHODS
                 )
 
         # Log outbound request once (before retry loop).

src/PowerPlatform/Dataverse/data/_odata.py

@@ -45,7 +45,6 @@
     _USER_AGENT,
     _DEFAULT_EXPECTED_STATUSES,
     _RequestContext,
-    _COLUMN_OVERRIDE_KEYS,
     _TYPED_COLUMN_PROPERTIES,
 )
 
@@ -1031,18 +1030,17 @@ def _update_attribute(
         column_name: str,
         overrides: Dict[str, Any],
     ) -> str:
-        """Update constraints on an existing column: GET typed attr -> PUT + @odata.type (#202).
-
-        Hides the PUT-not-PATCH metadata-update contract and the derived
-        ``@odata.type`` discriminator. GETs the existing attribute to learn its
-        derived type + ``MetadataId``, applies the override spec (same shape as
-        create; see ``_apply_column_overrides``), then PUTs a merge payload.
+        """Update constraints on an existing column: retrieve the full attr -> PUT it back (#202).
+
+        Follows the documented column-update contract: the attribute is updated
+        with ``PUT`` carrying the *entire* current definition (not a sparse body),
+        so properties the caller did not touch are preserved. We learn the derived
+        ``@odata.type``, retrieve the complete concrete attribute via the type
+        cast, apply the override spec (same shape as create; see
+        ``_apply_column_overrides``), and ``PUT`` the merged definition back with
+        ``MSCRM.MergeLabels: true`` so localized labels in other languages survive.
         """
-        if not isinstance(overrides, dict) or not overrides:
-            raise TypeError("overrides must be a non-empty dict of column constraints")
-        unknown = set(overrides) - (_COLUMN_OVERRIDE_KEYS - {"type"})
-        if unknown:
-            raise ValueError(f"Unknown column constraint override(s) for '{column_name}': {sorted(unknown)}")
+        self._validate_column_overrides(column_name, overrides)
         ent = self._get_entity_by_table_schema_name(table_schema_name)
         if not ent or not ent.get("MetadataId"):
             raise MetadataError(
@@ -1058,26 +1056,30 @@ def _update_attribute(
             if getattr(err, "status_code", None) == 404:
                 raise MetadataError(
                     f"Column '{column_name}' not found on table '{table_schema_name}'.",
-                    subcode=METADATA_TABLE_NOT_FOUND,
+                    subcode=METADATA_COLUMN_NOT_FOUND,
                 ) from err
             raise
         attr_metadata_id = existing.get("MetadataId")
         odata_type = str(existing.get("@odata.type", "")).lstrip("#")
         if not attr_metadata_id or not odata_type:
             raise MetadataError(
                 f"Column '{column_name}' not found on table '{table_schema_name}'.",
-                subcode=METADATA_TABLE_NOT_FOUND,
+                subcode=METADATA_COLUMN_NOT_FOUND,
             )
-        body: Dict[str, Any] = {
-            "@odata.type": odata_type,
-            "MetadataId": attr_metadata_id,
-            "SchemaName": existing.get("SchemaName", column_name),
-        }
+        # Retrieve the COMPLETE concrete definition (via the @odata.type cast) so the
+        # PUT round-trips every property the caller did not override, per the
+        # documented column-update contract.
+        full = self._request("get", f"{attr_url}/{odata_type}").json()
+        body: Dict[str, Any] = {k: v for k, v in full.items() if not str(k).startswith("@odata.")}
+        body["@odata.type"] = odata_type
+        body["MetadataId"] = attr_metadata_id
+        body.setdefault("SchemaName", existing.get("SchemaName", column_name))
         self._apply_column_overrides(body, overrides)
         req = _RawRequest(
             method="PUT",
             url=f"{self.api}/EntityDefinitions({metadata_id})/Attributes({attr_metadata_id})",
             body=json.dumps(body, ensure_ascii=False),
+            headers={"MSCRM.MergeLabels": "true"},
         )
         self._execute_raw(req)
         return column_name

src/PowerPlatform/Dataverse/data/_odata_base.py

@@ -504,6 +504,14 @@ def _attribute_payload(
             self._apply_column_overrides(payload, overrides)
         return payload
 
+    def _validate_column_overrides(self, column_name: str, overrides: Dict[str, Any]) -> None:
+        """Validate a column-override spec locally (shape + known keys) before any I/O (#202)."""
+        if not isinstance(overrides, dict) or not overrides:
+            raise TypeError("overrides must be a non-empty dict of column constraints")
+        unknown = set(overrides) - (_COLUMN_OVERRIDE_KEYS - {"type"})
+        if unknown:
+            raise ValueError(f"Unknown column constraint override(s) for '{column_name}': {sorted(unknown)}")
+
     def _apply_column_overrides(self, payload: Dict[str, Any], overrides: Dict[str, Any]) -> None:
         """Apply per-column constraint overrides onto a base attribute payload (#194)."""
         if "max_length" in overrides:

src/PowerPlatform/Dataverse/operations/tables.py

@@ -828,6 +828,10 @@ def update_columns(
             raise TypeError("columns must be a non-empty dict of {column: overrides}")
         updated: List[str] = []
         with self._client._scoped_odata() as od:
+            # Validate every spec up front so a bad entry can't leave the table
+            # partially modified by earlier successful updates.
+            for col, spec in columns.items():
+                od._validate_column_overrides(col, spec)
             for col, spec in columns.items():
                 od._update_attribute(table, col, spec)
                 updated.append(col)

tests/unit/aio/core/test_async_http.py

@@ -67,13 +67,13 @@ async def test_delete_uses_120s_default_timeout(self):
         _, kwargs = session.request.call_args
         assert kwargs["timeout"].total == 120
 
-    async def test_put_uses_10s_default_timeout(self):
-        """PUT requests use 10 s default (only POST/DELETE get 120 s)."""
+    async def test_put_uses_120s_default_timeout(self):
+        """PUT requests use 120 s default (it is a write method, like POST/PATCH/DELETE)."""
         session = _make_session()
         client = _AsyncHttpClient(retries=1, session=session)
         await client._request("put", "https://example.com/data")
         _, kwargs = session.request.call_args
-        assert kwargs["timeout"].total == 10
+        assert kwargs["timeout"].total == 120
 
     async def test_patch_uses_120s_default_timeout(self):
         """PATCH requests use 120 s default (same as POST/DELETE)."""

tests/unit/aio/data/test_async_odata_internal.py

@@ -12,6 +12,7 @@
 
 from PowerPlatform.Dataverse.aio.data._async_odata import _AsyncODataClient
 from PowerPlatform.Dataverse.core.errors import HttpError, MetadataError, ValidationError
+from PowerPlatform.Dataverse.core._error_codes import METADATA_COLUMN_NOT_FOUND
 
 # ---------------------------------------------------------------------------
 # Helpers
@@ -2007,10 +2008,14 @@ async def test_update_issues_put_with_odata_type_and_override(self):
         await client._update_attribute(
             "new_Feedback", "new_Comment", {"max_length": 4000, "display_name": "Customer Comment"}
         )
+        # Two GETs: lightweight (learn @odata.type) then the full concrete definition via the type cast.
+        assert client._request.await_count == 2
+        assert "Microsoft.Dynamics.CRM.MemoAttributeMetadata" in client._request.call_args_list[1].args[1]
         client._execute_raw.assert_awaited_once()
         req = client._execute_raw.call_args.args[0]
         assert req.method == "PUT"
         assert "Attributes(attr-1)" in req.url
+        assert req.headers == {"MSCRM.MergeLabels": "true"}
         body = json.loads(req.body)
         assert body["@odata.type"] == "Microsoft.Dynamics.CRM.MemoAttributeMetadata"
         assert body["MetadataId"] == "attr-1"
@@ -2036,8 +2041,9 @@ async def test_update_table_not_found_raises(self):
     async def test_update_column_get_404_raises_metadata_error(self):
         client = self._client()
         client._request = AsyncMock(side_effect=HttpError("not found", 404))
-        with pytest.raises(MetadataError):
+        with pytest.raises(MetadataError) as ei:
             await client._update_attribute("new_Feedback", "ghost", {"max_length": 10})
+        assert ei.value.subcode == METADATA_COLUMN_NOT_FOUND
         client._execute_raw.assert_not_awaited()
 
     async def test_update_column_get_500_propagates(self):
@@ -2049,6 +2055,7 @@ async def test_update_column_get_500_propagates(self):
     async def test_update_column_missing_odata_type_raises(self):
         client = self._client()
         client._request = AsyncMock(return_value=_resp(json_data={"SchemaName": "new_Comment"}))
-        with pytest.raises(MetadataError):
+        with pytest.raises(MetadataError) as ei:
             await client._update_attribute("new_Feedback", "new_Comment", {"max_length": 10})
+        assert ei.value.subcode == METADATA_COLUMN_NOT_FOUND
         client._execute_raw.assert_not_awaited()

tests/unit/aio/test_async_tables.py

@@ -337,13 +337,28 @@ async def test_update_column(self, async_client, mock_od):
 
     async def test_update_columns(self, async_client, mock_od):
         """update_columns() updates each column and returns the names in order."""
+        from unittest.mock import MagicMock
+
+        mock_od._validate_column_overrides = MagicMock()  # sync helper, not a coroutine
         result = await async_client.tables.update_columns(
             "new_Feedback",
             {"new_Comment": {"max_length": 4000}, "new_Rating": {"max_value": 10}},
         )
         assert result == ["new_Comment", "new_Rating"]
         assert mock_od._update_attribute.await_count == 2
 
+    async def test_update_columns_validates_all_before_updating(self, async_client, mock_od):
+        """A bad spec is rejected before ANY column is updated (no partial update)."""
+        from unittest.mock import MagicMock
+        import pytest
+
+        mock_od._validate_column_overrides = MagicMock(side_effect=[None, TypeError("empty")])
+        with pytest.raises(TypeError):
+            await async_client.tables.update_columns(
+                "new_Feedback", {"new_First": {"max_length": 100}, "new_Second": {}}
+            )
+        mock_od._update_attribute.assert_not_awaited()
+
     async def test_update_columns_empty_raises(self, async_client, mock_od):
         """update_columns() rejects an empty mapping before touching the client."""
         import pytest