Refactor storages and add StoreRegistry

[provinzkraut]

Content

This builds upon #1304.

• Add Starlite.cache_config attribute
• Rename starlite/storage > starlite/stores and update general naming to "store" instead of "storage"
• Add starlite/stores/registry.py module, implementing a StoreRegistry
• Add Starlite.stores attribute, containing a StoreRegistry
• Add stores kwarg to Starlite and AppConfig to allow seeding of the StoreRegistry
• Change RateLimitMiddleware to use app.storages
• Change request caching to use app.storages
• Change server side sessions to use app.storages
• Move starlite/cache/config/ to starlite/config/response_cache.py
• Rename CacheConfig > ResponseCacheConfig
• Rename Starlite.cache_config > response_cache_config
• Rename AppConfig.cache_config > response_cache_config
• Remove starlite/cache module
• Remove ASGIConnection.cache property
• Remove Starlite.cache attribute

---

This fundamentally changes how different parts of the app interact with stores, and in its current state, requires less configuration by default.

At the core of this pattern is the StoreRegistry, which provides a central places where stores are registered under a name. The current implementation is not yet complete, and some details still have to be worked out. Open questions at the bottom.

---

Examples

from starlite import Starlite

app = Starlite(...)
some_store = app.stores.get("some")
# we now have access to a newly created store instance. 
# This is provided by the registry via a factory function, 
# that gets called every time a `.get` call requests an unknown name.
assert app.stores.get("some") is some_store  # subsequent calls will return the registered instance

This allows configuration requiring very little boilerplate, but still offering access to the stores used:

from starlite import Starlite
from starlite.middleware.rate_limit import RateLimitConfig

app = Starlite(..., rate_limit_config=RateLimitConfig(("second", 1))))
# RateLimitMiddleware will request a store via app.stores.get("rate_limit")
app.stores.get("rate_limit")  # and  we can access the store directly without having to set it up

By default, the default factory will return a new instance of MemoryStore, but we can configure it ourselves.
In this example, we return the same MemoryStore instance every time.

from starlite import Starlite
from starlite.stores.memory import MemoryStore

from starlite.stores.registry import StoreRegistry

memory_store = MemoryStore()
app = Starlite(..., stores=StoreRegistry(default_factory=lambda _: memory_store))

This pattern can also be used to conveniently build a store hierarchy. This simple configuration will use the same underlying redis connection, while making use of the namespacing feature. Every time a new store is requested via StoreRegistry.get, that hasn't been registered yet, the registry will call root_store.with_namespace, to create a new namespaced RedisStore instance:

from starlite import Starlite, get
from starlite.stores.redis import RedisStore
from starlite.middleware.rate_limit import RateLimitConfig
from starlite.middleware.session.server_side import ServerSideSessionConfig
from starlite.stores.registry import StoreRegistry

root_store = RedisStore.with_client("redis://localhost")


@get(cache=True)
def cached_handler() -> str:
    # this will use app.stores.get("request_cache")
    return "Hello, world!"


app = Starlite(
    [cached_handler],
    stores=StoreRegistry(default_factory=root_store.with_namespace),
    middleware=[
        RateLimitConfig(("second", 1)).middleware,
        ServerSideSessionConfig().middleware,
    ],
)

the same can be done with a FileStore:

from pathlib import Path

from starlite import Starlite, get
from starlite.middleware.rate_limit import RateLimitConfig
from starlite.middleware.session.server_side import ServerSideSessionConfig
from starlite.stores.file import FileStore
from starlite.stores.registry import StoreRegistry

root_store = FileStore(Path("data"))


@get(cache=True)
def cached_handler() -> str:
    # this will use app.stores.get("request_cache")
    return "Hello, world!"


app = Starlite(
    [cached_handler],
    stores=StoreRegistry(default_factory=root_store.with_namespace),
    # the line above is essentially equivalent to
    # stores=StoreRegistry(default_factory=lambda name: FileStore(root_store.path / name)),
    middleware=[
        RateLimitConfig(("second", 1)).middleware,
        ServerSideSessionConfig().middleware
    ],
)

It is also possible to explicitly set up stores:

from pathlib import Path

from starlite import Starlite, get
from starlite.middleware.rate_limit import RateLimitConfig
from starlite.middleware.session.server_side import ServerSideSessionConfig
from starlite.stores.file import FileStore
from starlite.stores.redis import RedisStore
from starlite.stores.memory import MemoryStore

file_store = FileStore(Path("data"))
redis_store = RedisStore.with_client("redis://localhost")
memory_store = MemoryStore()


@get(cache=True)
def cached_handler() -> str:
    return "Hello, world!"


app = Starlite(
    [cached_handler],
    stores={
        "request_cache": redis_store,
        "sessions": file_store,
        "rate_limit": memory_store,
    },
    middleware=[
        RateLimitConfig(("second", 1)).middleware,
        ServerSideSessionConfig().middleware,
    ],
)

or by passing a store name to the configs, which also allows to easily re-use stores:

from pathlib import Path

from starlite import Starlite, get
from starlite.middleware.rate_limit import RateLimitConfig
from starlite.middleware.session.server_side import ServerSideSessionConfig
from starlite.stores.memory import MemoryStore


@get(cache=True)
def cached_handler() -> str:
    return "Hello, world!"


app = Starlite(
    [cached_handler],
    stores={"root": MemoryStore()},
    middleware=[
        RateLimitConfig(("second", 1), store="root").middleware,
        ServerSideSessionConfig(store="root").middleware,
    ],
    response_cache_config=ResponseCacheConfig(store="root")
)

---

Open questions

Should the store names used by various Starlite features be configurable or constant?
(This has since been decided: Store names are configurable for each integration)

Currently, for the sake of keeping this RFC simple, the names of the stores used by Starlite internally (e.g. by RateLimtiMiddleware are hardcoded.

A disadvantage of this is that it decouples the configuration value from where it's used. For example, there is aRateLimitConfig, but you can only specify which store it should use via another configuration value on the application.

app = Starlite(
    ..., 
    stores={"rate_limit": some_store, "session": some_store}, 
    rate_limit_config=RateLimitConfig(("second", 1)),
    middleware=[SServerSideSessionConfig().middleware]
)

If we were to add a store field to those, it could be instead configure like:

app = Starlite(
    ..., 
    stores={"some_store": some_store}, 
    rate_limit_config=RateLimitConfig(("second", 1), store="some_store"),
    middleware=[SServerSideSessionConfig(store="some_store").middleware]
)

Should users be forced to go through the registry?

As of now, the only way to configure a store for e.g. RateLimitMiddleware is by setting it via the stores kwarg to Starlite (e.g. Starlite(..., stores={"rate_limit": my_store}).

The benefit of this is that all stores will always be registered, making them accessible everywhere, and configuration relatively simple, since there's only one way to set things up.

Should the registry include a "default cache" property?

Currently, the registry includes a .default property, which is equivalent to registry.get("default"). This is intended to mimic the .cache property of the Starlite app / ASGIConnection, which this PR removes.

The issue with using a default cache like this is that it returns the same instance, occupying the same namespace. This is not desirable in most cases, and can lead to issue if users do not pay attention to this. If we do not offer such a property, we would instead promote the pattern of using registry.get(<my store>), to get a unique, namespaced store for each use case, while still allowing a user to override this behaviour (using the default_factory) if they so wish.

Especially for third party integrations that wish to make use of the stores provided by Starlite this pattern could prove very valuable, since no precautions have to be taken how to safely operate with a store without interfering with data not owned by this party.

If we don't offer a "default cache", we will also ensure that a user is always able to control which integration uses which store, via the registry.

Considerations

For convenience (connection.app.stores.get("foo") is quite complicated), we could proxy app.stores in these cases, like connection.cache does in its current implementation.

Should the registry be "frozen" after application startup?

Currently, you can register (and possibly override) stores at any time through the registry. This might lead to side effects if a store is acquired via registry.get("some store"), and later on overridden. We could add an option that prevents store overrides after application startup.

[Goldziher]

Ok, gotta say I'm a bit annoyed. I feel like I'm returning to the same points again and again.

One thing is to remove the cache abstraction, fine. But I'm really not on board with removing the access to the default store from request, app etc. I think there are a lot of over complications here, and am overall degradation of DX.

[provinzkraut]

Ok, gotta say I'm a bit annoyed. I feel like I'm returning to the same points again and again.

One thing is to remove the cache abstraction, fine. But I'm really not on board with removing the access to the default store from request, app etc. I think there are a lot of over complications here, and am overall degradation of DX.

Please wait until I'm done with the examples and reasoning. I specifically took care of addressing your particular concerns.

[Goldziher]

Ok, gotta say I'm a bit annoyed. I feel like I'm returning to the same points again and again.

One thing is to remove the cache abstraction, fine. But I'm really not on board with removing the access to the default store from request, app etc. I think there are a lot of over complications here, and am overall degradation of DX.

Please wait until I'm done with the examples and reasoning. I specifically took care of addressing your particular concerns.

Yup, i feel a bit stupid. Didn't see the description at all.

This is actually very good and a big step forward. I apologize for being an idiot.

All Looks good.

Regarding the open questions - I'd say yes, let's expose a store property on the configs.

[provinzkraut]

@Goldziher thanks for the consideration. Please excuse the snarky tone in my comments to your remarks, I was a bit taken aback by your initial comment. But no hard feelings from my side ❤️

Let me get through with the examples and such and then let's see where this takes us.

I'd say yes, let's expose a store property on the configs.

Well, I too have an opinion on that, but I'll get to that in a separate comment 😬

[provinzkraut]

My personal thoughts on the two open questions:

Should users be forced to go through the registry?

Yes. If we want to facilitate the patterns explained in the above section, we should make this non-optional. There is little to gain from adding a third way to define stores for an integration and it will only make things more confusing an complex. The current solution allows to express every configuration a user wishes. If they absolutely need to bypass the registry, they can still customize the configuration objects.

Should the registry include a "default cache" property?

I don't actually think I have anything to add there 🙃

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

97.0% Coverage
0.0% Duplication

[JacobCoffee]

looks great