Skip to content
This repository was archived by the owner on Mar 23, 2026. It is now read-only.

S3: improve typing, removing int dict keys, clean up backwards getattr#13687

Merged
bentsku merged 5 commits into
mainfrom
s3-store-avro
Feb 4, 2026
Merged

S3: improve typing, removing int dict keys, clean up backwards getattr#13687
bentsku merged 5 commits into
mainfrom
s3-store-avro

Conversation

@bentsku

@bentsku bentsku commented Feb 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Motivation

This PR implements type hints for the S3 store and attributes that are stored in it.

One issue that needed to be addressed is that Avro does not support int-based dict keys, so multiparts have been reworked.

I've also took the opportunity to remove all the hacks around backward compatibility via getattr.

Changes

  • add type hints to VersionedKeyStore and KeyStore
  • use string for every dict containing Parts
  • remove all workaround with getattr

Tests

Related

@bentsku bentsku added this to the 4.14 milestone Feb 3, 2026
@bentsku bentsku self-assigned this Feb 3, 2026
@bentsku bentsku added aws:s3 Amazon Simple Storage Service semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases docs: skip Pull request does not require documentation changes notes: skip Pull request does not have to be mentioned in the release notes labels Feb 3, 2026
@github-actions

github-actions Bot commented Feb 3, 2026 •
edited
Loading

Copy link
Copy Markdown

S3 Image Test Results (AMD64 / ARM64)

    2 files  ±0      2 suites  ±0   8m 41s ⏱️ +5s
  559 tests ±0    507 ✅ ±0   52 💤 ±0  0 ❌ ±0 
1 118 runs  ±0  1 014 ✅ ±0  104 💤 ±0  0 ❌ ±0 

Results for commit e3b6696. ± Comparison against base commit 706ea55.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Feb 3, 2026 •
edited
Loading

Copy link
Copy Markdown

Test Results - Preflight, Unit

23 114 tests  ±0   21 255 ✅ ±0   6m 12s ⏱️ ±0s
     1 suites ±0    1 859 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit e3b6696. ± Comparison against base commit 706ea55.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Feb 3, 2026 •
edited
Loading

Copy link
Copy Markdown

Test Results (amd64) - Acceptance

7 tests  ±0   5 ✅ ±0   3m 5s ⏱️ -3s
1 suites ±0   2 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit e3b6696. ± Comparison against base commit 706ea55.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Feb 3, 2026 •
edited
Loading

Copy link
Copy Markdown

Test Results (amd64) - Integration, Bootstrap

    5 files      5 suites   1h 38m 41s ⏱️
2 075 tests 1 909 ✅ 166 💤 0 ❌
2 081 runs  1 909 ✅ 172 💤 0 ❌

Results for commit e3b6696.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Feb 3, 2026 •
edited
Loading

Copy link
Copy Markdown

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   1h 14m 13s ⏱️ - 44m 34s
2 051 tests  - 3 138  1 881 ✅  - 2 917  170 💤  - 221  0 ❌ ±0 
2 053 runs   - 3 138  1 881 ✅  - 2 917  172 💤  - 221  0 ❌ ±0 

Results for commit e3b6696. ± Comparison against base commit 706ea55.

This pull request removes 3138 tests. 
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

♻️ This comment has been updated with latest results.

@bentsku bentsku requested a review from giograno February 3, 2026 15:43
@bentsku bentsku marked this pull request as ready for review February 3, 2026 15:43
@bentsku bentsku requested review from aidehn and k-a-il as code owners February 3, 2026 15:43
@bentsku bentsku marked this pull request as draft February 3, 2026 15:43
@bentsku bentsku marked this pull request as ready for review February 4, 2026 01:52
aidehn
aidehn approved these changes Feb 4, 2026
View reviewed changes

@aidehn aidehn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just some questions around default values but nothing blocking - also it's a shame that Avro doesn't support int-based keys so we need to convert between a string and int for the parts 😭

Out of curiosity, how does it behave if something does have int-based keys? Does it silently fail or does it raise an error? If it's the prior it would be good to have some sort of mention of this somewhere or have some sort of safe-guarding around this to prevent this from happening as I can imagine it will cause some very confusing bugs (but I imagine this has already been considered / in the works!)

Comment thread localstack-core/localstack/services/s3/provider.py
Comment thread localstack-core/localstack/services/s3/provider.py
bentsku
bentsku commented Feb 4, 2026
View reviewed changes

@bentsku bentsku left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, how does it behave if something does have int-based keys? Does it silently fail or does it raise an error? If it's the prior it would be good to have some sort of mention of this somewhere or have some sort of safe-guarding around this to prevent this from happening as I can imagine it will cause some very confusing bugs (but I imagine this has already been considered / in the works!)

Yes, there will be a CI step happening early to warn you that this would be an incompatible type 👍

Comment thread localstack-core/localstack/services/s3/provider.py
@bentsku bentsku merged commit d2b4bb7 into main Feb 4, 2026
49 checks passed
@bentsku bentsku deleted the s3-store-avro branch February 4, 2026 15:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@giograno giograno Awaiting requested review from giograno

@k-a-il k-a-il Awaiting requested review from k-a-il k-a-il is a code owner

1 more reviewer

@aidehn aidehn aidehn approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@bentsku bentsku

Labels

aws:s3 Amazon Simple Storage Service docs: skip Pull request does not require documentation changes notes: skip Pull request does not have to be mentioned in the release notes semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases

Projects

None yet

Milestone

4.14

Development

Successfully merging this pull request may close these issues.

2 participants

@bentsku @aidehn