Policy engine proptests #3985
Policy engine proptests #3985
Conversation
vadim-kovalyov
requested review from
dmolokanov,
and-rewsmith and
dylanbronson
mqtt/policy/src/core/mod.rs
Outdated
| .iter() | ||
| .cartesian_product(statement.operations()) | ||
| .cartesian_product(statement.resources()) | ||
| .map(|item| Request::new(item.0.0, item.0.1, item.1) |
There was a problem hiding this comment.
something went wrong with formatting :(
There was a problem hiding this comment.
yeah... there is a known issue with nested tuples and rustfmt which should be fixed in upcoming release. rust-lang/rustfmt#4355
mqtt/policy/src/core/mod.rs
Outdated
|
|
||
| // collect all combos of identity/operation/resource | ||
| // in the statement. | ||
| let requests = statement.identities() |
There was a problem hiding this comment.
I guess this macro do the same https://docs.rs/itertools/0.9.0/itertools/macro.iproduct.html
| @@ -843,4 +833,98 @@ pub(crate) mod tests { | |||
| policy.starts_with(input) | |||
| } | |||
| } | |||
|
|
|||
| #[cfg(feature = "proptest")] | |||
| mod proptests { | |||
There was a problem hiding this comment.
Do you want to split proptest generators and test themselves and move generators to a separate file?
There was a problem hiding this comment.
Good suggestion, but at the moment I don't see a strong reason to do that. It adds more complexity with feature flagging and such..
| #[derive(Debug, Copy, Clone, PartialOrd, PartialEq)] | ||
| enum Effect { | ||
| enum EffectImpl { |
There was a problem hiding this comment.
Not sure I understood this change. You have Effect, EffectImpl, and Decision. What are the differences?
There was a problem hiding this comment.
Effect is a part of policy definition. It can be "undefined" in certain cases. We used to have two Effect enums, in mod.rs and builder.rs. They are a bit different, but probably can be combined together. I have some ideas I'll explore in the following PR.
Decision is a result of policy evaluation. It looks similar, but conceptually a different enum.
There was a problem hiding this comment.
Can you please put some docs in the comments for each of them covering differences, especially Effect/EffectImpl?
| use itertools::Itertools; | ||
|
|
||
| // take very first statement, which should have top priority. | ||
| let statement = &definition.statements()[0]; |
There was a problem hiding this comment.
I guess it is possible to receive an empty definition from the customer. Do we want to cover that here?
There was a problem hiding this comment.
It is possible, and in that case every request will return default decision. We don't need to cover that in proptests though, I believe we have a unit test for that.
mqtt/policy/src/core/mod.rs
Outdated
| .cartesian_product(statement.resources()) | ||
| .map(|item| Request::new(item.0.0, item.0.1, item.1) | ||
| .expect("unable to create a request") | ||
| ).collect::<Vec<_>>(); |
There was a problem hiding this comment.
| ).collect::<Vec<_>>(); | |
| ); |
I believe you don't need to allocate a vec only to iterate it later in for loop
There was a problem hiding this comment.
I needed to materialize the collection, b/c I can't continue to use borrowed statement after I create a policy from it. Policy consumes the definition.
| .map(|item| Request::new(item.0.0, item.0.1, item.1) | ||
| .expect("unable to create a request") | ||
| ).collect::<Vec<_>>(); | ||
| let requests = iproduct!( |
In this PR I've added one proptest scenario for Policy engine. To make that work I had to introduce a method `PolicyBuilder::from_definition(definition)`, that can accept struct, instead of json string. Proptests guarder by the "proptest" feature.
In this PR I've added one proptest scenario for Policy engine. To make that work I had to introduce a method `PolicyBuilder::from_definition(definition)`, that can accept struct, instead of json string. Proptests guarder by the "proptest" feature.
In this PR I've added one proptest scenario for Policy engine.
To make that work I had to introduce a method
PolicyBuilder::from_definition(definition), that can accept struct, instead of json string.Proptests guarder by the "proptest" feature.