Move attribute parsing to attribute trait #65
Conversation
Signed-off-by: Adam Cattermole <acatterm@redhat.com>
src/policy.rs
Outdated
| } | ||
| Ok(attribute_value) => attribute_value, | ||
| }, | ||
| Some(attribute_bytes) => Attribute::parse(attribute_bytes).ok()?, |
There was a problem hiding this comment.
I'm not 100% here, because this could take advantage of the type_of to determine the type instead of assuming it's a String
Signed-off-by: Adam Cattermole <acatterm@redhat.com>
Signed-off-by: Adam Cattermole <acatterm@redhat.com>
adam-cattermole
force-pushed
the
attribute-parsing
branch
from
55123bf
to
d044a3e
Compare
| #[allow(dead_code)] | ||
| pub fn get_attribute<T>(f: &Filter, attr: &str) -> Result<T, String> |
There was a problem hiding this comment.
This is apparently still dead_code, but I also wonder how this is going to be useful... <T> needs to be known at compile time here, so that we need to know at method invocation time what the type of attr... is this ever going to be the case that this is known before runtime?
There was a problem hiding this comment.
As an example when I was building some request stuff for auth I was using it like this (providing a default to infer types):
attr.set_source(build_peer(
get_attribute(&self, "source.address").unwrap_or("".to_string()),
get_attribute(&self, "source.port").unwrap_or(0i64) as u32,
));
But then again now I think about it, these should probably remain unset if we can't get the attribute and not be a default value.
There was a problem hiding this comment.
Yeah, that actually makes sense... forgot that in the case of auth, there is a set of attributes (both their name and type) that need to be retrieved no matter what... so yes, that fn would be useful for that. The unwrap_or I'm less confident about it indeed as well
| ValueType::String => Value::String(Arc::new(Attribute::parse(raw_attribute)?)), | ||
| ValueType::Int => Value::Int(Attribute::parse(raw_attribute)?), | ||
| ValueType::UInt => Value::UInt(Attribute::parse(raw_attribute)?), | ||
| ValueType::Float => Value::Float(Attribute::parse(raw_attribute)?), | ||
| ValueType::Bytes => Value::Bytes(Arc::new(Attribute::parse(raw_attribute)?)), | ||
| ValueType::Bool => Value::Bool(Attribute::parse(raw_attribute)?), | ||
| ValueType::Timestamp => Value::Timestamp(Attribute::parse(raw_attribute)?), |
| Self: Sized; | ||
| } | ||
|
|
||
| impl Attribute for String { |
There was a problem hiding this comment.
I think this could "just" be the TryInto trait, but it is that against the orphan rules
Signed-off-by: Adam Cattermole <acatterm@redhat.com>
(builds on top of #64 refactoring-1-bis)
For the auth implementation I wanted to be able to retrieve/parse the different attributes from
filter.get_propertywithout need to evaluate aPatternExpressionso I decided to move this to it's own trait with an implementation for each type (where the implementation comes from the original one within the pattern expression).This then introduces a generic
get_attributefunction which can be used like thisTo return a
Result<String,String>in this case with the property.Looking for all feedback as I'm unsure if this is the correct approach to do this.