-
-
Notifications
You must be signed in to change notification settings - Fork 278
fix: copy client options instead of deepcopy #1130
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
base: main
Are you sure you want to change the base?
Conversation
…py thread objects, like I can't implement a config class that has a client instance of Supabase as a attribute
@AsahiSoftWareEngineer hope you find time to take a look here, it has total relation with your last week contribution #1095 |
Hi @rild-software, thanks for taking the time to fix this. Asking @silentworks for some guidance on how to better solve this issue, since the last fix didn't produce the desired effect. So let’s think again about how we should handle it this way, or is there a better way? |
I will look into this a bit more in the morning. Your example is a bit strange though as you have supabase inside of supabase. Normally if you are creating a storage driver to a database you would use direct database connection string with something like sqlalchemy or psycopg, you wouldn't be doing so with a Restful client library. |
Sure it's a odd architecture, but I don't think the "weirdness" of this architecture is relevant to this bug (it's a special situation when I wan't to minimize the libraries used and external resources, like relying only in supabase and supabase-py, a serverless minimal code/effort aproach). If the class accepts any StorageClass implementation it should also be able to construct it, no matter how it's implemented. Don't you think? |
any other specific implementation of a storage class should also face this bug if used a not deepcopyable object inside, like a supabase client instance (my example). |
Pull Request Test Coverage Report for Build 15524373424Details
💛 - Coveralls |
This is all looking good to me and I've tested it in my repo which uses Redis for storage and it is indeed working as expected. When I tested my repo with the currently released version (2.15.2) I do get the pickle error for my Redis storage class. |
Deepcopy fails to copy thread objects, like I can't implement a config class that has a client instance of Supabase as a attribute
What kind of change does this PR introduce?
Bug fix for this issue: #1129
What is the current behavior?
SyncClient.init receives a
option
argument that could be a ClientOptions, this ClientOptions instance can also have specific implementations for storage.Then, init tries to copy the options attribute to it's instance, since options.headers is a dictionary, a deepcopy garantees recursively that headers is copied too. But this drives to another kind's of unexpected erros since we don't have control of the implementation of any ClientOptions derived class.
The bug is:
You can reproduce my scenario by checking this gist:
https://gist.github.com/rild-software/ad23c35c49f64508cf2cc530e72b703f
What is the new behavior?
init method only copy the contents (not recurively) and then explicitly copy the headers.