Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

Tinitto
Copy link
Contributor

@Tinitto Tinitto commented Jun 24, 2024

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.

@Tinitto Tinitto merged commit 3d7d960 into master Jul 1, 2024
6 checks passed
@wasperen2
Copy link

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
Copy link
Contributor Author

Tinitto commented Jul 5, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

using any passed Redis instance
2 participants