Skip to content

add storage parameter to S3FileAdmin#2742

Open
samialfattani wants to merge 1 commit into
pallets-eco:masterfrom
samialfattani:fix/s3-storage
Open

add storage parameter to S3FileAdmin#2742
samialfattani wants to merge 1 commit into
pallets-eco:masterfrom
samialfattani:fix/s3-storage

Conversation

@samialfattani
Copy link
Copy Markdown
Contributor

This PR enables users to inherit the S3Storage and inject it in S3FileAdmin

@samialfattani samialfattani marked this pull request as ready for review January 14, 2026 06:31
@ElLorans
Copy link
Copy Markdown
Contributor

What is the advantage?

@samialfattani
Copy link
Copy Markdown
Contributor Author

What is the advantage?

One of the uses cases that if the user wants to add the Prefix functionality, then he needs to change the behavior of some functions in S3Storage class. so this PR enables him to do so by subclassing of S3Storage and override the functions that need to bo changed to support Prefix functionality

@ElLorans
Copy link
Copy Markdown
Contributor

I have never used file admin, but what happens here?

client_a = boto3.client('s3', region='us-east-1')
client_b = boto3.client('s3', region='eu-west-1')

storage = S3Storage(client_a, 'bucket-in-us')

# This is confusing - which client gets used?
admin = S3FileAdmin(client_b, 'bucket-in-eu', storage=storage)

should we rather

def __init__(
    self,
    s3_client: BaseClient | None = None,
    bucket_name: str | None = None,
    storage: S3Storage | None = None,
    *args: t.Any,
    **kwargs: t.Any,
) -> None:
    if storage is None:
        if s3_client is None or bucket_name is None:
            raise ValueError(
                "s3_client and bucket_name are required when storage is not provided"
            )
        storage = S3Storage(s3_client, bucket_name)
    elif s3_client is not None or bucket_name is not None:
        import warnings
        warnings.warn(
            "s3_client and bucket_name are ignored when storage is provided",
            UserWarning,
            stacklevel=2
        )
    
    super().__init__(*args, storage=storage, **kwargs)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants