implemented DynamoDBSessionInterface and tests.

[necat1]

No description provided.

[Lxstr]

Thanks for the continued efforts. I think it would be best to not have URL parameter SESSION_DYNAMODB_URL if possible because none of the other backends use it, and it can be used directly in the boto client. Having the table one is good though.

[Lxstr]

Also something like this before self.store = client.Table(table_name)?

I'm not sure best way to code this

        # Check if the table exists
        existing_tables = self.client.meta.client.list_tables()["TableNames"]
        if table_name not in existing_tables:
            # Create the table if it does not exist
            self.client.create_table............

[Lxstr]

Also regarding this part, why are you passing on attribute error? I would think we want to know if any issues with initialization?

        try:
            client.meta.client.update_time_to_live(
                TableName=self.table_name,
                TimeToLiveSpecification={
                    "Enabled": True,
                    "AttributeName": "expiration",
                },
            )
        except AttributeError:
            pass

[Lxstr]

Need to use .get("Value") so no exception is raised

[Lxstr]

Also do we need

                region_name="us-west-2",
                aws_access_key_id="dummy",
                aws_secret_access_key="dummy",

[necat1]

Also regarding this part, why are you passing on attribute error? I would think we want to know if any issues with initialization?

        try:
            client.meta.client.update_time_to_live(
                TableName=self.table_name,
                TimeToLiveSpecification={
                    "Enabled": True,
                    "AttributeName": "expiration",
                },
            )
        except AttributeError:
            pass

When TTL index is already added to table dynamodb client raises an error next time its called, and i don't know a way to explixitly check if index is added, although i can do some research.

[necat1]

Also something like this before self.store = client.Table(table_name)?

I'm not sure best way to code this

        # Check if the table exists
        existing_tables = self.client.meta.client.list_tables()["TableNames"]
        if table_name not in existing_tables:
            # Create the table if it does not exist
            self.client.create_table............

I will look into this, i think boto3 supports this.

[necat1]

Thanks for the continued efforts. I think it would be best to not have URL parameter SESSION_DYNAMODB_URL if possible because none of the other backends use it, and it can be used directly in the boto client. Having the table one is good though.

Thanks for comprehensive review. boto3 doesn't have default connections. The alternative here is to have hardcoded URL when user doesn't provide client.

[necat1]

Thanks for the continued efforts. I think it would be best to not have URL parameter SESSION_DYNAMODB_URL if possible because none of the other backends use it, and it can be used directly in the boto client. Having the table one is good though.

Thanks for comprehensive review. boto3 doesn't have default connections. The alternative here is to have hardcoded URL when user doesn't provide client.

Or maybe just use default value from defaults without parametrizing SessionInterface?

[Lxstr]

Nice

[Lxstr]

Thanks for the continued efforts. I think it would be best to not have URL parameter SESSION_DYNAMODB_URL if possible because none of the other backends use it, and it can be used directly in the boto client. Having the table one is good though.

Thanks for comprehensive review. boto3 doesn't have default connections. The alternative here is to have hardcoded URL when user doesn't provide client.

I mean instead you should pass the instance like SESSION_DYNAMODB = boto3.resource(
"dynamodb",
endpoint_url="http://localhost:8000", etc)

[Lxstr]

Also the CI tests are working, maybe add

name: Run unittests
on: [push, pull_request]
jobs:
    unittests:
        runs-on: ubuntu-latest
        services:
            mongodb:
                image: mongo
                ports:
                    - 27017:27017
            dynamodb:
                image: amazon/dynamodb-local
                ports:
                    - 8000:8000
        steps:
...

[Lxstr]

Thanks for the continued efforts. I think it would be best to not have URL parameter SESSION_DYNAMODB_URL if possible because none of the other backends use it, and it can be used directly in the boto client. Having the table one is good though.

Thanks for comprehensive review. boto3 doesn't have default connections. The alternative here is to have hardcoded URL when user doesn't provide client.

I mean instead you should pass the instance like SESSION_DYNAMODB = boto3.resource( "dynamodb", endpoint_url="http://localhost:8000", etc)

Yeah long story short, I think it's fine to just have the port 800 hardcoded into the default client and remove url: str = Defaults.SESSION_DYNAMODB_URL. This will make it the same as all of the other backends.

[Lxstr]

Also lets compare to cachelib backend, which will be very similar: https://github.com/pallets-eco/cachelib/blob/main/src/cachelib/dynamodb.py

[necat1]

Great suggestions! I will have time to work on this on weekend.

[Lxstr]

@necat1 I noticed the use BillingMode="PAY_PER_REQUEST", instead of throughput in cachelib, should we do that?

They also use table.wait_until_exists() and table.load(). Not sure if we need those.

https://github.com/pallets-eco/cachelib/blob/main/src/cachelib/dynamodb.py

[Lxstr]

I'm thinking we don't need either

BillingMode="PAY_PER_REQUEST"

or

        read_capacity: int = Defaults.SESSION_DYNAMODB_READ,
        write_capacity: int = Defaults.SESSION_DYNAMODB_WRITE,

The user could switch using AWS Management Console, AWS CLI, or programatically

dynamodb.update_table(
        TableName=table_name,
        BillingMode='PAY_PER_REQUEST'
    )

This would simplify things a fair bit for us

[MauriceBrg]

I think there may be a problem here:

Scenario:

• Running inside a real environment, trying to use existing table.
• The instance or whatever is running this code doesn't have dynamodb:CreateTable or dynamodb:UpdateTimeToLive or dynamodb:Describe/GetTable permissions
• I'd expect an access denied error in this case, because it doesn't even get to the point of checking if the table exists etc.
• This except probably doesn't handle that.

This means we'd have to give whichever resource is running this permission to use the CreateTable etc. APIs even though we want to use an existing table. Not ideal from a security perspective.

(Purely theoretical at this point, but I'm fairly certain this is what would happen)

[Lxstr]

Fair point, seems more reasonable to check if the table exists before the try, is that what you would suggest? Could you add a PR to development?

[MauriceBrg]

From a security perspective I'd prefer that the implementation can work with minimal permissions, in this case:

• dynamodb:DeleteItem
• dynamodb:GetItem
• dynamodb:UpdateItem

This would be sufficient for read and write access to a properly configured table.

All this table and TTL management is something that we usually like to handle through Infrastructure as Code. Many companies also require additional configuration regarding backups, etc.

Having the option of doing that through flask-session is nice for convenience, but IMO a bit too much magic that happens by default.

I'd prefer an create_table flag or something like that, which is should be disabled by default.
If we find a good approach that balances different requirements I may be able to add a PR.

---

Got distracted and forgot the middle part :D

I'd prefer to handle the non-existence of a table when we try to write to it/read from it. The ask for forgiveness instead of approval approach - that requires fewer API calls and AWS permissions.

[Lxstr]

I see what you mean. I think the first step would be to factor out the creation into a single method on each interface. Similar to what is done here https://github.com/kroketio/quart-session/blob/master/quart_session/sessions.py.This could then be easily subclassed to skip the method (maybe like 3 lines of code).

The second step would be adding the config setting for all interfaces, which I am less sure of (adding more settings) unless there is a bit of demand or discussing with the team.

I think if the first was done neatly it would be merged quickly.

[MauriceBrg]

I'm currently looking into this, but I'm not super familiar with the codebase yet.

My approach would be to:

1. Refactor DynamoDBSessionInterface to have the create table logic as a separate function
2. Create a subclass ExistingDynamoDBTableSessionInterface that skips the create logic
3. Add a SESSION_TYPE existing_dynamodb_table to flask_session/__init__.py that uses 2)
4. Tests, obviously

This would result in there being two DynamoDB varieties for the SESSION_TYPE.
That would make this implementation different from the others.

I'm considering another optional flag to the constructor of DynamoDBSessionInterface, something like table_already_exists (Default: False), which would skip the create function.
This would probably require another DynamoDB parameter, e.g. SESSION_DYNAMODB_TABLE_ALREADY_EXISTS.

Not sure what is a better approach, any thoughts @Lxstr ?

[Lxstr]

I think we can simplify a bit

Step 1 is good.

This is already done for postgresql (impending PR) but would also need to be done for SQLAlchemy and the base class

Steps 2 and 3 can be instead adding the flag SESSION_TABLE_EXISTS

Base class should look something like:

class ServerSideSessionInterface(FlaskSessionInterface, ABC):
    ...

    def __init__(... session_table_exists: Optional[bool] = Defaults.SESSION_TABLE_EXISTS,):
        ...
        if not session_table_exists:
            self._create_schema_and_table()

    def _create_schema_and_table():
        pass

Tests would be great

Does that make sense?

[MauriceBrg]

Thanks, looks good to me, I've added a variation of that as a PR: #237