Alternative implementation of the policy document parser #3246
Conversation
narrieta
requested review from
ZhidongPeng,
nagworld9 and
maddieford
as code owners
| _PolicyEngine.__parse_version(template, custom_policy) | ||
| _PolicyEngine.__parse_extension_policies(template, custom_policy) | ||
| if not isinstance(policy, dict): | ||
| raise InvalidPolicyError("expected an object describing a Policy; got {0}.".format(type(policy).__name__)) |
There was a problem hiding this comment.
nit: policy doesn't need to be capitalized
There was a problem hiding this comment.
thanks; in that message, I'm trying to refer to the concept/type of "Policy", rather than a specific "policy" instance.
if that does not make sense, i can lowercase it
| if not isinstance(policy, dict): | ||
| raise InvalidPolicyError("expected an object describing a Policy; got {0}.".format(type(policy).__name__)) | ||
|
|
||
| _PolicyEngine._check_attributes(policy, object_name="policy", valid_attributes=["policyVersion", "extensionPolicies"]) |
There was a problem hiding this comment.
could we get valid_attributes from a schema constant instead of hardcoding them here?
There was a problem hiding this comment.
i want them to be inline so that people can scan/read the code linearly, without having to jump to the definition of a constant to check what the valid attributes are
| for extension_name, individual_policy in extensions.items(): | ||
| for extension, extension_policy in extensions.items(): | ||
| if not isinstance(extension_policy, dict): | ||
| raise InvalidPolicyError("invalid type {0} for attribute '{1}', must be 'object'".format(type(extension_policy).__name__, extension)) |
There was a problem hiding this comment.
nit: semicolon before "must" for consistency with the error message in get_value( )
| raise InvalidPolicyError( | ||
| "invalid value for attribute 'policyVersion' attribute 'policyVersion' is expected to be in format 'major.minor.patch' " | ||
| "(e.g., '1.0.0'). Please change to a valid value.") | ||
| if not re.match(r"^\d+\.\d+\.\d+$", version): |
There was a problem hiding this comment.
I think we should also accept "1" or "1.0" as valid versions (I should have added a unit test for too).
There was a problem hiding this comment.
I added the strict check since that is what the error message was specifically asking ('major.minor.patch'). Note, though, that this check should be done with a regex or similar, since FlexibleVersion accepts many, many other formats than 'major.minor.patch'.
"1", and "1.0" sound ok to me. If you want, you could add that after this change gets merged in
| "invalid value for attribute 'policyVersion' attribute 'policyVersion' is expected to be in format 'major.minor.patch' " | ||
| "(e.g., '1.0.0'). Please change to a valid value.") | ||
| if not re.match(r"^\d+\.\d+\.\d+$", version): | ||
| raise InvalidPolicyError("invalid value for attribute 'policyVersion'; it should be in format 'major.minor.patch' (e.g., '1.0.0')") |
There was a problem hiding this comment.
maybe include the provided version in the error message
|
|
||
| # The runtimePolicy is an arbitrary object. | ||
| runtime_policy = extension.get("runtimePolicy") | ||
| if runtime_policy is not None: |
There was a problem hiding this comment.
according to the schema defined spec, runtimePolicy is an object with arbitrary attributes, so we should probably validate that the type is dict.
There was a problem hiding this comment.
or if we want to accept list, string, etc., update the schema and spec accordingly
There was a problem hiding this comment.
yes, the extension policy is completely arbitrary and defined by the extension. spec needs to be updated
| } | ||
| for k in object_.keys(): | ||
| if k not in valid_attributes: | ||
| raise InvalidPolicyError("invalid attribute '{0}' in {1}".format(k, object_name)) |
There was a problem hiding this comment.
maybe "unrecognized" or "undefined" instead of "invalid" to make it clear that the attribute is not defined in the schema, as opposed to the type or format being invalid
There was a problem hiding this comment.
invalid: (of computer instructions, data, etc.) not conforming to the correct format or specifications
your comment made me doubt :)
but i can change it to "unknown" if it helps
Discussed with @mgunnala an alternative approach to write the parse for policy documents. It is similar in concept to a recursive descendent parser and produces code that is simpler to write, read, and debug.
We also decided to make the document version a required attribute.
Lastly, I added code to also parse the runtimePolicy for extensions.