Skip to content
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

Merged
merged 11 commits into from
Apr 28, 2021
Merged

Implement combining validators #249

merged 11 commits into from
Apr 28, 2021

Conversation

borzunov
Copy link
Member

@borzunov borzunov commented Apr 26, 2021

This PR proposes a way to combine validators. The API should match the following requirements:

  1. An application should be able to add new validators to an existing DHT instance, so that several applications (e.g. the averager and the MoE) may use one DHT instance (passed to them in the class constructors).
  2. Some validator classes allow using multiple validator instances (e.g. each application may add a SchemaValidator with its own schema), others don't (e.g. only one RSASignatureValidator makes sense).
  3. The validator classes should be executed in a specific order (e.g. RSASignatureValidator should remove the signature before SchemaValidator will try to deserialize a record and validate the schema).

The proposed solution is described in this comment.

@borzunov borzunov added the dht label Apr 26, 2021
@borzunov borzunov requested a review from justheuristic April 26, 2021 03:18
@borzunov borzunov changed the title Add a class to combine validators Implement combining validators Apr 26, 2021
@justheuristic
Copy link
Member

[optional suggestion] the term for adding (with overwriting) keys and values of one dictionary to another is dict1.update(dict2). Perhaps it is more intuitive to rename this method to .update_validator. That said, i definitely do not insist and the decision is entirely yours.

@borzunov
Copy link
Member Author

borzunov commented Apr 26, 2021

Initially, I've decided to name it extend to emphasize the different behavior from update (the fact that it does not change the existing keys). However, now it seems to be unclear, so I'll rename it to set_if_not_present.

Copy link
Member

@justheuristic justheuristic left a 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]

@borzunov borzunov force-pushed the composite-validator branch 2 times, most recently from 33cec13 to 406f77f Compare April 26, 2021 14:35
@borzunov borzunov marked this pull request as ready for review April 26, 2021 17:56
@borzunov
Copy link
Member Author

borzunov commented Apr 27, 2021

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 .priority property and the .merge_with() policy, so the CompositeValidator may merge validators of different classes correctly (e.g. with the OR policy for schemas) and then figure out the correct order for applying validators. See the code for details.

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.

@borzunov borzunov requested review from mryab and justheuristic April 27, 2021 02:12
@borzunov borzunov force-pushed the composite-validator branch from 646a842 to c462233 Compare April 27, 2021 02:19
if validators is not None:
self.extend(validators)

def extend(self, validators: List[RecordValidatorBase]) -> None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def extend(self, validators: List[RecordValidatorBase]) -> None:
def extend(self, validators: List[RecordValidatorBase]):

Copy link
Member Author

@borzunov borzunov Apr 27, 2021

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).

@borzunov borzunov force-pushed the composite-validator branch from 8847e2a to 52ebdc1 Compare April 28, 2021 12:49
@borzunov borzunov requested a review from mryab April 28, 2021 12:50
@borzunov borzunov force-pushed the composite-validator branch from 6212e01 to b7b5b4a Compare April 28, 2021 14:33
@borzunov borzunov force-pushed the composite-validator branch from d62342f to 62d7c71 Compare April 28, 2021 17:27
@borzunov
Copy link
Member Author

borzunov commented Apr 28, 2021

@justheuristic has verbally confirmed that I can merge this.

@borzunov borzunov merged commit 18add2c into master Apr 28, 2021
@borzunov borzunov deleted the composite-validator branch April 28, 2021 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants