fix(SPSTRAT-760): Sort resources by durable ID before diffing offers#210
Open
lslebodn wants to merge 2 commits into
Open
fix(SPSTRAT-760): Sort resources by durable ID before diffing offers#210lslebodn wants to merge 2 commits into
lslebodn wants to merge 2 commits into
Conversation
Reviewer's GuideSort Azure offer resources by durable ID and filter out submission resources before computing DeepDiffs, while tightening excluded diff paths and updating tests to reflect order-independent, noise-free diffs. Sequence diagram for AzureService offer diff with resource preparationsequenceDiagram
actor Caller
participant AzureService
participant RemoteProduct as Product_remote
participant LocalProduct as Product_local
participant DeepDiff
Caller->>AzureService: diff_offer(product, target)
AzureService->>AzureService: get_product(product.id, target)
AzureService-->>AzureService: remote Product_remote
AzureService->>RemoteProduct: to_json()
RemoteProduct-->>AzureService: remote_json
AzureService->>AzureService: _prepare_resources_for_diff(remote_json)
AzureService-->>AzureService: prepared_remote
AzureService->>LocalProduct: to_json()
LocalProduct-->>AzureService: local_json
AzureService->>AzureService: _prepare_resources_for_diff(local_json)
AzureService-->>AzureService: prepared_local
AzureService->>DeepDiff: DeepDiff(prepared_remote, prepared_local, exclude_regex_paths=DIFF_EXCLUDES)
DeepDiff-->>AzureService: diff
AzureService-->>Caller: diff
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider making
_sort_resourcesresilient to missing or non-listresourceskeys (e.g., usingoffer_json.get('resources')and returning the input unchanged if absent) to avoid potentialKeyErroror type issues when the API response shape varies. - The updated tests that assert on specific resource indices (e.g., index 7 or 12 after sorting) are somewhat brittle and may break if the fixture data changes; it might be more robust to assert on the presence of a particular DeepDiff path/value pair without depending on its exact index.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider making `_sort_resources` resilient to missing or non-list `resources` keys (e.g., using `offer_json.get('resources')` and returning the input unchanged if absent) to avoid potential `KeyError` or type issues when the API response shape varies.
- The updated tests that assert on specific resource indices (e.g., index 7 or 12 after sorting) are somewhat brittle and may break if the fixture data changes; it might be more robust to assert on the presence of a particular DeepDiff path/value pair without depending on its exact index.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Collaborator
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
_sort_resourcesassumesoffer_json['resources']always exists and is a list; consider usingoffer_json.get('resources')with a type check or early return to avoidKeyError/TypeErrorwhen the shape changes or when resources are absent.- The tests now assert on hard-coded indices (e.g., 7 and 12) after sorting, which couples them tightly to fixture structure; consider asserting on the presence of the specific diff entry (e.g., via substring match or by computing the expected index in the test) rather than relying on exact indices.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- `_sort_resources` assumes `offer_json['resources']` always exists and is a list; consider using `offer_json.get('resources')` with a type check or early return to avoid `KeyError`/`TypeError` when the shape changes or when resources are absent.
- The tests now assert on hard-coded indices (e.g., 7 and 12) after sorting, which couples them tightly to fixture structure; consider asserting on the presence of the specific diff entry (e.g., via substring match or by computing the expected index in the test) rather than relying on exact indices.
## Individual Comments
### Comment 1
<location path="cloudpub/ms_azure/service.py" line_range="454-463" />
<code_context>
)
+ @staticmethod
+ def _sort_resources(offer_json: dict) -> dict:
+ """Sort the resources list by durable ID for consistent comparison.
+
+ The Product Ingestion API does not guarantee the order of resources in the
+ resource-tree response. Sorting by durable ID ensures that DeepDiff compares
+ matching resources rather than resources that happen to share the same index.
+
+ Args:
+ offer_json (dict)
+ The serialized product JSON.
+ Returns:
+ dict: The product JSON with resources sorted by durable ID.
+ """
+ sorted_json = offer_json.copy()
+ sorted_json["resources"] = sorted(offer_json["resources"], key=lambda r: r.get("id", ""))
+ return sorted_json
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against missing or non-list `resources` in `_sort_resources`.
This helper assumes `offer_json["resources"]` is always present and a list. If the JSON sometimes omits `resources` or sets it to `None`/a non-list, this will raise at runtime. Consider using `resources = offer_json.get("resources")` and only sorting when it’s a list, otherwise returning the dict unchanged.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Collaborator
Author
added 2 commits
June 17, 2026 18:37
The Product Ingestion API does not guarantee the order of resources in resource-tree responses. When draft and live versions return resources in different order, DeepDiff compares unrelated resource types at the same index, producing false diffs that block publishing. Sort the resources list by durable ID (which is stable and permanent per the API documentation) before passing to DeepDiff, ensuring matching resources are compared against each other. Submission resources always differ between targets (different durable ID, status, result, created fields). Filter them out before diffing. Ref: https://learn.microsoft.com/en-us/partner-center/marketplace-offers/product-ingestion-api#durable-id Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
When comparing draft vs live offers, certain differences are expected
and should not be flagged:
- The top-level targetType always differs ("draft" vs "live").
- Nested asset URLs contain SAS tokens with timestamps that change
between API calls.
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
_prepare_resources_for_diff, consider handling offers that might not have a'resources'key or where it is not a list (e.g. viaoffer_json.get('resources', [])) to avoid unexpectedKeyError/type issues when schemas or fixtures change. - The updated tests assert on specific resource indices after sorting (e.g. index 7 or 11), which tightly couples them to the current fixture contents; it would be more robust to locate the relevant resource by
idor other stable attributes rather than assuming a fixed position.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_prepare_resources_for_diff`, consider handling offers that might not have a `'resources'` key or where it is not a list (e.g. via `offer_json.get('resources', [])`) to avoid unexpected `KeyError`/type issues when schemas or fixtures change.
- The updated tests assert on specific resource indices after sorting (e.g. index 7 or 11), which tightly couples them to the current fixture contents; it would be more robust to locate the relevant resource by `id` or other stable attributes rather than assuming a fixed position.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Product Ingestion API does not guarantee the order of resources in resource-tree responses. When draft and live versions return resources in different order, DeepDiff compares unrelated resource types at the same index, producing false diffs that block publishing.
Sort the resources list by durable ID (which is stable and permanent per the API documentation) before passing to DeepDiff, ensuring matching resources are compared against each other.
Ref: https://learn.microsoft.com/en-us/partner-center/marketplace-offers/product-ingestion-api#durable-id
Assisted-by: Claude Opus 4.6 noreply@anthropic.com
Summary by Sourcery
Sort Azure offer resources by durable ID and filter out submission resources before diffing to avoid spurious differences caused by resource ordering and target-specific metadata.
Bug Fixes:
Tests: