Issue #132 - Added needed routes for geolocate #133
Conversation
| let mut response = HttpResponse::Ok(); | ||
| response.append_header(( | ||
| http::header::CACHE_CONTROL, | ||
| "max-age=0, no-cache, no-store, must-revalidate", |
There was a problem hiding this comment.
Why do we set these? Can we explicit in a comment?
There was a problem hiding this comment.
Mirrored logic from classify_client. I'm guessing this is so requests are re-made every time in case addresses change, but that is speculation. Will investigate.
src/endpoints/country.rs
Outdated
| "max-age=0, no-cache, no-store, must-revalidate", | ||
| )); | ||
|
|
||
| let country = location.unwrap().country.unwrap(); |
There was a problem hiding this comment.
We can unwrap location because we have a branch when it's none.
But what if country is none here? Don't we want to cover that branch above to return not found if country is none?
src/endpoints/country.rs
Outdated
| let country = location.unwrap().country.unwrap(); | ||
| response.json(CountryResponse { | ||
| country_code: country.iso_code.unwrap(), | ||
| country_name: country.names.unwrap()["en"], |
There was a problem hiding this comment.
We don't want to panic, so we have to avoid using unwrap here.
We may want to use .unwrap_or("default")
or
country_name: match country.names {
Some(v) => v["en"],
None => ""
}There was a problem hiding this comment.
I kinda don't like that this is a struct where all props are Options, seems very messy but that is an upstream thing. I'll try to make it as readable as possible.
| country_name: country.names.unwrap()["en"], | ||
| }) | ||
| }) | ||
| .map_err(|err| ClassifyError::from_source("Future failure", err)); |
There was a problem hiding this comment.
What's a "future failure"?
There was a problem hiding this comment.
I was mirroring the logic from classify_client but I'm not sure why. I don't think we should even be able to hit this error.
| assert_eq!( | ||
| miss_response.status(), | ||
| 404, | ||
| "Geoip should return 404 http status for an unknown IP" |
There was a problem hiding this comment.
Is this specification really part of the endpoint?
There was a problem hiding this comment.
As far as I can tell, yes.
There was a problem hiding this comment.
Even if there's no explicit code for it, right? You are checking here that it also applies to this endpoint I guess
There was a problem hiding this comment.
Oh, we actually do return 404 specifically up on line 49.
| struct CountryNotFoundError<'a> { | ||
| domain: &'a str, | ||
| reason: &'a str, | ||
| message: &'a str, |
There was a problem hiding this comment.
(not sure) Are lifetimes necessary if they are all the same as the struct?
There was a problem hiding this comment.
When I try to remove them the compiler gets upset with me. Will look into a bit more.
| assert_eq!( | ||
| miss_response.status(), | ||
| 404, | ||
| "Geoip should return 404 http status for an unknown IP" |
There was a problem hiding this comment.
Even if there's no explicit code for it, right? You are checking here that it also applies to this endpoint I guess
| country_code: match country.iso_code { | ||
| Some(x) => x, | ||
| None => "", | ||
| }, |
There was a problem hiding this comment.
this could be country.iso_code.unwrap_or("")
| country_name: match country.names { | ||
| Some(x) => x["en"], | ||
| None => "", | ||
| }, |
There was a problem hiding this comment.
I never know whether the short form is better in this case: country.names.and_then(|x| x["en"]);
src/endpoints/country.rs
Outdated
| .insert_header(("x-forwarded-for", "127.0.0.2")) | ||
| .to_request(); | ||
| println!("Request: {:?}", miss_request); |
src/endpoints/country.rs
Outdated
| } | ||
| }, | ||
| _ => { | ||
| return Ok(HttpResponse::Unauthorized().body("")); |
src/endpoints/country.rs
Outdated
| Ok(req_query) => match KEYS_HASHSET.lock() { | ||
| Ok(keys) => { | ||
| if !keys.contains(&req_query.key) { | ||
| return Ok(HttpResponse::Unauthorized().body("")); |
src/endpoints/country.rs
Outdated
| } | ||
| } | ||
| _ => { | ||
| return Ok(HttpResponse::Unauthorized().body("")); |
There was a problem hiding this comment.
We could want to return a 5XX if the file is not present
| @@ -1,3 +1,4 @@ | |||
| /target/ | |||
| GeoLite2-Country.mmdb | |||
| .vscode/ | |||
| apiKeys.json | |||
There was a problem hiding this comment.
Why this if we ship it in the repo?
There was a problem hiding this comment.
We ship the test file to make it easy. But we won't have the keys in the repo, they'll get merged in as the container is deployed.
This relates to the retirement of the Mozilla Location Service. We still rely on some (very limited) functionality so we'll be porting that over to classify-client.
Added routes for issue #132 . Added unit tests for country endpoint.
Didn't add unit tests for canned responses as that seemed excessive, but can add if we think that's useful.
This is first rust work, so please be nitpicky but detailed. But I think me and rust are gonna get along great.
This relies on writing a file in the deployment process that contains the valid API keys for the
/api/v1/countryroute.Note: Pulled in changes from #135 because circle-ci wasn't working right.