Open
Conversation
Reviewer's GuideRefactors the AWS PromotionalResources model so PromotionalMedia is represented as a typed list of PromotionalMedia items instead of a single string, and adds tests to cover various payload shapes and absence cases. Class diagram for updated AWS PromotionalResources and new PromotionalMedia modelclassDiagram
class BaseResources
class AttrsJSONDecodeMixin
class PromotionalMedia {
+str title
+Optional~str~ description
+from_json(data)
}
class PromotionalResources {
+Optional~List~promotional_media~
}
PromotionalMedia --|> BaseResources
PromotionalResources ..|> AttrsJSONDecodeMixin
PromotionalResources "0..1" --> "*" PromotionalMedia : promotional_media
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Collaborator
Author
![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 found 3 issues, and left some high level feedback:
- The
promotional_mediaconverter assumes every element is raw JSON and will fail if a list ofPromotionalMediaobjects is passed directly; consider making the converter a small helper that is a no-op for existingPromotionalMediainstances and only callsfrom_jsonfor dicts. - The inline lambda used as the
promotional_mediaconverter is getting a bit dense; extracting it into a named function would make the conversion logic clearer and easier to extend (e.g., to handle unexpected input types more gracefully).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `promotional_media` converter assumes every element is raw JSON and will fail if a list of `PromotionalMedia` objects is passed directly; consider making the converter a small helper that is a no-op for existing `PromotionalMedia` instances and only calls `from_json` for dicts.
- The inline lambda used as the `promotional_media` converter is getting a bit dense; extracting it into a named function would make the conversion logic clearer and easier to extend (e.g., to handle unexpected input types more gracefully).
## Individual Comments
### Comment 1
<location> `cloudpub/models/aws.py:658-659` </location>
<code_context>
- promotional_media: Optional[str] = field(
- validator=optional(instance_of(str)),
+ promotional_media: Optional[List[PromotionalMedia]] = field(
+ converter=lambda x: [PromotionalMedia.from_json(a) for a in x] if x else None,
+ on_setattr=NO_OP,
metadata={"alias": "PromotionalMedia", "hide_unset": True},
</code_context>
<issue_to_address>
**issue:** The converter collapses an empty list into None, which may conflate two distinct states.
Because the converter uses `if x else None`, both `[]` and `None` become `None`, losing the distinction between "no PromotionalMedia provided" and "provided but empty". If that distinction is needed by callers or serialization, keep empty lists, e.g.:
`converter=lambda x: [PromotionalMedia.from_json(a) for a in x] if x is not None else None`
</issue_to_address>
### Comment 2
<location> `tests/aws/test_models.py:132-141` </location>
<code_context>
+@pytest.mark.parametrize(
</code_context>
<issue_to_address>
**suggestion:** Add a test case for an empty PromotionalMedia list to cover the falsy-but-non-None edge case.
The current parametrization only covers `None`, a single-element list, and a multi-element list. Because the converter uses `if x else None`, an empty list will also become `None`, which is a distinct case worth testing. Please add a `[]` case and assert the resulting `obj.promotional_media` matches the intended behavior for an empty list. You may also need to update `if not promotional_media:` to `if promotional_media is None` so that the empty-list case behaves correctly and is accurately captured by the new test.
Suggested implementation:
```python
@pytest.mark.parametrize(
"promotional_media",
[
None,
[],
[
{
'Type': 'Image',
'Url': 'https://foo.com/bar.png',
'Title': 'Logo1',
'Description': None,
}
```
To fully implement the requested behavior and assertions, you will also need to:
1. Update the production code where `promotional_media` is converted (likely in the model or response parsing logic, not shown here):
- Change any `if not promotional_media:` guard to `if promotional_media is None:` so that an empty list is treated as an empty collection instead of `None`.
- Ensure that when `promotional_media` is `[]`, the resulting `obj.promotional_media` is an empty list (or the equivalent empty collection type used in your model), not `None`.
2. Update the corresponding parametrized test function in `tests/aws/test_models.py` (the body is not visible in the snippet):
- For the `promotional_media is None` case, assert that `obj.promotional_media` is `None`.
- For the new `promotional_media == []` case, assert that `obj.promotional_media` is an empty list (or the appropriate empty collection), e.g. `assert obj.promotional_media == []`.
- For the non-empty list case(s), keep (or add) assertions that `obj.promotional_media` contains the expected `PromotionalMedia` items with the correct fields.
Because the test function body and the converter implementation are not shown in the provided snippet, you will need to adapt these changes to the exact function names and structures in your codebase.
</issue_to_address>
### Comment 3
<location> `tests/aws/test_models.py:160-169` </location>
<code_context>
+ ],
+ ],
+)
+def test_promotional_resources_promotional_media(
+ promotional_media, promotional_resource: Dict[str, Any]
+) -> None:
+ promotional_resource["PromotionalMedia"] = promotional_media
+
+ obj = PromotionalResources.from_json(promotional_resource)
+
+ assert obj
+ if not promotional_media:
+ assert obj.promotional_media is None
+ else:
+ assert obj.promotional_media == [PromotionalMedia.from_json(x) for x in promotional_media]
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test where the PromotionalMedia key is completely absent from the payload.
Right now the test always includes a `PromotionalMedia` key in `promotional_resource`, even when its value is `None`. Please add coverage (or a parametrized case) where the key is omitted entirely and confirm `PromotionalResources.from_json` treats that as expected (e.g., `promotional_media is None`), since AWS can distinguish between `null` and a missing field.
Suggested implementation:
```python
_PROMOTIONAL_MEDIA_MISSING = object()
@pytest.mark.parametrize(
"promotional_media",
```
```python
},
],
_PROMOTIONAL_MEDIA_MISSING,
],
)
def test_promotional_resources_promotional_media(
promotional_media, promotional_resource: Dict[str, Any]
) -> None:
if promotional_media is not _PROMOTIONAL_MEDIA_MISSING:
promotional_resource["PromotionalMedia"] = promotional_media
```
```python
obj = PromotionalResources.from_json(promotional_resource)
assert obj
if promotional_media is _PROMOTIONAL_MEDIA_MISSING:
# Key is completely absent from the payload
assert obj.promotional_media is None
elif not promotional_media:
# Key is present but value is falsy (e.g. None or empty list)
assert obj.promotional_media is None
else:
assert obj.promotional_media == [PromotionalMedia.from_json(x) for x in promotional_media]
```
These edits assume the existing parametrization is `@pytest.mark.parametrize("promotional_media", [...])` and that the shown list is the complete list of values for `promotional_media`.
If the decorator signature differs (e.g. multiple parameters, different name), adjust the first SEARCH/REPLACE block accordingly so that `_PROMOTIONAL_MEDIA_MISSING` is defined near the tests and added as an additional case in the appropriate parametrized argument list.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
JAVGan
force-pushed
the
aws_models_fix
branch
from
1e865e7 to
64a77a5
Compare
Collaborator
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
promotional_mediafield currently lacks a validator, so arbitrary types could pass through; consider adding avalidator=optional(deep_iterable(member_validator=instance_of(PromotionalMedia), iterable_validator=instance_of(list)))(or similar) to keep the list contents type-safe. - The converter for
promotional_mediaalways callsPromotionalMedia.from_jsonon each element when truthy, which will fail if callers pass already-instantiatedPromotionalMediaobjects; consider making the converter idempotent by detecting existing instances before converting.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `promotional_media` field currently lacks a validator, so arbitrary types could pass through; consider adding a `validator=optional(deep_iterable(member_validator=instance_of(PromotionalMedia), iterable_validator=instance_of(list)))` (or similar) to keep the list contents type-safe.
- The converter for `promotional_media` always calls `PromotionalMedia.from_json` on each element when truthy, which will fail if callers pass already-instantiated `PromotionalMedia` objects; consider making the converter idempotent by detecting existing instances before converting.
## Individual Comments
### Comment 1
<location> `tests/aws/test_models.py:134-138` </location>
<code_context>
+
+@pytest.mark.parametrize(
+ "promotional_media",
+ [
+ None,
+ [],
+ [
+ {
+ 'Type': 'Image',
+ 'Url': 'https://foo.com/bar.png',
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a case where `Description` is completely absent, not just `None`.
Current cases cover `Description` as `None` and as a non-empty string, but not when the key is missing. Please add a parameterized input where the JSON omits the `"Description"` field and assert that deserialization still succeeds and behaves as intended, to fully verify the field’s optionality.
Suggested implementation:
```python
@pytest.mark.parametrize(
"promotional_media",
[
None,
[],
[
{
'Type': 'Image',
'Url': 'https://foo.com/bar.png',
'Title': 'Logo1',
'Description': None,
}
],
[
{
'Type': 'Image',
'Url': 'https://foo.com/bar.png',
'Title': 'Logo1',
# Description field intentionally omitted to test absence
}
],
[
{
'Type': 'Image',
'Url': 'https://foo.com/bar.png',
'Title': 'Logo1',
'Description': 'This is the logo1',
```
This change assumes the underlying deserialization and the test assertions already treat `Description` as optional (i.e., they do not assume presence of the attribute). If any assertion in the associated test function explicitly expects a `description` attribute/key to always exist, it should be relaxed to allow for the "absent" case (for example, by using `get` or checking for `hasattr`/`in` only when appropriate).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
JAVGan
force-pushed
the
aws_models_fix
branch
from
64a77a5 to
74b259e
Compare
Collaborator
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
promotional_mediaconverter assumes all incoming items are raw JSON dicts; consider making it idempotent (e.g., pass through existingPromotionalMediainstances) and extracting it into a named helper for clarity and reuse. - In
PromotionalMedia, thedescriptionfield is optional but has no default; iffrom_jsonever calls the regular initializer directly, this may raise a missing-argument error, so it could be safer to setdescription: Optional[str] = field(default=None, ...)to make the optionality explicit at the model level.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `promotional_media` converter assumes all incoming items are raw JSON dicts; consider making it idempotent (e.g., pass through existing `PromotionalMedia` instances) and extracting it into a named helper for clarity and reuse.
- In `PromotionalMedia`, the `description` field is optional but has no default; if `from_json` ever calls the regular initializer directly, this may raise a missing-argument error, so it could be safer to set `description: Optional[str] = field(default=None, ...)` to make the optionality explicit at the model level.
## Individual Comments
### Comment 1
<location> `tests/aws/test_models.py:172` </location>
<code_context>
+def test_promotional_resources_promotional_media(
+ promotional_media, promotional_resource: Dict[str, Any]
+) -> None:
+ promotional_resource["PromotionalMedia"] = promotional_media
+
+ obj = PromotionalResources.from_json(promotional_resource)
</code_context>
<issue_to_address>
**suggestion (testing):** Consider copying the `promotional_resource` fixture before mutating it in tests
Because this test mutates the shared fixture dict in place, it relies on the fixture remaining function-scoped and on no other test mutating it. To avoid future cross-test coupling or flaky tests if the scope changes, create and mutate a deep copy instead, e.g. `resource = copy.deepcopy(promotional_resource)`.
Suggested implementation:
```python
def test_promotional_resources_promotional_media(
promotional_media, promotional_resource: Dict[str, Any]
) -> None:
resource = copy.deepcopy(promotional_resource)
resource["PromotionalMedia"] = promotional_media
obj = PromotionalResources.from_json(resource)
assert obj
if not promotional_media:
assert obj.promotional_media is None
else:
assert obj.promotional_media == [PromotionalMedia.from_json(x) for x in promotional_media]
def test_promotional_resources_promotional_media_absent(promotional_resource: Dict[str, Any]):
resource = copy.deepcopy(promotional_resource)
del resource["PromotionalMedia"]
obj = PromotionalResources.from_json(resource)
assert obj
assert obj.promotional_media is None
```
1. Ensure `import copy` is present at the top of `tests/aws/test_models.py`, alongside the other imports:
- `import copy`
2. If there are existing tests that assert behavior when `"PromotionalMedia"` is absent, confirm that the new assertion `assert obj.promotional_media is None` is consistent with the model’s expected behavior. Adjust the assertion if the domain model uses a different sentinel (e.g. empty list) to represent absence.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This commit fixes a bug in the `PromotionalResources` which did expected the attribute `promotional_media` to be an optional string, while it should be an optional list of resources. It also introduces the class `PromotionalMedia` to map the missing resource type. Refers to SPSTRAT-671 Signed-off-by: Jonathan Gangi <jgangi@redhat.com>
JAVGan
force-pushed
the
aws_models_fix
branch
from
74b259e to
bf8151a
Compare
Collaborator
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
promotional_mediaconverter assumes every element is JSON-like and will break if a list ofPromotionalMediainstances is passed (e.g.,from_jsonor manual construction), so consider updating the converter to handle both dicts and preconstructedPromotionalMediaobjects safely. - Using
on_setattr=NO_OPonpromotional_mediaprevents any reassignment of this field after initialization; if the intent is only to avoid double conversion, a more targeted approach (e.g., a converter that is idempotent or checks types) would preserve mutability while avoiding reconversion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `promotional_media` converter assumes every element is JSON-like and will break if a list of `PromotionalMedia` instances is passed (e.g., `from_json` or manual construction), so consider updating the converter to handle both dicts and preconstructed `PromotionalMedia` objects safely.
- Using `on_setattr=NO_OP` on `promotional_media` prevents any reassignment of this field after initialization; if the intent is only to avoid double conversion, a more targeted approach (e.g., a converter that is idempotent or checks types) would preserve mutability while avoiding reconversion.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
1 participant
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.
This commit fixes a bug in the
PromotionalResourceswhich did expected the attributepromotional_mediato be an optional string, while it should be an optional list of resources.It also introduces the class
PromotionalMediato map the missing resource type.Refers to SPSTRAT-671
Summary by Sourcery
Fix the AWS PromotionalResources model to correctly represent promotional media as structured resources rather than a simple string.
New Features:
Bug Fixes:
Tests: