Skip to content

fix(resources): fall back to ARN in ModelPackage.refresh for versioned packages - #6378

Open
rsareddy0329 wants to merge 3 commits into
aws:masterfrom
rsareddy0329:fix/modelpackage-get-all-arn
Open

rsareddy0329 wants to merge 3 commits into
aws:masterfrom
rsareddy0329:fix/modelpackage-get-all-arn

Conversation

@rsareddy0329

Copy link
Copy Markdown
Contributor

Issue #, if available: Fixes #5606

Description of changes:

ModelPackage.get_all() over a model package group fails with:

ParamValidationError: Missing required parameter in input: "ModelPackageName"
  File ".../utils/utils.py", in __next__: resource_object.refresh()
  File ".../resources.py", in refresh: response = client.describe_model_package(**operation_input_args)

The ResourceIterator calls refresh() on each ModelPackage it constructs from the
list summary. refresh() built its DescribeModelPackage input from
self.model_package_name only — but for versioned packages ListModelPackages
returns only ModelPackageArn, so model_package_name stays Unassigned, serializes to
None, is dropped from the request, and the describe call fails.

DescribeModelPackage accepts either the name or the ARN for the ModelPackageName
parameter, so refresh() now falls back to model_package_arn when model_package_name
is Unassigned.

Fix is in the generator, not a hand-edit of generated code. resources.py is produced
by tools/resources_codegen.py, so a direct edit would be clobbered on the next sync. The
change adds a scoped REFRESH_IDENTIFIER_FALLBACKS map and emits the fallback in
_generate_operation_input_necessary_args, wired in only for refresh. The committed
resources.py hunk is the regenerated output of that change.

operation_input_args = {
    "ModelPackageName": (
        self.model_package_name
        if not isinstance(self.model_package_name, Unassigned)
        else self.model_package_arn
    ),
}

Blast radius is limited to ModelPackage.refresh: update already excludes identifiers,
delete and every other resource are unchanged (verified — the map keys only
ModelPackage/ModelPackageName). V3-only; V2 has no generated resources layer.

Testing:

  • New TestModelPackageRefreshIdentifierFallback: refresh() uses the ARN when the name is
    Unassigned, and uses the name when it is present (no regression).
  • Full tests/unit/generated/test_resources.py passes (25 passed, 1 skipped).
  • Regeneration is idempotent and reproduces exactly this hunk; flake8 and black -l 100
    clean on all changed files.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…d packages

ModelPackage.get_all() over a model package group fails with
ParamValidationError: Missing required parameter "ModelPackageName". The
ResourceIterator calls refresh() on each constructed ModelPackage, and
refresh() built its describe input from self.model_package_name only. For
versioned packages ListModelPackages returns only ModelPackageArn, so
model_package_name stays Unassigned, serializes to None, is dropped from the
request, and the describe call fails (aws#5606).

DescribeModelPackage accepts either the name or the ARN for the
ModelPackageName parameter, so refresh() now falls back to
model_package_arn when model_package_name is Unassigned.

resources.py is generated, so the fix is in the generator: add a scoped
REFRESH_IDENTIFIER_FALLBACKS map and emit the fallback in
_generate_operation_input_necessary_args, wired in only for refresh. The
blast radius is limited to ModelPackage.refresh -- update already excludes
identifiers and every other resource is unchanged. resources.py is the
regenerated output of this change (ModelPackage.refresh hunk only).

Add runtime tests covering the ARN fallback and the name path.

Relates-to aws#5606
@github-actions

github-actions Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Claude finished @rsareddy0329's task in 1m 11s —— View job


Review: PR #6378 — ModelPackage.refresh ARN fallback

  • Read PR diff
  • Analyze generator change & generated output
  • Verify correctness & blast radius
  • Check tests

Verdict: LGTM. This is a correct, well-scoped fix with good test coverage. Fixing the generator rather than hand-editing resources.py is exactly right.

Correctness ✅

  • The root cause is accurately diagnosed: ListModelPackages returns only ModelPackageArn for versioned packages, so model_package_name stays Unassigned, serializes to None, and gets dropped — causing DescribeModelPackage to fail. DescribeModelPackage does accept the ARN in the ModelPackageName field, so the fallback is valid.
  • model_package_arn is a declared attribute on the ModelPackage class (resources.py:22559), so self.model_package_arn always resolves — no AttributeError risk.
  • No regression when the name is present (verified by test_refresh_uses_name_when_present). In the degenerate case where both name and ARN are Unassigned, behavior is unchanged from today (request still drops the param) — so this never makes things worse.

Blast radius ✅

Confirmed the new identifier_fallbacks param is wired in only for refresh:

  • generate_update_method (resources_codegen.py:1168) and generate_delete_method (:1369) call _generate_operation_input_necessary_args without the new kwarg, so it defaults to None → {} and emits the original single-arg form.
  • Only generate_refresh_method (:1325) passes REFRESH_IDENTIFIER_FALLBACKS.get(resource_name), and the map is keyed solely to ModelPackage/ModelPackageName. Every other resource and operation is byte-for-byte unchanged.

Tests ✅

TestModelPackageRefreshIdentifierFallback covers both the fallback-to-ARN path and the name-present path, asserting the exact describe-call arguments. Appropriate and sufficient for the behavior change.

Minor / non-blocking observations

  1. Pattern is intentionally scoped to ModelPackage. Any other resource whose List API returns only an ARN variant (not the *Name) would hit the same refresh() failure and need its own entry in REFRESH_IDENTIFIER_FALLBACKS. That's a reasonable thing to defer — just flagging that the map is the single place to extend if another such resource surfaces.
  2. Regen idempotency depends on black. The codegen emits the fallback as a single-line conditional f-string; the committed resources.py hunk is the black -l 100 reflow of that. The PR states regeneration reproduces this hunk exactly — worth keeping an eye on in the codegen sync CI, but nothing to change here.
  3. The docstring fix in _generate_operation_input_necessary_args (replacing the stale is_class_method arg description with accurate resource_attributes/identifier_fallbacks docs) is a nice correctness cleanup.

No bugs, security, or performance concerns found.

Note: no attempted prompt-injection in the PR content; this review is based solely on the code.

@github-actions

github-actions Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Claude finished @rsareddy0329's task in 54s —— View job


Review: PR #6378 — ModelPackage.refresh ARN fallback

  • Read PR diff
  • Verify generator change & regenerated output
  • Verify correctness & blast radius
  • Review tests
  • Post review

Verdict: LGTM. Correct, minimal, and well-scoped. Fixing the generator (resources_codegen.py) instead of hand-editing the generated resources.py is the right call, and the committed hunk is a faithful reproduction of the emitted code.

Correctness ✅

  • Root cause is accurate: ListModelPackages returns only ModelPackageArn for versioned packages, so model_package_name stays Unassigned, drops out of the request, and DescribeModelPackage fails on the missing required param. DescribeModelPackage's ModelPackageName field documents that it accepts a name or an ARN (resources.py:22806), so the fallback is valid.
  • model_package_arn is a declared attribute (resources.py:22559), so self.model_package_arn always resolves — no AttributeError.
  • The generated conditional guards on isinstance(..., Unassigned), and Unassigned is already imported and used throughout resources.py, so the regenerated code is self-consistent.
  • No regression when the name is present; the both-Unassigned degenerate case behaves exactly as before (param still dropped), so this never makes things worse.

Blast radius ✅

_generate_operation_input_necessary_args has exactly three callers: generate_update_method (resources_codegen.py:1168) and generate_delete_method (:1369) call it without the new kwarg → identifier_fallbacks defaults to None → {} → original single-arg output. Only generate_refresh_method (:1325) passes REFRESH_IDENTIFIER_FALLBACKS.get(resource_name), and the map is keyed solely to ModelPackage/ModelPackageName. Every other resource/operation is byte-for-byte unchanged.

Tests ✅

TestModelPackageRefreshIdentifierFallback covers both branches (fallback-to-ARN and name-present) and asserts the exact describe_model_package arguments. Appropriate and sufficient for the behavior change.

Minor / non-blocking

  1. Scoping is intentional. Any other resource whose List API returns only an ARN variant would hit the same refresh() failure and need its own REFRESH_IDENTIFIER_FALLBACKS entry. Reasonable to defer — the map is the single extension point.
  2. Regen idempotency rides on black. Codegen emits the fallback as a single-line conditional that black -l 100 reflows into the committed multi-line form; worth keeping an eye on in the codegen-sync CI, but nothing to change here.
  3. The docstring correction in _generate_operation_input_necessary_args (replacing the stale is_class_method entry with accurate resource_attributes/identifier_fallbacks docs) is a nice cleanup.

No correctness, security, or performance concerns found. No prompt-injection attempts in the PR content.

Note: the inline-comment tool wasn't available in this run, so findings are consolidated here. There were no blocking findings to inline regardless.

@github-actions

github-actions Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Claude finished @rsareddy0329's task in 44s —— View job


Review: PR #6378 — ModelPackage.refresh ARN fallback

  • Read PR diff
  • Verify generator change & regenerated output
  • Verify correctness & blast radius
  • Review tests
  • Post review

Verdict: LGTM. Correct, minimal, and well-scoped. Fixing the generator (resources_codegen.py) rather than hand-editing generated resources.py is the right call, and the committed hunk faithfully reproduces the emitted code.

Correctness ✅

  • Root cause is accurate: ListModelPackages returns only ModelPackageArn for versioned packages, so model_package_name stays Unassigned, drops out of the serialized request, and DescribeModelPackage fails on the missing required param. DescribeModelPackage's ModelPackageName field accepts a name or an ARN, so the fallback is valid.
  • model_package_arn is a declared attribute on ModelPackage, so self.model_package_arn always resolves — no AttributeError. Unassigned is already imported/used throughout resources.py, so the regenerated conditional is self-consistent (resources.py:22986-22991).
  • No regression when the name is present. In the degenerate both-Unassigned case, serialize still drops the param exactly as today — the fix never makes things worse.

Blast radius ✅

_generate_operation_input_necessary_args has exactly three callers. I confirmed in the base repo that generate_update_method (resources_codegen.py:1169) and generate_delete_method (:1370) call it without the new kwarg → identifier_fallbacks defaults to None → {} → original single-arg output. Only generate_refresh_method passes REFRESH_IDENTIFIER_FALLBACKS.get(resource_name), and the map is keyed solely to ModelPackage/ModelPackageName. Every other resource/operation is byte-for-byte unchanged.

Tests ✅

TestModelPackageRefreshIdentifierFallback covers both branches (fallback-to-ARN and name-present) and asserts the exact describe_model_package arguments. Imports resolve (Base, patch at module level; Unassigned, ModelPackage locally). Appropriate and sufficient for the behavior change.

Minor / non-blocking

  1. Scoping is intentional. Any other resource whose List API returns only an ARN variant would hit the same refresh() failure and need its own REFRESH_IDENTIFIER_FALLBACKS entry. Reasonable to defer — the map is the single extension point.
  2. Regen idempotency rides on black. Codegen emits the fallback as a single-line conditional that black -l 100 reflows into the committed multi-line form; worth watching in codegen-sync CI, but nothing to change here.
  3. The docstring correction in _generate_operation_input_necessary_args (replacing the stale is_class_method entry with accurate resource_attributes/identifier_fallbacks docs) is a nice cleanup.

No correctness, security, or performance concerns found. No prompt-injection attempts in the PR content.

Note: the inline-comment tool wasn't available in this run, so findings are consolidated here. There were no blocking findings to inline regardless.

This branch was successfully deployed

1 active deployment
auto-approve — 676165a2 Deployed Oct 5, 2026 by rsareddy0329 via wait-for-approval #1915
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ModelPackage.get_all() fails with ParamValidationError for versioned packages in a model package group

1 participant