Refactor way of working with RQ meta #9082
Conversation
|
❌ Some checks failed |
|
❌ Some checks failed |
Marishka17
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #9082 +/- ##
===========================================
+ Coverage 73.83% 73.89% +0.06%
===========================================
Files 431 432 +1
Lines 44807 44984 +177
Branches 3892 3892
===========================================
+ Hits 33084 33243 +159
- Misses 11723 11741 +18
|
Marishka17
changed the title
Marishka17
force-pushed
the
mk/refactor_working_with_rq_meta
branch
from
3348a62 to
7ea0523
Compare
Marishka17
changed the title
Marishka17
force-pushed
the
mk/refactor_working_with_rq_meta
branch
from
b99d1b2 to
9ce85ac
Compare
Marishka17
changed the title
|
| pass | ||
|
|
||
|
|
||
| class UserRQMetaAttribute(ImmutableRQMetaAttribute): |
There was a problem hiding this comment.
I don't think you're actually getting anything from the parent class here. This class could be reduced to:
class UserRQMetaAttribute:
def __get__(self, instance: WithMeta, objtype: type[WithMeta] | None = None):
return UserMeta(instance.meta[RQJobMetaField.USER])(In fact, I think it could even be replaced with a property in BaseRQMeta.)
| pass | ||
|
|
||
|
|
||
| class MutableRQMetaAttribute(_GettableRQMetaAttribute, _SettableRQMetaAttribute): |
There was a problem hiding this comment.
It doesn't seem like 5 classes are needed here. They could be combined into two - ImmutableRQMetaAttribute (merged _AbstractRQMetaAttribute and _GettableRQMetaAttribute), and MutableRQMetaAttribute (same as _SettableRQMetaAttribute, but inheriting from ImmutableRQMetaAttribute).
| RQJobMetaField.UserField.EMAIL, validator=lambda x: isinstance(x, str) | ||
| ) | ||
|
|
||
| def __init__(self, meta: dict[RQJobMetaField.UserField, Any]) -> None: |
There was a problem hiding this comment.
This type annotation is incorrect, because e.g. UserField.ID is not an instance of UserField.
| def to_dict(self): | ||
| return self.meta |
| ) -> None: | ||
| assert validator is None or callable(validator), "validator must be callable" | ||
| self._key = key | ||
| self._validator = validator |
There was a problem hiding this comment.
The validator parameter ought to be moved to _SettableRQMetaAttribute, since it doesn't do anything in an immutable attribute.
| if value is None and not self._optional: | ||
| raise ValueError(f"{self._key} is required") | ||
| if value is not None and self._validator and not self._validator(value): | ||
| raise ValueError("Wrong type") |
There was a problem hiding this comment.
IMO, if the validator can only signal that the type is wrong, then you might as well replace the validator parameter with something like expected_type.
|
|
||
| @classmethod | ||
| def for_job(cls, job: RQJob): | ||
| return cls(job=job) |
There was a problem hiding this comment.
| return cls(job=job) | |
| return cls(job=job, meta=job.meta) |
This would simplify the rest of the class: no need for the if in def meta, no need for the assert in __init__.
| { | ||
| RQJobMetaField.UserField.ID: user.id, | ||
| RQJobMetaField.UserField.USERNAME: user.username, | ||
| RQJobMetaField.UserField.EMAIL: getattr(user, "email", ""), |
There was a problem hiding this comment.
How could email be a missing attribute? Isn't it part of the User model?
| RQJobMetaField.REQUEST: RequestMeta( | ||
| { | ||
| RQJobMetaField.RequestField.UUID: request.uuid, | ||
| RQJobMetaField.RequestField.TIMESTAMP: timezone.localtime(), |
|
|
||
| user = request.user | ||
|
|
||
| return cls.for_meta( |
There was a problem hiding this comment.
Isn't the class redundant here? You're creating a dictionary, wrapping it, then immediately unwrapping it.
Motivation and context
This PR changes the way we work with RQ meta. The previous implementation tried to standardize the fields saved in the meta and how to access them, however, we had to remember the structure of nested objects. This approach explicitly defines the structure of the meta being used.
Extracted from #9075
Partially depends on #9077
How has this been tested?
Checklist
developbranchLicense
Feel free to contact the maintainers if that's a concern.