pallets-eco / flask-session

Server side session extension for Flask
https://flask-session.readthedocs.io
BSD 3-Clause "New" or "Revised" License
488 stars 236 forks source link

implemented DynamoDBSessionInterface and tests. #214

Closed necat1 closed 3 months ago

Lxstr commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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
necat1 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

necat1 commented 4 months ago

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

Lxstr commented 3 months ago

@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 commented 3 months ago

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