Use deepcopy instead of a shallow copy for middlewares, #2349 #2354
Conversation
|
I'm not sure that is the best way. As far as I understand - you want this to allow middleware developer to change values of data in HIS handler. For other cases that is not needed and will just make overhead. Is it more reasonable to "say" developers who want to change data values in HIS handler to make deepcopy BEFORE make changes?... I'm not against it. Deepcopy is more safe way, but I'm in a humble because I'm not sure that LIBRARY should care about it... |
|
Well, suppose the dev is adding some data, for post_process. |
| @@ -558,7 +558,7 @@ async def _run_middlewares_and_handlers(self, message, handlers, middlewares, up | |||
| logger.error("It is not allowed to pass data and values inside data to the handler. Check your handler: {}".format(handler['function'])) | |||
| return | |||
| else: | |||
| data_copy = data.copy() | |||
| data_copy = copy.deepcopy(data) | |||
There was a problem hiding this comment.
You should no copy values inside data. It is required to pass original objects retrieved from middlewares to handlers. They can be mutated and it is expected behavior. That is how you pass objectes to handlers instead of global variables
|
@Badiboy sorry for the inconvenience, we might need to revert this PR |
|
Or maybe let's wait for me to get back to work, I'll be available to check disadvantages of this again on Sunday |
|
I think it may wait for Sunday. |
|
As I said from the very beginnig: you are trying to think about all developer's needs yourself. It not looks like a good practice, because in most cases you cannot even imagine all cases how the library is used by all hundreds of thousands of developers. That's why I always propose as much flexibility for developers as it possible. In this case I still think it's better to leave decision about making deepcopy to developer of middleware... |
#2349