feat: implement mock for S3Tables service #8470

felixscherz · 2025-01-05T15:41:42Z

Hi, this is in regards to #8422.

I used scripts/scaffold.py to setup the s3tables service. I had to modify some URL patterns for the S3 service, as they overlap with s3tables.

Supported methods:

Currently working on supporting the new type of S3 buckets within the existing S3 backend so that data can be written to/read from S3 tables.

codecov · 2025-01-05T16:05:31Z

Codecov Report

Attention: Patch coverage is 92.66504% with 30 lines in your changes missing coverage. Please review.

Project coverage is 92.63%. Comparing base (cfaa898) to head (c7888a1).
Report is 14 commits behind head on master.

Files with missing lines	Patch %	Lines
moto/s3tables/models.py	90.54%	19 Missing ⚠️
moto/s3tables/exceptions.py	89.47%	6 Missing ⚠️
moto/s3/models.py	76.19%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8470      +/-   ##
==========================================
- Coverage   92.63%   92.63%   -0.01%     
==========================================
  Files        1224     1229       +5     
  Lines      105922   106394     +472     
==========================================
+ Hits        98120    98553     +433     
- Misses       7802     7841      +39

Flag	Coverage Δ
servertests	`27.91% <69.68%> (+0.16%)`	⬆️
unittests	`92.60% <92.66%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bblommers

Hi @felixscherz, thank you for the PR! Please see my comments. There's quite a few, but I think they're all minor - let me know if you have any questions.

bblommers · 2025-01-20T20:00:19Z

tests/test_s3tables/test_s3tables.py

+    resp = client.get_table_metadata_location(
+        tableBucketARN=arn, namespace="bar", name="baz"
+    )
+    assert "metadataLocation" in resp


Can we use assert metadataLocation == "s3://abc" here? Or is the exact value different from what we provided?

It should be exactly that. Your comment had me check the actual AWS implementation again however. They validate the metadata location to make sure it is part of the storage bucket assigned to the table, so I am going to add this check and then update the test as well:)

bblommers · 2025-01-20T20:01:35Z

tests/test_s3tables/test_s3tables.py

+    )
+    token = resp["versionToken"]
+
+    with pytest.raises(client.exceptions.ConflictException):


Would be useful to also match on the error, to verify we're throwing the correct error here

bblommers · 2025-01-20T20:01:54Z

tests/test_s3tables/test_s3tables.py

+    s3 = boto3.client("s3", region_name="us-east-2")
+
+    bucket_name = resp["warehouseLocation"].replace("s3://", "")
+    with pytest.raises(s3.exceptions.ClientError):


Same here for error matching

bblommers · 2025-01-20T20:02:00Z

tests/test_s3tables/test_s3tables.py

+
+    bucket_name = resp["warehouseLocation"].replace("s3://", "")
+    s3.put_object(Bucket=bucket_name, Key="test", Body=b"{}")
+    with pytest.raises(s3.exceptions.ClientError):


Same here for error matching

bblommers · 2025-01-20T20:07:13Z

moto/s3/models.py

@@ -1876,13 +1884,25 @@ def create_bucket(self, bucket_name: str, region_name: str) -> FakeBucket:

        return new_bucket

+    def create_table_storage_bucket(self, region_name: str) -> FakeTableStorageBucket:
+        # every s3 table is assigned a unique s3 bucket with a random name
+        bucket_name = f"{str(uuid4())}--table-s3"


We should use random.uuid4 here, to ensure the names are deterministic (see the docs)

bblommers · 2025-01-20T20:10:51Z

moto/s3tables/models.py

+from base64 import b64decode, b64encode
+from hashlib import md5
+from typing import Dict, List, Literal, Optional, Tuple, Union
+from uuid import uuid4


Same here - we should be using from moto.moto_api._internal import mock_random as random; random.uuid4()

I changed both occurrences to use the moto API

bblommers · 2025-01-20T20:12:53Z

moto/s3tables/models.py

+
+        return new_table_bucket
+
+    def list_table_buckets(


Can we use the @paginate-decorator here? That should simplify the implementation.

See the docs: https://docs.getmoto.org/en/latest/docs/contributing/development_tips/utilities.html#paginator

Yes that seems a lot cleaner! I changed all the list endpoints accordingly

bblommers · 2025-01-20T20:14:09Z

moto/s3tables/models.py

+        bucket.namespaces[ns.name] = ns
+        return ns
+
+    def list_namespaces(


Same here - ideally we paginate using the decorator, instead of rolling our own logic

bblommers · 2025-01-20T20:21:22Z

moto/s3/models.py

@@ -2644,6 +2677,8 @@ def list_objects(

        MOTO_S3_DEFAULT_MAX_KEYS=5
        """
+        if isinstance(bucket, FakeTableStorageBucket):


Does AWS throw the same error here?

It seems like a useful feature to let users look at the actual data that was stored, but this is the first time that I'm looking at this service, so maybe I'm missing something obvious 🙂

AWS does throw the same error, I think it has to do with the way they want users to use the service i.e. not rely on list operations. They also disable delete_object which raises the same exception. I think its important to replicate this behaviour since AWS not supporting it can throw a lot of people off:)

bblommers · 2025-01-20T20:21:59Z

moto/s3/models.py

+                    key.dispose()
+            for part in bucket.multiparts.values():
+                part.dispose()
+        s3_backends.bucket_accounts.pop(bucket_name, None)


This doesn't seem necessary - we're never adding the bucket to bucket_accounts

The bucket is added to bucket_accounts as part of the FakeBucket.__init__ method that's why we're removing it here. It does need to be part of bucket_accounts to be able to lookup the actual account the bucket is in I think.

felixscherz · 2025-01-20T20:55:11Z

Thank you for the review! I'll start addressing your comments:)

felixscherz mentioned this pull request Jan 5, 2025

feat: support S3 Table Buckets with S3TablesCatalog apache/iceberg-python#1429

Open

bblommers linked an issue Jan 5, 2025 that may be closed by this pull request

Feat: Support for S3 Tables API #8422

Open

felixscherz added 27 commits January 17, 2025 10:16

feat: modify backend_index negative lookahead

d803f46

feat: explicitly set s3tables regions

b1e52b1

feat: implement create_table_bucket

14d7093

feat: add scaffolding for other table_bucket methods

c57f3b9

fix: add s3tables to typing

3e23356

fix: update s3 urls to not overlap with s3tables

55821cd

feat: implement list_table_buckets

9ed68e9

feat: implement get_table_bucket

32514de

feat: implement delete_table_bucket

39e0fa3

chore: run formatter

98d928a

chore: add type hints

2d0cffe

fix: no longer return empty continuationToken

dde45d0

refactor: move response formatting out of models

8ad9d5a

feat: validate table bucket name

8068329

fix: response headers

2403717

chore: format

f742e86

chore: add types

7790f6d

fix: clean up path variables

641b3aa

fix: change url patterns to work wiht slashes in path

460b282

chore: remove comments

ac04831

feat: scaffold namespace operations

ea1a555

feat: implement create_namespace

c0ba657

feat: implement get_namespace

d038686

feat: implement list_namespaces

76584a5

feat: implement delete_namespace

9e3c0a5

chore: format

db550e1

chore: add type hints

a817c9b

felixscherz added 16 commits January 17, 2025 10:16

test: use random bucket names for tests

659da73

fix: change how to handle path variables

19b54da

feat: implement create_table

3d4cf86

feat: implement get_table

0be049f

feat: implement list_tables

d8a13f7

feat: implement delete_table

21da908

chore: format

6e7d1bf

fix: url handling for tables

67d93e5

feat: start modeling s3 tables

9ff07e4

feat: create s3 buckets when creating s3 tables

74df3e2

feat: implement get/update_table_metadata_location

8916bed

test: write metadata to underlying s3 bucket

0e24ad2

fix: type hints for python 3.8

57f56cd

feat: improve excpetion handling

d187aed

feat: improve handling of s3 table storage

3b5ca32

feat: disable specific s3 methods for table storage buckets

6752429

felixscherz force-pushed the feat/s3tables branch from 16f75a4 to 6752429 Compare January 17, 2025 09:16

felixscherz added 2 commits January 18, 2025 18:28

feat: implement rename_table

04d616d

docs: document implemented services

968cc5b

felixscherz marked this pull request as ready for review January 18, 2025 17:34

felixscherz added 4 commits January 18, 2025 22:46

fix: urls for rename_table

e5ef686

test: assert delete_table raises on version_token mismatch

a77ca1d

feat: simplify creating underlying s3 table storage buckets

4cd5c73

chore: run formatter

04b9fb7

bblommers requested changes Jan 20, 2025

View reviewed changes

felixscherz added 4 commits January 20, 2025 22:18

refactor: use moto pagination decorator

fdafeb8

fix: rely on internal random.uuid4 instead of uuid.uuid4

60670e8

feat: check metadata location before updating it

daa0f42

test: validate error messages

c7888a1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement mock for S3Tables service #8470

feat: implement mock for S3Tables service #8470

felixscherz commented Jan 5, 2025 •

edited

Loading

codecov bot commented Jan 5, 2025 •

edited

Loading

bblommers left a comment

bblommers Jan 20, 2025

felixscherz Jan 20, 2025

bblommers Jan 20, 2025

bblommers Jan 20, 2025

bblommers Jan 20, 2025

bblommers Jan 20, 2025

bblommers Jan 20, 2025

felixscherz Jan 20, 2025

bblommers Jan 20, 2025

felixscherz Jan 20, 2025

bblommers Jan 20, 2025

bblommers Jan 20, 2025

felixscherz Jan 20, 2025

bblommers Jan 20, 2025

felixscherz Jan 20, 2025

felixscherz commented Jan 20, 2025

feat: implement mock for S3Tables service #8470

Are you sure you want to change the base?

feat: implement mock for S3Tables service #8470

Conversation

felixscherz commented Jan 5, 2025 • edited Loading

codecov bot commented Jan 5, 2025 • edited Loading

Codecov Report

bblommers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixscherz commented Jan 20, 2025

felixscherz commented Jan 5, 2025 •

edited

Loading

codecov bot commented Jan 5, 2025 •

edited

Loading