Business logic validator plugin implementation #84

chipn · 2021-02-08T19:38:20Z

Resolves #24

schema_enforcer/schemas/validator.py

itdependsnetworks · 2021-02-08T20:48:35Z

Can you update the docs on how to use it?

chipn · 2021-02-08T20:56:15Z

Can you update the docs on how to use it?

I figured it was worth waiting for agreement on the approach before adding docs, just in case we decide not to use this particular implementation.

itdependsnetworks · 2021-02-08T21:10:56Z

Just enough for me to understand, I don't actually know how it is working.

schema_enforcer/config.py

schema_enforcer/schemas/manager.py

schema_enforcer/schemas/validator.py

dgarros · 2021-02-08T23:15:34Z

tests/fixtures/test_validators/validators/check_peers.py

+                            peer_match = data[peer]["interfaces"][peer_int]["peer"] == normal_hostname(host)
+                            peer_int_match = data[peer]["interfaces"][peer_int]["peer_int"] == interface
+                            if not (peer_match and peer_int_match):
+                                raise ValidationError


Ideally we should return a list of ValidationResult here instead of expecting an exception to be thrown. It allows the validators to flag more than 1 error at a time.

Thinking out loud, would be useful to provide something in the ModelValidation class to generate/store ValidationResult easily.

I haven't updated the check_peer examples to return ValidationResults since cross-host correlation isn't currently supported. The check_interface example is what is supported and it returns a ValidationResult.

I like the idea of helper functions for generating/storing ValidationResult. I'll have to think about a good way to implement that.

dgarros · 2021-02-08T23:19:42Z

@chipn I forgot to mention it but it's great to see so much progress already and it's exciting to add this capabilities to the schema-enforcer 👍
Impressive work so far

itdependsnetworks · 2021-02-08T23:21:34Z

Yes, agreed!! Really great work with little guidance

docs/custom_validators.md

glennmatthews · 2021-02-09T18:10:55Z

docs/custom_validators.md

+    id = "CheckInterface"
+    left = "interfaces.*[@.type=='core'][] | length([?@])"
+    right = 2
+    operator = "eq"


Would it be possible to support specifying left/right/operator as a single combined string, such as "interfaces.*[@.type=='core'][] | length([?@]) eq 2"? That seems like it'd be more user-friendly.

I'd like to support this format, but I couldn't think of a good way to do so since jmespath expressions can be complex and contain spaces/pipes. Any thoughts on how to implement it?

Probably fine to defer to a later PR, but off the top of my head can we regex around the supported operator strings (plus surrounding whitespace)? Something like r"(.*) (eq|lt|gt|lte|gte|ne) (.*)"? Or can these operators potentially be used within left/right?

Yes, that is likely a valid use case. The optimization doesn’t seem like too much to me, would prefer to do something similar in a json/yaml file outside of python if we are going to spend efforts on it.

docs/custom_validators.md

schema_enforcer/schemas/validator.py

glennmatthews · 2021-02-09T18:16:58Z

schema_enforcer/schemas/validator.py

+            "eq": lambda r, v: r == v,
+            "lt": lambda r, v: int(r) < int(v),
+            "lte": lambda r, v: int(r) <= int(v),
+            "contains": lambda r, v: v in r,


Could we support the actual operators (">", ">=", etc.) as well? Or is that not useful/desirable in a jmespath context?

We can support the actual operators. However, I wonder if that may be confusing. "gt" is obviously a string, but someone may try to use > (unquoted). Easy to correct by adding quotes, but maybe not always obvious?

Yeah agreed; I think we'd only want to do this if we're able to combine the "left", "right", and "operator" parameters into a single string as discussed above.

schema_enforcer/schemas/validator.py

tests/fixtures/test_validators/validators/check_interfaces.py

tests/fixtures/test_validators/validators/check_peers.py

glennmatthews

Looks great to me!

examples/ansible3/validators/check_interfaces.py

docs/custom_validators.md

tests/test_schemas_validator.py

dgarros · 2021-02-11T11:52:36Z

schema_enforcer/schemas/manager.py

@@ -43,6 +44,11 @@ def __init__(self, config):
            schema = self.create_schema_from_file(root, filename)
            self.schemas[schema.get_id()] = schema

+        # Load validators
+        full_validator_dir = f"{config.validator_directory}"


we should remove the fstring since it's not used.

Why isn't our tests catching this?

schema_enforcer/schemas/validator.py

dgarros

Thanks Chip, looks great

schema_enforcer/schemas/validator.py

README.md

itdependsnetworks · 2021-03-02T15:17:21Z

docs/custom_validators.md

+
+   * `top_level_properties`: Field for mapping of validator to data
+   * `id`: Schema ID to use for reporting purposes (optional - defaults to class name)
+   * `left`: Jmespath expression to query your host data


I am the one who indicated left/right, but I did so out of lack of imagination. Just want to make sure we are all good with the name here.

Another option is lhs (left-hand side) and rhs (right-hand side).

left / right sounds good to me

tests/fixtures/test_validators/validators/check_peers.py

schema_enforcer/schemas/validator.py

dgarros · 2021-03-03T11:22:03Z

schema_enforcer/cli.py

@@ -303,8 +303,9 @@ def ansible(
                data = hostvars

            # Validate host vars against schema
-            for result in schema_obj.validate(data=data, strict=strict):
+            schema_obj.validate(data=data, strict=strict)


Please can you make the same changes to the validate command , otherwise validators won't work for this command

The validate CLI command calls validate on the InstanceFile object. The InstanceFile validate method was refactored for the new approach:

schema-enforcer/schema_enforcer/instances/file.py

Lines 128 to 134 in 7d557ef

for schema_id, schema in schema_manager.iter_schemas():

if schema_id not in self.matches:

continue

schema.validate(self.get_content(), strict)

results = schema.get_results()

errs = itertools.chain(errs, results)

schema.clear_results()

dgarros · 2021-03-03T11:31:48Z

docs/custom_validators.md

+
+   * `top_level_properties`: Field for mapping of validator to data
+   * `id`: Schema ID to use for reporting purposes (optional - defaults to class name)
+   * `left`: Jmespath expression to query your host data


left / right sounds good to me

PhillSimonds

Great work! LGTM. A few comments in line.

PhillSimonds · 2021-03-04T17:43:31Z

docs/custom_validators.md

@@ -0,0 +1,158 @@
+# Implementing custom validators
+
+With custom validators, you can implement business logic in Python. Schema-enforcer will automatically


An intro of what's meant by "business logic" might be valuable here to set proper context?

pinging myself to review

PhillSimonds · 2021-03-04T18:31:37Z

examples/ansible3/validators/check_interfaces.py

+
+class CheckInterface(JmesPathModelValidation):  # pylint: disable=too-few-public-methods
+    """Check that each device has more than one core uplink."""
+


I think pydocstyle requires no newline here?

No newline after the comment? I vaguely remember that pydocstyle complained when it isn't in a single line or you miss the period at the end. This validator is also in the test directory and pydocstyle is passing.

Ack. That's interesting -- I've seen pydocstyle flag when a function has space between the docstring and the beginning of the logic executed by the function. It may just be on multiline docstrings though.. IDK. If pydocstyle isn't complaining I'm good :).

PhillSimonds · 2021-03-04T18:39:35Z

schema_enforcer/schemas/validator.py

+    def get_results(self) -> list[ValidationResult]:
+        """Return all validation results for this validator."""
+        if not self._results:
+            self._results.append(ValidationResult(result="PASS", schema_id=self.id))


Somewhat adjacent to #46, If there are no validation results, do we want to print a log message indicating that nothing was validated?

May defer to @dgarros here. I assumed the intent was to support the use case of no failure is equal to a pass, but that doesn't really fit the current runtime model.

chipn requested review from dgarros, glennmatthews and PhillSimonds as code owners February 8, 2021 19:38

itdependsnetworks reviewed Feb 8, 2021

View reviewed changes

schema_enforcer/schemas/validator.py Outdated Show resolved Hide resolved

dgarros reviewed Feb 8, 2021

View reviewed changes

glennmatthews reviewed Feb 9, 2021

View reviewed changes

glennmatthews approved these changes Feb 10, 2021

View reviewed changes

chipn requested review from itdependsnetworks and dgarros February 10, 2021 18:33

dgarros reviewed Feb 12, 2021

View reviewed changes

dgarros approved these changes Mar 1, 2021

View reviewed changes

schema_enforcer/schemas/validator.py Show resolved Hide resolved

chipn added 14 commits March 1, 2021 15:29

Add jmespath dependency

ada0b89

Initial commit of validator classes

a47c0d8

Initial commit of validator tests

796e597

Support compiled jmespath expr for rhs

52e6a2a

Add ValidationResult class to context

19a4d60

Add ansible example for validator plugins

08afcf3

Exclude plugin examples from linting

3511724

Integrate validator plugin support

691c899

Update tests for validator plugin integration

674a119

Fix pylint and yamllint errors

6e98b64

Fix pydocstyle errors

63239c4

Update pytest task

65fc216

Rename validator dir config option

5407340

Refactor to use importlib for validators

6f08f68

chipn added 18 commits March 1, 2021 15:29

Disable pylint rule at file level

3c961ee

Update tests and comments

a4b57a5

Add example of jmespathvalidation with compile

f3a4868

Add additional test for jmespath compile

856102b

Remove unused f-string

5427077

Refactor base classes for common use

db5109c

Disable pylint rule

1459cc0

Refactor jsonschema to use BaseValidation class

0a120a9

Refactor ansible cli for BaseValidation class

67c0cff

Fix pydocstyle errors

220590b

Update documentation for new base class usage

afc47b6

Update example validator

ad31827

Exclude examples dir from pylint due to dup-code

0d5c573

Revert to class vars for jmespathmodelvalidation

e1dbdf9

Increase pylint min-similarity-lines

99eef97

Revert exclude of examples for pylint

abaf0a7

Add link to custom validator doc in README

622feea

Update validate doc string

01297dc

chipn force-pushed the plugin branch from a07ffe0 to 01297dc Compare March 1, 2021 22:32

itdependsnetworks reviewed Mar 2, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

itdependsnetworks reviewed Mar 2, 2021

View reviewed changes

Fix unaccepted change from rebase

7d557ef

dgarros approved these changes Mar 3, 2021

View reviewed changes

chipn added 2 commits March 3, 2021 09:48

Update type annotations

e264591

Update duplicate validator error

8e1dab7

PhillSimonds approved these changes Mar 4, 2021

View reviewed changes

itdependsnetworks approved these changes Mar 18, 2021

View reviewed changes

chipn changed the title ~~[WIP] Business logic validator plugin implementation~~ Business logic validator plugin implementation Mar 18, 2021

dgarros merged commit d165177 into develop Apr 5, 2021

dgarros deleted the plugin branch December 21, 2021 19:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Business logic validator plugin implementation #84

Business logic validator plugin implementation #84

chipn commented Feb 8, 2021

itdependsnetworks commented Feb 8, 2021

chipn commented Feb 8, 2021

itdependsnetworks commented Feb 8, 2021 •

edited

Loading

dgarros Feb 8, 2021

dgarros Feb 8, 2021

chipn Feb 8, 2021

chipn Feb 8, 2021

dgarros commented Feb 8, 2021

itdependsnetworks commented Feb 8, 2021

glennmatthews Feb 9, 2021

chipn Feb 9, 2021

glennmatthews Feb 9, 2021

itdependsnetworks Feb 11, 2021

glennmatthews Feb 9, 2021

chipn Feb 9, 2021 •

edited

Loading

glennmatthews Feb 10, 2021

glennmatthews left a comment

dgarros Feb 11, 2021

itdependsnetworks Feb 12, 2021

dgarros left a comment

itdependsnetworks Mar 2, 2021

chipn Mar 2, 2021

dgarros Mar 3, 2021

dgarros Mar 3, 2021

chipn Mar 3, 2021

dgarros Mar 3, 2021

PhillSimonds left a comment

PhillSimonds Mar 4, 2021

itdependsnetworks Mar 4, 2021

PhillSimonds Mar 4, 2021

chipn Mar 4, 2021

PhillSimonds Mar 4, 2021

PhillSimonds Mar 4, 2021

chipn Mar 4, 2021

	for schema_id, schema in schema_manager.iter_schemas():
	if schema_id not in self.matches:
	continue
	schema.validate(self.get_content(), strict)
	results = schema.get_results()
	errs = itertools.chain(errs, results)
	schema.clear_results()

		@@ -0,0 +1,158 @@
		# Implementing custom validators

		With custom validators, you can implement business logic in Python. Schema-enforcer will automatically


		class CheckInterface(JmesPathModelValidation): # pylint: disable=too-few-public-methods
		"""Check that each device has more than one core uplink."""

Business logic validator plugin implementation #84

Business logic validator plugin implementation #84

Conversation

chipn commented Feb 8, 2021

itdependsnetworks commented Feb 8, 2021

chipn commented Feb 8, 2021

itdependsnetworks commented Feb 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgarros commented Feb 8, 2021

itdependsnetworks commented Feb 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chipn Feb 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennmatthews left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgarros left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhillSimonds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itdependsnetworks commented Feb 8, 2021 •

edited

Loading

chipn Feb 9, 2021 •

edited

Loading