Fix Python ETag preprocessing for nested operation groups - #11964
Libba Lawrence (l0lawrence) wants to merge 7 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
commit: |
|
All changed packages have been documented.
Show changes
|
Python emitter diffBaseline No changes to generated output. Rendered diff: inline on the run summary, or the emitter-diff-html artifact. Informational check (eng/emitter-diff); does not block the PR. |
|
You can try these changes here
|
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Add regression assertions covering optional lone and both-role ETag paths.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes Python ETag preprocessing for nested operation groups and required conditional headers.
Changes:
- Recursively processes nested ETag headers while retaining root-client state.
- Adds defaults for lone required
If-MatchandIf-None-Matchheaders. - Adds regression tests and a Chronus changelog entry.
File summaries
| File | Description |
|---|---|
packages/http-client-python/tests/unit/test_preprocess_etag.py |
Adds nested and edge-case regression tests. |
packages/http-client-python/generator/pygen/preprocess/__init__.py |
Implements recursive ETag handling and conditional defaults. |
.chronus/changes/fix-required-etag-header-2026-09-14.md |
Records the Python emitter fix. |
Review details
Suppressed comments (1)
packages/http-client-python/tests/unit/test_preprocess_etag.py:350
- The new negative test covers only the multiple-candidate case. The
optionalguard in lines 275-278 is the other advertised non-default path, but no test asserts that a lone optional header's synthesized partner lacksclientDefaultValue; add a regression case here so that guard cannot be removed without failing the suite.
def test_required_match_condition_default_not_added_for_multiple_candidates():
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Add regression coverage for immediate versus nested client-request-id handling.
Review details
Suppressed comments (1)
packages/http-client-python/generator/pygen/preprocess/init.py:625
- This new
requestIdHeaderNameloop has no regression coverage for the explicitly preserved immediate-group scope: the added test only exercises nested ETag processing. Please add cases proving an immediate operation still setsrequestIdHeaderNamewhile the same header in a nested group does not, so this behavior cannot silently change during the recursion refactor.
for operation_group in yaml_data.get("operationGroups", []):
for operation in operation_group.get("operations", []):
for parameter in operation["parameters"]:
if parameter["location"] == "header" and get_wire_name_lower(parameter) == "client-request-id":
yaml_data["requestIdHeaderName"] = "client-request-id"
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
match_conditionpartner without inferring a default, leaving requiredetagandmatch_conditionparameters requiredclient-request-iddetectionRoot cause
The emitter correctly assigns
etagRoleto conditional headers. Python preprocessing then handled ETag setup only for immediate operation groups, while the later parameter transformation already recursed into descendants. Nested operations were therefore partially transformed:ifMatchbecameetag, but nomatch_conditionpartner was synthesized and neither the operation nor root client enabled ETag helper imports.Required-header API
A required
If-Match: Azure.Core.eTagin a nested operation group now generates the existing Python convenience API coherently:Both values are required and forwarded to
prep_if_match; no match condition is inferred by the generator. The service remains responsible for validating the resulting conditional request.Validation
No Spector scenario changes are included.