-
Notifications
You must be signed in to change notification settings - Fork 176
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
Implement combining validators #249
Conversation
[optional suggestion] the term for adding (with overwriting) keys and values of one dictionary to another is |
Initially, I've decided to name 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.
[i have no further suggestions, LGTM]
33cec13
to
406f77f
Compare
I've tried to implement a simpler scheme to combine validators. Now the applications may append them to the DHT in any order: # In the application A's code
dht.add_validators([RSASignatureValidator(), SchemaValidator(SchemaA)])
# In the application B's code
dht.add_validators([SchemaValidator(SchemaB), RSASignatureValidator()]) Internally, each validator class implements the I've also changed the SchemaValidator to reject keys that are not specified in any of the schemas (that became possible due to the OR policy mentioned above). I will re-request the review because of these significant changes. |
646a842
to
c462233
Compare
hivemind/dht/validation.py
Outdated
if validators is not None: | ||
self.extend(validators) | ||
|
||
def extend(self, validators: List[RecordValidatorBase]) -> None: |
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.
def extend(self, validators: List[RecordValidatorBase]) -> None: | |
def extend(self, validators: List[RecordValidatorBase]): |
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.
Why is that?
I thought the convention is that this means no annotation, and to add one we should explicitly specify -> None
(like we do in other parts of the project, e.g. here).
8847e2a
to
52ebdc1
Compare
Co-authored-by: Max Ryabinin <[email protected]>
6212e01
to
b7b5b4a
Compare
d62342f
to
62d7c71
Compare
@justheuristic has verbally confirmed that I can merge this. |
This PR proposes a way to combine validators. The API should match the following requirements:
SchemaValidator
with its own schema), others don't (e.g. only oneRSASignatureValidator
makes sense).RSASignatureValidator
should remove the signature beforeSchemaValidator
will try to deserialize a record and validate the schema).The proposed solution is described in this comment.