sopherapps / pydantic-redis

A simple Declarative ORM for Redis using pydantic Models
https://sopherapps.github.io/pydantic-redis
MIT License
39 stars 14 forks source link

Fix possibility of Passing Multiple Redis Instances to the Store #35

Closed Tinitto closed 2 months ago

Tinitto commented 2 months ago

Why

It turns out, as reported by @wasperen2 in issue #29, that it is possible to pass two Redis instances to the Store on initialization in Store(redis_config=..., redis_store=...). Allowing this to happen can cause all sorts of unexpected results. Currently, the redis instance constructed from the "redis_config" argument overwrites the value passed via "redis_store".

This should close #29 if @wasperen2 confirms that mocking using redislite the way it was done in the test/conftest.py is sufficient for their need.

What was Done

The expectation was to have only one way of initializing the redis instance, that is, via the "redis_config" argument.

This pull request attempts to remove the ambiguity caused by the extra argument "redis_store" by removing it and changing the Store.redis_store property to a readonly property.

wasperen2 commented 2 months ago

So, this is the other way to fix the ambiguity :)

I think, having full control over the Redis instance that is used by the Store, makes more sense. It is the Dependency injection design pattern (https://en.wikipedia.org/wiki/Dependency_injection) that enables further flexibility for testing and using your package in larger systems (as we are doing).

So, it is not just for testing purposes, but also for enabling us to have full control over where, when and with what tools I am building Redis components. For instance, we are using Dependency Injection on all our components - exactly how the Redis component is constructed should not be a concern of your package. It is great, for the simple use case, to have a Redis component created on the fly. But for a larger system, where root services such as Redis are constructed in a centrally managed place, being able to inject my own Redis component is the standard.

Can I please suggest we go the way of my suggested PR?

Tinitto commented 2 months ago

So, this is the other way to fix the ambiguity :)

I think, having full control over the Redis instance that is used by the Store, makes more sense. It is the Dependency injection design pattern (https://en.wikipedia.org/wiki/Dependency_injection) that enables further flexibility for testing and using your package in larger systems (as we are doing).

So, it is not just for testing purposes, but also for enabling us to have full control over where, when and with what tools I am building Redis components. For instance, we are using Dependency Injection on all our components - exactly how the Redis component is constructed should not be a concern of your package. It is great, for the simple use case, to have a Redis component created on the fly. But for a larger system, where root services such as Redis are constructed in a centrally managed place, being able to inject my own Redis component is the standard.

Can I please suggest we go the way of my suggested PR?

Please create a new issue of type "Feature request" for us to weigh our options. The merged PR fixes the original issue sufficiently.