[python] avoid etag/match_condition clientName collision with multiple etag headers#10816
[python] avoid etag/match_condition clientName collision with multiple etag headers#10816l0lawrence wants to merge 1 commit into
Conversation
…ion 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 microsoft#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>
microsoft-github-policy-service
Bot
added
the
emitter:client:python
commit: |
|
All changed packages have been documented.
Show changes
|
|
You can try these changes here
|
msyyc
left a comment
msyyc
left a comment
There was a problem hiding this comment.
A few minor suggestions on the etag-slot picker logic.
| fall back to the first candidate. Returns None if there are no candidates. | ||
| """ | ||
| if not candidates: | ||
| return None |
There was a problem hiding this comment.
Nit: this early-return is redundant — iterating an empty list naturally falls through to return candidates[0], which would raise IndexError. Either drop the guard and rely on the loop (then handle the empty case at the call site), or keep it but note that candidates[0] is unreachable when empty. As-is it's just dead-code-ish.
| # matches the pre-PR-10494 behaviour, and strip etagRole from the | ||
| # rest so they retain their natural clientName. | ||
| property_if_match = _pick_etag_slot(if_match_candidates, "if-match") | ||
| property_if_none_match = _pick_etag_slot(if_none_match_candidates, "if-none-match") |
There was a problem hiding this comment.
Nit: the standard wire-name literals 'if-match' / 'if-none-match' are passed from the call site. Consider promoting them to module-level constants (e.g. STANDARD_IF_MATCH_WIRE_NAME) for consistency with ETAG_MATCH_DATA / ETAG_NONE_MATCH_DATA already defined in this file, and to avoid a second source of truth.
| c.pop("etagRole", None) | ||
| for c in if_none_match_candidates: | ||
| if c is not property_if_none_match: | ||
| c.pop("etagRole", None) |
There was a problem hiding this comment.
Edge case worth considering (not covered by tests): if an op has only a standard If-Match and only a custom x-ms-source-if-none-match (no standard If-None-Match and no custom If-Match partner), both will be selected and the custom source_if_none_match will be renamed to match_condition — pairing a standard header with a custom one under the same slot. Unlikely in practice, but worth either a defensive check (only promote a custom candidate if its standard counterpart was also selected, or vice versa) or an explicit comment documenting the chosen behavior.
2 participants
Problem
PR #10494 (which added support for custom etag wire names) broke operations that have more than one
Azure.Core.eTag-typed header. The Azure Storage BlobcopyFromUrloperation is a concrete example — it carries:If-Match/If-None-Matchx-ms-source-if-match/x-ms-source-if-none-matchThe emitter in
http.tsstampsetagRole = "ifMatch"/"ifNoneMatch"onto every one of those headers. Then inpreprocess/__init__.py,update_parameterblindly appliesheaders_convert(ETAG_MATCH_DATA)/ETAG_NONE_MATCH_DATAto every parameter with that role — overwritingclientNameto"etag"and"match_condition"on both pairs. The operation ends up with two parameters namedetagand two namedmatch_condition.The slot-picker in
update_clientalready chose one ifMatch/ifNoneMatch slot per operation, but it did nothing to prevent the per-parameter rename on the others.Fix
In
preprocess/__init__.pyupdate_client:ifMatchandifNoneMatchcandidates per operation, not just the first._pick_etag_slothelper prefers the standardIf-Match/If-None-Matchwire names over custom etag headers (matches pre-PR-10494 behaviour when both are present).etagRolefrom non-selected candidates soupdate_parameterleaves their naturalclientNameintact (e.g.source_if_match,source_if_none_match).Result
For
copyFromUrl:If-Match/If-None-Match→ promoted to theetag/match_conditionpair (unchanged behaviour vs. pre-PR-10494).x-ms-source-if-match/x-ms-source-if-none-match→ keep their naturalsource_if_match/source_if_none_matchclient names.No more clientName collisions.
Tests
Adds
tests/unit/test_preprocess_etag.pywith six tests:test_etag_role_preserved_when_only_standard_pair_presenttest_etag_role_preserved_when_only_custom_pair_presenttest_standard_etag_wins_over_custom_when_both_present(regression)test_first_custom_pair_chosen_when_multiple_custom_pairs_presenttest_synthetic_partner_still_works_with_only_one_custom_etagtest_full_update_yaml_does_not_collide_client_names(end-to-end)Verified the three multi-etag tests fail on
upstream/mainand pass with this change. All existing unit tests still pass.Spec referenced
The original repro is
copyFromUrlinspecification/storage/Microsoft.BlobStorage/routes.tspofAzure/azure-rest-api-specs, which composesSourceIfMatchParameter,SourceIfNoneMatchParameter,IfMatchParameter, andIfNoneMatchParameter(allAzure.Core.eTag).