Skip to content

Commit f4c7af2

Browse files
l0lawrenceCopilot
andcommitted
fix(http-client-python): avoid etag/match_condition clientName collision with multiple etag headers
When an operation has more than one Azure.Core.eTag-typed header (e.g. Storage's copyFromUrl, which carries both standard If-Match/If-None-Match AND custom x-ms-source-if-match/x-ms-source-if-none-match), PR #10494's per-parameter conversion renamed every etag-typed header to clientName='etag' or 'match_condition', producing two parameters with the same name on the generated method. Fix: in preprocess update_client, collect all ifMatch/ifNoneMatch candidates per operation, prefer the standard If-Match/If-None-Match wire names for the etag/match_condition slot, and strip etagRole from the non-selected candidates so they retain their natural clientName (e.g. source_if_match). Adds tests/unit/test_preprocess_etag.py covering the standard-only, custom-only, mixed (regression), multi-custom-pair, synthetic-partner, and end-to-end clientName uniqueness scenarios. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 2a20433 commit f4c7af2

3 files changed

Lines changed: 250 additions & 6 deletions

File tree

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
---
2+
changeKind: fix
3+
packages:
4+
- "@typespec/http-client-python"
5+
---
6+
7+
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`).

packages/http-client-python/generator/pygen/preprocess/__init__.py

Lines changed: 40 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,23 @@ def _get_etag_role(parameter: dict[str, Any]) -> Optional[str]:
169169
return parameter.get("etagRole")
170170

171171

172+
def _pick_etag_slot(
173+
candidates: list[dict[str, Any]], standard_wire_name: str
174+
) -> Optional[dict[str, Any]]:
175+
"""Choose which etag-typed header should be promoted to the etag/match_condition slot.
176+
177+
When more than one etag-typed header is present in an operation, prefer the
178+
standard If-Match/If-None-Match header (matched on wireName). Otherwise
179+
fall back to the first candidate. Returns None if there are no candidates.
180+
"""
181+
if not candidates:
182+
return None
183+
for c in candidates:
184+
if get_wire_name_lower(c) == standard_wire_name:
185+
return c
186+
return candidates[0]
187+
188+
172189
def headers_convert(yaml_data: dict[str, Any], replace_data: Any) -> None:
173190
if isinstance(replace_data, dict):
174191
for k, v in replace_data.items():
@@ -313,18 +330,35 @@ def update_client(self, yaml_data: dict[str, Any]) -> None:
313330
yaml_data["builderPadName"] = to_snake_case(prop_name)
314331
for og in yaml_data.get("operationGroups", []):
315332
for o in og["operations"]:
316-
property_if_match = None
317-
property_if_none_match = None
333+
if_match_candidates: list[dict[str, Any]] = []
334+
if_none_match_candidates: list[dict[str, Any]] = []
318335
for p in o["parameters"]:
319336
wire_name_lower = get_wire_name_lower(p)
320337
if p["location"] == "header" and wire_name_lower == "client-request-id":
321338
yaml_data["requestIdHeaderName"] = wire_name_lower
322339
if self.version_tolerant and p["location"] == "header":
323340
role = _get_etag_role(p)
324-
if role == "ifMatch" and not property_if_match:
325-
property_if_match = p
326-
elif role == "ifNoneMatch" and not property_if_none_match:
327-
property_if_none_match = p
341+
if role == "ifMatch":
342+
if_match_candidates.append(p)
343+
elif role == "ifNoneMatch":
344+
if_none_match_candidates.append(p)
345+
346+
# When an operation has multiple etag-typed headers (e.g. Storage's
347+
# copyFromUrl, which has both standard If-Match/If-None-Match and
348+
# custom x-ms-source-if-match/x-ms-source-if-none-match), only one
349+
# pair can be promoted to the etag/match_condition convention.
350+
# Prefer the standard If-Match/If-None-Match pair so the result
351+
# matches the pre-PR-10494 behaviour, and strip etagRole from the
352+
# rest so they retain their natural clientName.
353+
property_if_match = _pick_etag_slot(if_match_candidates, "if-match")
354+
property_if_none_match = _pick_etag_slot(if_none_match_candidates, "if-none-match")
355+
for c in if_match_candidates:
356+
if c is not property_if_match:
357+
c.pop("etagRole", None)
358+
for c in if_none_match_candidates:
359+
if c is not property_if_none_match:
360+
c.pop("etagRole", None)
361+
328362
# pylint: disable=line-too-long
329363
# 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)
330364
# only has one, so we need to add "if-none-match" or "if-match" if it's missing
Lines changed: 203 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,203 @@
1+
# -------------------------------------------------------------------------
2+
# Copyright (c) Microsoft Corporation. All rights reserved.
3+
# Licensed under the MIT License. See License.txt in the project root for
4+
# license information.
5+
# --------------------------------------------------------------------------
6+
"""Tests for etag-typed header handling in the preprocess plugin."""
7+
from pygen.preprocess import PreProcessPlugin
8+
9+
10+
def _plugin() -> PreProcessPlugin:
11+
return PreProcessPlugin(
12+
output_folder="",
13+
options={
14+
"version-tolerant": True,
15+
"models-mode": "dpg",
16+
"show-operations": True,
17+
"show-send-request": True,
18+
"builders-visibility": "public",
19+
},
20+
)
21+
22+
23+
def _header_param(client_name: str, wire_name: str, etag_role: str | None) -> dict:
24+
p: dict = {
25+
"clientName": client_name,
26+
"wireName": wire_name,
27+
"location": "header",
28+
"optional": True,
29+
"implementation": "Method",
30+
"type": {"type": "string"},
31+
}
32+
if etag_role is not None:
33+
p["etagRole"] = etag_role
34+
return p
35+
36+
37+
def _client_yaml(operation_params: list[dict]) -> dict:
38+
return {
39+
"name": "TestClient",
40+
"namespace": "test",
41+
"moduleName": "test",
42+
"url": "",
43+
"description": "test",
44+
"parameters": [],
45+
"operationGroups": [
46+
{
47+
"operations": [
48+
{
49+
"name": "copyFromUrl",
50+
"parameters": operation_params,
51+
}
52+
]
53+
}
54+
],
55+
}
56+
57+
58+
def _get_op(client: dict) -> dict:
59+
return client["operationGroups"][0]["operations"][0]
60+
61+
62+
def test_etag_role_preserved_when_only_standard_pair_present():
63+
"""Standard If-Match/If-None-Match keep their etagRole."""
64+
if_match = _header_param("if_match", "If-Match", "ifMatch")
65+
if_none_match = _header_param("if_none_match", "If-None-Match", "ifNoneMatch")
66+
client = _client_yaml([if_match, if_none_match])
67+
68+
_plugin().update_client(client)
69+
70+
op = _get_op(client)
71+
standard_match = next(p for p in op["parameters"] if p["wireName"] == "If-Match")
72+
standard_none = next(p for p in op["parameters"] if p["wireName"] == "If-None-Match")
73+
assert standard_match.get("etagRole") == "ifMatch"
74+
assert standard_none.get("etagRole") == "ifNoneMatch"
75+
assert op["hasEtag"] is True
76+
77+
78+
def test_etag_role_preserved_when_only_custom_pair_present():
79+
"""Custom etag headers alone are promoted to the etag/match_condition slot."""
80+
source_match = _header_param("source_if_match", "x-ms-source-if-match", "ifMatch")
81+
source_none = _header_param(
82+
"source_if_none_match", "x-ms-source-if-none-match", "ifNoneMatch"
83+
)
84+
client = _client_yaml([source_match, source_none])
85+
86+
_plugin().update_client(client)
87+
88+
assert source_match.get("etagRole") == "ifMatch"
89+
assert source_none.get("etagRole") == "ifNoneMatch"
90+
91+
92+
def test_standard_etag_wins_over_custom_when_both_present():
93+
"""When both standard and custom etag headers are present in the same operation,
94+
the standard If-Match/If-None-Match pair takes the etag/match_condition slot and
95+
the custom headers have their etagRole stripped so they retain their natural
96+
clientName (e.g. source_if_match) instead of colliding with the standard pair.
97+
98+
Regression test for PR #10494 which caused operations like Storage's copyFromUrl
99+
to emit two parameters named "etag" and two named "match_condition".
100+
"""
101+
source_match = _header_param(
102+
"source_if_match", "x-ms-source-if-match", "ifMatch"
103+
)
104+
source_none = _header_param(
105+
"source_if_none_match", "x-ms-source-if-none-match", "ifNoneMatch"
106+
)
107+
if_match = _header_param("if_match", "If-Match", "ifMatch")
108+
if_none_match = _header_param("if_none_match", "If-None-Match", "ifNoneMatch")
109+
110+
# Mirror the parameter ordering in routes.tsp: source headers come first.
111+
client = _client_yaml([source_match, source_none, if_match, if_none_match])
112+
_plugin().update_client(client)
113+
114+
# Standard pair keeps etagRole and so will be transformed into etag/match_condition.
115+
assert if_match.get("etagRole") == "ifMatch"
116+
assert if_none_match.get("etagRole") == "ifNoneMatch"
117+
# Custom pair has etagRole stripped, so update_parameter will NOT rename them.
118+
assert "etagRole" not in source_match
119+
assert "etagRole" not in source_none
120+
121+
# The selected pair should be at the end of the operation parameters.
122+
op = _get_op(client)
123+
assert op["parameters"][-2] is if_match
124+
assert op["parameters"][-1] is if_none_match
125+
assert op["hasEtag"] is True
126+
127+
128+
def test_first_custom_pair_chosen_when_multiple_custom_pairs_present():
129+
"""With multiple custom etag pairs and no standard pair, the first candidate wins."""
130+
blob_match = _header_param("blob_if_match", "x-ms-blob-if-match", "ifMatch")
131+
blob_none = _header_param(
132+
"blob_if_none_match", "x-ms-blob-if-none-match", "ifNoneMatch"
133+
)
134+
source_match = _header_param(
135+
"source_if_match", "x-ms-source-if-match", "ifMatch"
136+
)
137+
source_none = _header_param(
138+
"source_if_none_match", "x-ms-source-if-none-match", "ifNoneMatch"
139+
)
140+
client = _client_yaml([blob_match, blob_none, source_match, source_none])
141+
142+
_plugin().update_client(client)
143+
144+
# First-encountered pair wins; the rest have etagRole stripped.
145+
assert blob_match.get("etagRole") == "ifMatch"
146+
assert blob_none.get("etagRole") == "ifNoneMatch"
147+
assert "etagRole" not in source_match
148+
assert "etagRole" not in source_none
149+
150+
151+
def test_synthetic_partner_still_works_with_only_one_custom_etag():
152+
"""When only a single custom etag header is present (no partner), the existing
153+
synthetic-partner code path still creates a matching ifNoneMatch (or ifMatch)
154+
copy. The fix must not regress this behaviour.
155+
"""
156+
source_match = _header_param(
157+
"source_if_match", "x-ms-source-if-match", "ifMatch"
158+
)
159+
client = _client_yaml([source_match])
160+
161+
_plugin().update_client(client)
162+
163+
op = _get_op(client)
164+
assert op.get("hasEtag") is True
165+
# The original custom param plus a synthetic partner are pushed to the end.
166+
last_two = op["parameters"][-2:]
167+
assert last_two[0]["etagRole"] == "ifMatch"
168+
assert last_two[1]["etagRole"] == "ifNoneMatch"
169+
170+
171+
def test_full_update_yaml_does_not_collide_client_names():
172+
"""End-to-end: after update_client + update_parameter, the four etag headers
173+
have distinct clientNames.
174+
175+
Without the fix, both source_if_match and if_match end up with clientName="etag",
176+
and both source_if_none_match and if_none_match end up with clientName="match_condition".
177+
"""
178+
source_match = _header_param(
179+
"source_if_match", "x-ms-source-if-match", "ifMatch"
180+
)
181+
source_none = _header_param(
182+
"source_if_none_match", "x-ms-source-if-none-match", "ifNoneMatch"
183+
)
184+
if_match = _header_param("if_match", "If-Match", "ifMatch")
185+
if_none_match = _header_param("if_none_match", "If-None-Match", "ifNoneMatch")
186+
client = _client_yaml([source_match, source_none, if_match, if_none_match])
187+
188+
plugin = _plugin()
189+
plugin.update_client(client)
190+
# update_client does the slot/strip; update_parameter does the rename.
191+
op = _get_op(client)
192+
for p in op["parameters"]:
193+
plugin.update_parameter(p)
194+
195+
client_names = [p["clientName"] for p in op["parameters"]]
196+
assert len(client_names) == len(set(client_names)), (
197+
f"Duplicate clientNames after preprocess: {client_names}"
198+
)
199+
# The standard pair was promoted; the custom pair retains its natural names.
200+
assert "etag" in client_names
201+
assert "match_condition" in client_names
202+
assert "source_if_match" in client_names
203+
assert "source_if_none_match" in client_names

0 commit comments

Comments
 (0)