diff --git a/.chronus/changes/fix-python-multi-etag-collision-2026-05-27-10-15-00.md b/.chronus/changes/fix-python-multi-etag-collision-2026-05-27-10-15-00.md new file mode 100644 index 00000000000..92d4f21b256 --- /dev/null +++ b/.chronus/changes/fix-python-multi-etag-collision-2026-05-27-10-15-00.md @@ -0,0 +1,7 @@ +--- +changeKind: fix +packages: + - "@typespec/http-client-python" +--- + +Fix `etag`/`match_condition` clientName collision when an operation has more than one `Azure.Core.eTag`-typed header (e.g. Storage's `copyFromUrl`, which has both `If-Match`/`If-None-Match` and `x-ms-source-if-match`/`x-ms-source-if-none-match`). The standard `If-Match`/`If-None-Match` pair is now preferred for the `etag`/`match_condition` slot, and any additional etag-typed headers retain their natural client name (e.g. `source_if_match`). diff --git a/packages/http-client-python/generator/pygen/preprocess/__init__.py b/packages/http-client-python/generator/pygen/preprocess/__init__.py index d883f52cf31..1c1e575f44b 100644 --- a/packages/http-client-python/generator/pygen/preprocess/__init__.py +++ b/packages/http-client-python/generator/pygen/preprocess/__init__.py @@ -158,6 +158,8 @@ def update_paging_response(yaml_data: dict[str, Any]) -> None: "isTypingOnly": True, }, } +STANDARD_IF_MATCH_WIRE_NAME = "if-match" +STANDARD_IF_NONE_MATCH_WIRE_NAME = "if-none-match" def get_wire_name_lower(parameter: dict[str, Any]) -> str: @@ -169,6 +171,106 @@ def _get_etag_role(parameter: dict[str, Any]) -> Optional[str]: return parameter.get("etagRole") +def _pick_etag_slot( + candidates: list[dict[str, Any]], standard_wire_name: str +) -> Optional[dict[str, Any]]: + """Choose which etag-typed header should be promoted to the etag/match_condition slot. + + When more than one etag-typed header is present in an operation, prefer the + standard If-Match/If-None-Match header (matched on wireName). Otherwise + fall back to the first candidate. Returns None if there are no candidates. + """ + if not candidates: + return None + for c in candidates: + if get_wire_name_lower(c) == standard_wire_name: + return c + return candidates[0] + + +def _resolve_etag_pair( + if_match_candidates: list[dict[str, Any]], + if_none_match_candidates: list[dict[str, Any]], +) -> tuple[Optional[dict[str, Any]], Optional[dict[str, Any]]]: + """Select and reconcile the etag header pair for an operation. + + When multiple etag-typed headers are present, prefer the standard + If-Match / If-None-Match pair. Synthesize a missing partner when only + one side is present, and strip etagRole from non-selected candidates. + + Returns (property_if_match, property_if_none_match) — both None when + there are no etag candidates. + """ + property_if_match = _pick_etag_slot(if_match_candidates, STANDARD_IF_MATCH_WIRE_NAME) + property_if_none_match = _pick_etag_slot(if_none_match_candidates, STANDARD_IF_NONE_MATCH_WIRE_NAME) + + # Ensure the promoted pair come from the same family. When one slot is + # standard and the other custom (cross-family), replace the custom slot + # with a synthetic standard partner. Also synthesize the missing partner + # when only one side is present. + if property_if_match and property_if_none_match: + match_is_std = get_wire_name_lower(property_if_match) == STANDARD_IF_MATCH_WIRE_NAME + none_match_is_std = get_wire_name_lower(property_if_none_match) == STANDARD_IF_NONE_MATCH_WIRE_NAME + if match_is_std and not none_match_is_std: + property_if_none_match = property_if_match.copy() + property_if_none_match["wireName"] = STANDARD_IF_NONE_MATCH_WIRE_NAME + property_if_none_match["etagRole"] = "ifNoneMatch" + elif none_match_is_std and not match_is_std: + property_if_match = property_if_none_match.copy() + property_if_match["wireName"] = STANDARD_IF_MATCH_WIRE_NAME + property_if_match["etagRole"] = "ifMatch" + elif not property_if_match and property_if_none_match: + property_if_match = property_if_none_match.copy() + property_if_match["wireName"] = STANDARD_IF_MATCH_WIRE_NAME + property_if_match["etagRole"] = "ifMatch" + elif property_if_match and not property_if_none_match: + property_if_none_match = property_if_match.copy() + property_if_none_match["wireName"] = STANDARD_IF_NONE_MATCH_WIRE_NAME + property_if_none_match["etagRole"] = "ifNoneMatch" + + for c in if_match_candidates: + if c is not property_if_match: + c.pop("etagRole", None) + for c in if_none_match_candidates: + if c is not property_if_none_match: + c.pop("etagRole", None) + + return property_if_match, property_if_none_match + + +def _process_operation_etag_headers( + operation: dict[str, Any], + client: dict[str, Any], + version_tolerant: bool, +) -> None: + """Collect etag candidates from *operation*, resolve the promoted pair, + and update *operation* / *client* accordingly.""" + if_match_candidates: list[dict[str, Any]] = [] + if_none_match_candidates: list[dict[str, Any]] = [] + for p in operation["parameters"]: + wire_name_lower = get_wire_name_lower(p) + if p["location"] == "header" and wire_name_lower == "client-request-id": + client["requestIdHeaderName"] = wire_name_lower + if version_tolerant and p["location"] == "header": + role = _get_etag_role(p) + if role == "ifMatch": + if_match_candidates.append(p) + elif role == "ifNoneMatch": + if_none_match_candidates.append(p) + + property_if_match, property_if_none_match = _resolve_etag_pair( + if_match_candidates, if_none_match_candidates + ) + + if property_if_match and property_if_none_match: + etag_params = {id(property_if_match), id(property_if_none_match)} + operation["parameters"] = [ + item for item in operation["parameters"] if id(item) not in etag_params + ] + [property_if_match, property_if_none_match] + operation["hasEtag"] = True + client["hasEtag"] = True + + def headers_convert(yaml_data: dict[str, Any], replace_data: Any) -> None: if isinstance(replace_data, dict): for k, v in replace_data.items(): @@ -313,40 +415,7 @@ def update_client(self, yaml_data: dict[str, Any]) -> None: yaml_data["builderPadName"] = to_snake_case(prop_name) for og in yaml_data.get("operationGroups", []): for o in og["operations"]: - property_if_match = None - property_if_none_match = None - for p in o["parameters"]: - wire_name_lower = get_wire_name_lower(p) - if p["location"] == "header" and wire_name_lower == "client-request-id": - yaml_data["requestIdHeaderName"] = wire_name_lower - if self.version_tolerant and p["location"] == "header": - role = _get_etag_role(p) - if role == "ifMatch" and not property_if_match: - property_if_match = p - elif role == "ifNoneMatch" and not property_if_none_match: - property_if_none_match = p - # pylint: disable=line-too-long - # some service(e.g. https://github.com/Azure/azure-rest-api-specs/blob/main/specification/cosmos-db/data-plane/Microsoft.Tables/preview/2019-02-02/table.json) - # only has one, so we need to add "if-none-match" or "if-match" if it's missing - if not property_if_match and property_if_none_match: - property_if_match = property_if_none_match.copy() - property_if_match["wireName"] = "if-match" - property_if_match["etagRole"] = "ifMatch" - if not property_if_none_match and property_if_match: - property_if_none_match = property_if_match.copy() - property_if_none_match["wireName"] = "if-none-match" - property_if_none_match["etagRole"] = "ifNoneMatch" - - if property_if_match and property_if_none_match: - # arrange if-match and if-none-match to the end of parameters - etag_params = {id(property_if_match), id(property_if_none_match)} - o["parameters"] = [item for item in o["parameters"] if id(item) not in etag_params] + [ - property_if_match, - property_if_none_match, - ] - - o["hasEtag"] = True - yaml_data["hasEtag"] = True + _process_operation_etag_headers(o, yaml_data, self.version_tolerant) # add client signature cloud_setting for arm if self.azure_arm and yaml_data["parameters"]: diff --git a/packages/http-client-python/tests/unit/test_preprocess_etag.py b/packages/http-client-python/tests/unit/test_preprocess_etag.py new file mode 100644 index 00000000000..f631f71ba51 --- /dev/null +++ b/packages/http-client-python/tests/unit/test_preprocess_etag.py @@ -0,0 +1,270 @@ +# ------------------------------------------------------------------------- +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. See License.txt in the project root for +# license information. +# -------------------------------------------------------------------------- +"""Tests for etag-typed header handling in the preprocess plugin.""" +from pygen.preprocess import PreProcessPlugin + + +def _plugin() -> PreProcessPlugin: + return PreProcessPlugin( + output_folder="", + options={ + "version-tolerant": True, + "models-mode": "dpg", + "show-operations": True, + "show-send-request": True, + "builders-visibility": "public", + }, + ) + + +def _header_param(client_name: str, wire_name: str, etag_role: str | None) -> dict: + p: dict = { + "clientName": client_name, + "wireName": wire_name, + "location": "header", + "optional": True, + "implementation": "Method", + "type": {"type": "string"}, + } + if etag_role is not None: + p["etagRole"] = etag_role + return p + + +def _client_yaml(operation_params: list[dict]) -> dict: + return { + "name": "TestClient", + "namespace": "test", + "moduleName": "test", + "url": "", + "description": "test", + "parameters": [], + "operationGroups": [ + { + "operations": [ + { + "name": "copyFromUrl", + "parameters": operation_params, + } + ] + } + ], + } + + +def _get_op(client: dict) -> dict: + return client["operationGroups"][0]["operations"][0] + + +def test_etag_role_preserved_when_only_standard_pair_present(): + """Standard If-Match/If-None-Match keep their etagRole.""" + if_match = _header_param("if_match", "If-Match", "ifMatch") + if_none_match = _header_param("if_none_match", "If-None-Match", "ifNoneMatch") + client = _client_yaml([if_match, if_none_match]) + + _plugin().update_client(client) + + op = _get_op(client) + standard_match = next(p for p in op["parameters"] if p["wireName"] == "If-Match") + standard_none = next(p for p in op["parameters"] if p["wireName"] == "If-None-Match") + assert standard_match.get("etagRole") == "ifMatch" + assert standard_none.get("etagRole") == "ifNoneMatch" + assert op["hasEtag"] is True + + +def test_etag_role_preserved_when_only_custom_pair_present(): + """Custom etag headers alone are promoted to the etag/match_condition slot.""" + source_match = _header_param("source_if_match", "x-ms-source-if-match", "ifMatch") + source_none = _header_param( + "source_if_none_match", "x-ms-source-if-none-match", "ifNoneMatch" + ) + client = _client_yaml([source_match, source_none]) + + _plugin().update_client(client) + + assert source_match.get("etagRole") == "ifMatch" + assert source_none.get("etagRole") == "ifNoneMatch" + + +def test_standard_etag_wins_over_custom_when_both_present(): + """When both standard and custom etag headers are present in the same operation, + the standard If-Match/If-None-Match pair takes the etag/match_condition slot and + the custom headers have their etagRole stripped so they retain their natural + clientName (e.g. source_if_match) instead of colliding with the standard pair. + + Regression test for PR #10494 which caused operations like Storage's copyFromUrl + to emit two parameters named "etag" and two named "match_condition". + """ + source_match = _header_param( + "source_if_match", "x-ms-source-if-match", "ifMatch" + ) + source_none = _header_param( + "source_if_none_match", "x-ms-source-if-none-match", "ifNoneMatch" + ) + if_match = _header_param("if_match", "If-Match", "ifMatch") + if_none_match = _header_param("if_none_match", "If-None-Match", "ifNoneMatch") + + # Mirror the parameter ordering in routes.tsp: source headers come first. + client = _client_yaml([source_match, source_none, if_match, if_none_match]) + _plugin().update_client(client) + + # Standard pair keeps etagRole and so will be transformed into etag/match_condition. + assert if_match.get("etagRole") == "ifMatch" + assert if_none_match.get("etagRole") == "ifNoneMatch" + # Custom pair has etagRole stripped, so update_parameter will NOT rename them. + assert "etagRole" not in source_match + assert "etagRole" not in source_none + + # The selected pair should be at the end of the operation parameters. + op = _get_op(client) + assert op["parameters"][-2] is if_match + assert op["parameters"][-1] is if_none_match + assert op["hasEtag"] is True + + +def test_first_custom_pair_chosen_when_multiple_custom_pairs_present(): + """With multiple custom etag pairs and no standard pair, the first candidate wins.""" + blob_match = _header_param("blob_if_match", "x-ms-blob-if-match", "ifMatch") + blob_none = _header_param( + "blob_if_none_match", "x-ms-blob-if-none-match", "ifNoneMatch" + ) + source_match = _header_param( + "source_if_match", "x-ms-source-if-match", "ifMatch" + ) + source_none = _header_param( + "source_if_none_match", "x-ms-source-if-none-match", "ifNoneMatch" + ) + client = _client_yaml([blob_match, blob_none, source_match, source_none]) + + _plugin().update_client(client) + + # First-encountered pair wins; the rest have etagRole stripped. + assert blob_match.get("etagRole") == "ifMatch" + assert blob_none.get("etagRole") == "ifNoneMatch" + assert "etagRole" not in source_match + assert "etagRole" not in source_none + + +def test_synthetic_partner_still_works_with_only_one_custom_etag(): + """When only a single custom etag header is present (no partner), the existing + synthetic-partner code path still creates a matching ifNoneMatch (or ifMatch) + copy. The fix must not regress this behavior. + """ + source_match = _header_param( + "source_if_match", "x-ms-source-if-match", "ifMatch" + ) + client = _client_yaml([source_match]) + + _plugin().update_client(client) + + op = _get_op(client) + assert op.get("hasEtag") is True + # The original custom param plus a synthetic partner are pushed to the end. + last_two = op["parameters"][-2:] + assert last_two[0]["etagRole"] == "ifMatch" + assert last_two[1]["etagRole"] == "ifNoneMatch" + + +def test_full_update_yaml_does_not_collide_client_names(): + """End-to-end: after update_client + update_parameter, the four etag headers + have distinct clientNames. + + Without the fix, both source_if_match and if_match end up with clientName="etag", + and both source_if_none_match and if_none_match end up with clientName="match_condition". + """ + source_match = _header_param( + "source_if_match", "x-ms-source-if-match", "ifMatch" + ) + source_none = _header_param( + "source_if_none_match", "x-ms-source-if-none-match", "ifNoneMatch" + ) + if_match = _header_param("if_match", "If-Match", "ifMatch") + if_none_match = _header_param("if_none_match", "If-None-Match", "ifNoneMatch") + client = _client_yaml([source_match, source_none, if_match, if_none_match]) + + plugin = _plugin() + plugin.update_client(client) + # update_client does the slot/strip; update_parameter does the rename. + op = _get_op(client) + for p in op["parameters"]: + plugin.update_parameter(p) + + client_names = [p["clientName"] for p in op["parameters"]] + assert len(client_names) == len(set(client_names)), ( + f"Duplicate clientNames after preprocess: {client_names}" + ) + # The standard pair was promoted; the custom pair retains its natural names. + assert "etag" in client_names + assert "match_condition" in client_names + assert "source_if_match" in client_names + assert "source_if_none_match" in client_names + + +def test_standard_if_match_not_paired_with_custom_if_none_match(): + """Edge case: an op has a standard If-Match but only a custom + x-ms-source-if-none-match (no standard If-None-Match, no custom If-Match + partner). Without the defensive check the custom header would be promoted + to the match_condition slot, pairing a standard header with a custom one + from a different family. + + The fix demotes the custom header (strips etagRole) so the standard + If-Match gets a synthetic If-None-Match partner instead. + """ + if_match = _header_param("if_match", "If-Match", "ifMatch") + source_none = _header_param( + "source_if_none_match", "x-ms-source-if-none-match", "ifNoneMatch" + ) + client = _client_yaml([if_match, source_none]) + + plugin = _plugin() + plugin.update_client(client) + + op = _get_op(client) + assert op.get("hasEtag") is True + + # The standard If-Match should be promoted and paired with a synthetic partner. + last_two = op["parameters"][-2:] + assert last_two[0]["etagRole"] == "ifMatch" + assert last_two[0]["wireName"] == "If-Match" + assert last_two[1]["etagRole"] == "ifNoneMatch" + assert last_two[1]["wireName"] == "if-none-match" # synthetic + + # The custom header should NOT have been promoted — etagRole stripped. + assert "etagRole" not in source_none + + # After update_parameter, no duplicate clientNames. + for p in op["parameters"]: + plugin.update_parameter(p) + client_names = [p["clientName"] for p in op["parameters"]] + assert len(client_names) == len(set(client_names)), ( + f"Duplicate clientNames: {client_names}" + ) + + +def test_standard_if_none_match_not_paired_with_custom_if_match(): + """Symmetric case: standard If-None-Match + custom x-ms-source-if-match + (no standard If-Match, no custom If-None-Match). + + The custom header should be demoted; the standard If-None-Match gets a + synthetic If-Match partner. + """ + source_match = _header_param("source_if_match", "x-ms-source-if-match", "ifMatch") + if_none_match = _header_param("if_none_match", "If-None-Match", "ifNoneMatch") + client = _client_yaml([source_match, if_none_match]) + + plugin = _plugin() + plugin.update_client(client) + + op = _get_op(client) + assert op.get("hasEtag") is True + + last_two = op["parameters"][-2:] + assert last_two[0]["etagRole"] == "ifMatch" + assert last_two[0]["wireName"] == "if-match" # synthetic + assert last_two[1]["etagRole"] == "ifNoneMatch" + assert last_two[1]["wireName"] == "If-None-Match" + + assert "etagRole" not in source_match