bug: Add additional verification checks. #80
Conversation
| App::new().route( | ||
| fn init_mock_adm(response: String) -> (dev::Server, SocketAddr) { | ||
| let server = HttpServer::new(move || { | ||
| App::new().data(response.clone()).route( |
There was a problem hiding this comment.
Yes, it's gross.
Yes, it works.
| return Err(HandlerErrorKind::UnexpectedHost(species, host).into()); | ||
| } | ||
| Ok(()) | ||
| Ok(true) |
There was a problem hiding this comment.
Yep. it's used as a bool in check_click.
src/adm.rs
Outdated
| tags.add_extra("url", &url); | ||
| // Check the required fields are present for the `click_url` | ||
| // pg 15 of 5.7.21 spec. | ||
| let mut check_keys = vec![ |
There was a problem hiding this comment.
nit: don't need to_owned on these string values due to String having PartialEq<&str> -- it could also be a plain array instead of vec and could even be static, even w/ the sorting (via lazy_static)
There was a problem hiding this comment.
e.g.
lazy_static! {
static ref CLICK_PARAMS: [&'static str; 5] = {
let mut params = [
"ctag",
"version",
"key",
"ci",
"aespFlag",
];
params.sort();
params
};
}query_keys == *CLICK_PARAMS
There was a problem hiding this comment.
I thought about that, but I wanted to keep things together as much as possible, and lazy_static! doesn't want to be in a fn.
src/adm.rs
Outdated
| "ci".to_owned(), | ||
| "aespFlag".to_owned(), | ||
| ]; | ||
| check_keys.sort(); |
There was a problem hiding this comment.
Looks like the latest API doc has one more probably new addition here, an optional "click-status" field that we should allow if present. It's only for clicks.
There was a problem hiding this comment.
ok. I'll add a required and optional param check.
|
Also we can fix these after merging if they'll cause 3-PRs-in merge hell |
|
I've bought a bungalow in merge hell. The sunsets are nice if you ignore the wailing and gnashing of teeth. |
| return Err(HandlerErrorKind::InvalidHost(species, url.to_string()).into()); | ||
| } | ||
| }; | ||
| check_url(parsed, species, &filter.impression_hosts)?; |
There was a problem hiding this comment.
Sorry, we forgot one more thing: we should require an id param on impression_urls
…ile into feat/22-addl-check
…astapi-0.68.2 Bump fastapi from 0.68.1 to 0.68.2 in /partner
Closes #22
Description
Perform additional field validation checks for
impression_urlto ensure required values are present.Testing
Test included
Issue(s)
Closes #22