-
Notifications
You must be signed in to change notification settings - Fork 77
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
TaskAdmin +readonly_fields "created_at", "updated_at"; fix setup name #118
base: master
Are you sure you want to change the base?
Conversation
Dear @amureki , is project support are stopped/dead? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution!
I feel like, it'd be good to clarify some things here before we proceed; hence, I left some comments.
django_dramatiq/models.py
Outdated
try: | ||
return str(self.message) | ||
except Exception as e: | ||
return f'Failed to display Task: {e}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this is interesting.
Could you, please, provide some information regarding this?
What kind of exception did you get here and on which conditions?
If this is a valid case, and we'll decide to handle it - then we'd need to document this more and update a test suite to cover it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, it was a long time ago, the log has already been cleared. I remember that it was for various reasons:
- When I interfered with the models data.Task during migration
- Without my participation, maybe it was a bug in dramatiq.
- On incorrect using dramatiq actor.send, I have not saved example. I think it's somewhere in https://github.com/Bogdanp/dramatiq/issues
If I can find the info, I will definitely let you know. (or the dramatiq project)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you'd happen to find it, I am curious to see. 👍
And yeah, then we can check what'd be the best way to proceed - either in this library, or directly in dramatiq
. But without this knowledge, I am afraid, I would not proceed with these lines here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the broken data gets into the database, no matter how, then it actually breaks the admin web page. (we are dealing with bytes in BinaryField)
This edit will avoid 500 errors if the data was suddenly broken. I don't understand your skepticism, security doesn't suffer, performance doesn't suffer either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, if you felt scepticism in my comments - that was definitely not the intention.
This code might not suffer from security or performance issues, but this is not the point.
We start here, then we end up wrapping every __str__
or __repr__
functions for premature "safety".
But we don't want that, right?
We want to have every line of code meaning something and solving something. And we intend to ensure its function by adding a test case to make sure it won't happen again, and we won't forget about it.
Plus, I think, if we just catch all exceptions here - we'd start ignoring them (as it is kinda handled already, right?), we could accidentally ignore important symptoms leading to this error. So if there is something wrong with bytes in message
property - we should handle that bit and not try-catch
every place that uses message
.
And this is what I am missing here - I do not know the real case that this code change will solve, but it certainly introduces a different behaviour (even though it is a small one).
I hope you could follow me here, and I am sorry, if you still would not agree with my thinking process here, but I truly believe this is only for the good of the project.
Again, I am looking forward to understanding the underlying issue and let's try to solve it in its roots!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could accidentally ignore important symptoms leading to this error
- I absolutely agree, I added a message to the log.
The key feature here is that the database cannot guarantee us the encodability of a BinaryField data.
Any byte error will result in 500 at admin web page, no matter how it got into the database.
And I finally found the trace!
I then changed some dramatic settings in the project, but I didn’t log which ones :D
2022-03-10T16:01:44.770354+05:00 job1.stage.int.<mywork>.ru django: Failed to decode message using encoder .
Traceback (most recent call last):
File "/home/int/.pyenv/versions/3.9.7/envs/int-3.9.7/lib/python3.9/site-packages/dramatiq/message.py", line 102, in decode
return cls(**global_encoder.decode(data))
_pickle.UnpicklingError: invalid load key, '_'.
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "/home/int/.pyenv/versions/3.9.7/envs/int-3.9.7/lib/python3.9/site-packages/dramatiq/brokers/rabbitmq.py", line 511, in __next__
message = Message.decode(body)
File "/home/int/.pyenv/versions/3.9.7/envs/int-3.9.7/lib/python3.9/site-packages/dramatiq/message.py", line 104, in decode
raise DecodeError("Failed to decode message.", data, e) from e
dramatiq.errors.DecodeError: Failed to decode message.
hey @ikvk ! It remains minimally maintained, as continuous support of upcoming Python and Django versions, but the reviews are limited (as I'm sadly struggling with finding the time and strength for it). We are still grateful for the interest and opened PRs, I'll try to deal with them, but please do not expect instant feedback 😅. |
Also I suggest to implement white and black lists: Do you agree to this? I cat put it here or at next MR. |
I'd suggest opening an issue here with a bit more details, as from this description I am not sure, I did follow what exactly you are proposing here. |
…Task; logger var lo lower case
ikvk/imap_tools@f1b2614#diff-60f61ab7a8d1910d86d9fda2261620314edcae5894d5aaa236b821c7256badd7R16