Implementation of FilterChain #45
Conversation
src/extensions/filter_chain.rs
Outdated
| for filter_config in config.filters.clone() { | ||
| match filter_registry.get(&filter_config.name) { | ||
| None => { | ||
| return Err(Error::new( | ||
| ErrorKind::InvalidInput, | ||
| format!("Filter '{}' not found", filter_config.name), | ||
| )); | ||
| } | ||
| Some(filter) => { | ||
| filters.push(filter.clone()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
filter_config doesn't seem to get moved anywhere here, why is config.filters cloned instead of borrowed ?
There was a problem hiding this comment.
Good catch!!! Changed!
| } | ||
| } | ||
|
|
||
| impl Filter for FilterChain { |
There was a problem hiding this comment.
The FilterChain implementation reads rather weirdly. They always return the last filter of that type in the chain, and looping over the whole vec for each call doesn't feel super nice. I have no better suggestions at the moment though as I'm not familiar enough yet with the rest of the code base.
Or am I reading them wrong ? Thats entirely possible too :D
There was a problem hiding this comment.
Unfortunately Not sure I'm following what you mean.
Each filter implementation loops around all the filters stored in the FilterChain, passing the results of each filter to the next in the chain.
Finally it returns the results of data that has gone through each of the filters in the chain.
If any of the Filters in the chain return a None, then the chain is broken, and nothing is returned.
Does that make sense?
(Long term, I expect this API to shift with the Filter trait as we work out more use cases, and/or performance optimisations)
There was a problem hiding this comment.
Let me reread it with this in mind!
There was a problem hiding this comment.
Yes, I see how it works now. Could we add a big comment in the code somewhere prominent on how this works ? Its not super clear just reading it without comments
markmandel
force-pushed
the
feature/filter-chain
branch
from
342f55e to
e9f1403
Compare
This is so we can take a set of Filters and string their execution in the appropriate places. FilterChain also implements Filter, so it's clear what FilterChain functions match to Filter functions. Once we have this in place, we can build a FilterChain from the combination of a Config the FilterRegistry on startup of a Quilkin executable.
markmandel
force-pushed
the
feature/filter-chain
branch
from
e9f1403 to
d5c5d09
Compare
This is so we can take a set of Filters and string their execution in the appropriate places.
FilterChain also implements Filter, so it's clear what FilterChain functions match to Filter functions.
Once we have this in place, we can build a FilterChain from the combination of a Config the FilterRegistry on startup of a Quilkin executable.