-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/uri #11
base: develop
Are you sure you want to change the base?
Feature/uri #11
Changes from 14 commits
1889e01
5f51f8f
0790ec9
0b37b20
ea19d68
188a738
bc8fe2a
c5cad88
4a030b2
15587e8
cb4aaea
276f085
ca57949
25d1344
491882c
aa375d9
97085d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
mod api_key; | ||
mod uri; | ||
pub use api_key::{APIKey, Permission}; | ||
mod coins; | ||
pub use coins::Coins; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,261 @@ | ||
#![allow(dead_code)] | ||
|
||
use reqwest; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per the build output this import is not used. Delete. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its currently used within the tests, so i'll move it there. |
||
|
||
use serde_derive::Deserialize; | ||
|
||
// these item structs should be namespaced into the v2 api. | ||
#[derive(Deserialize, Debug)] | ||
struct ItemAttribute { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All of these Item types are not publicly accessible or used in non-test code. If these are intended for testing then they should be moved to the testing module. However, testing URI building does not really require fully testing the deserialization of the received data. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I can see moving them into the testing section. |
||
attribute: String, | ||
modifier: u32, | ||
} | ||
|
||
#[derive(Deserialize, Debug)] | ||
struct ItemDetails { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like it has the additional item attributes that only armor has. Not all items will have this. ItemDetails should probably be a sum type based on the item type. Something like enum ItemDetails {
Armor(ArmorDetails),
Weapons(WeaponDetails),
// etc...
} This does not seem needed for this feature anyway, so should probably be deleted. Even if we are going to be deserializing items to test the URI building we do not need to fully test the complicated data model. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it should be deleted as that sounds like its throwing away work. These are here as a implementation for what the api could be. I do agree that the format should be better suited like you are indicating. |
||
#[serde(rename = "type")] | ||
c_type: String, | ||
weight_class: String, | ||
defense: u32, | ||
// whats the type of infusions slots item? | ||
//infusion_slots: | ||
//infix_upgrade : HashMap<String, | ||
// infix upgrade is of format { "id" : u32, "attributes" : Vec<ItemAttribute> } | ||
// bit un certain how to tell serdea the values are of different types https://github.com/serde-rs/json/issues/144 | ||
attribute_adjustment: f64, | ||
suffix_item_id: u64, | ||
secondary_suffix_item_id: Option<String>, | ||
} | ||
|
||
#[derive(Deserialize, Debug)] | ||
struct Item { | ||
name: String, | ||
// maybe should be an option? | ||
description: String, | ||
#[serde(rename = "type")] | ||
item_type: String, | ||
id: u32, | ||
level: u32, | ||
rarity: String, | ||
default_skin: u64, | ||
game_types: Vec<String>, | ||
flags: Vec<String>, | ||
// not certain what the vec type should be. | ||
restrictions: Vec<String>, | ||
chat_link: String, | ||
icon: String, | ||
details: ItemDetails, | ||
} | ||
|
||
// should be usize? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using usize for values coming from a web API is iffy to me. Introduces different platforms treating the same data slightly differently for no real compelling reason. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah sure, this was here as a question of not really know what the range of values from the api would be returned as. |
||
|
||
// todo: move this to some types thing? | ||
pub struct ItemId(pub u128); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. u128 is really overkill for this. All of these will fit in a u32. I'm working on a sqlite3 caching feature and it does not support u128 or (currently) u64 as keys. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah i'm up for w/e the size should be, didn't really see anything on the wiki that specified an upper limit. |
||
pub struct ItemStatId(pub u128); | ||
pub struct RecipeId(pub u128); | ||
pub struct ApiVersion(pub u8); | ||
pub struct ApiKey(pub String); | ||
pub struct AchievementId(pub u128); | ||
|
||
#[allow(non_camel_case_types)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should remove this and fix the name of all enum variants that are triggering this warning. We should be following the default Rust style guides. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, yeah initially I was hoping for some sort of macro solution that could work on the names of the enumed items. ie. account_materials would resolve to account/materials or something. using snake seemed easier to me to implement such a thing, granted the value in determining such a system might not be worth it. |
||
pub enum EndPoint { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Documentation? What is the goal of this type. Is it to exactly model the gw2 API URIs? Is it to be the Requestor's request interface? Depending on the answer I'd recommend different design changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The basics is that its to capture all of the different end points of the gw2 api as well any helpeful information about each of the end points. note the functions for getting the uri strings as well as information like if authentication is required. With the additions of the various api keys i'd imagen it would also return out the required permissions per api end point or have a compiled time checked api for if a specific key is okay to be used for that end point. I'm also looking to have this provide utility functions for bundling batches of requests into a single request. |
||
achievements(Option<AchievementId>), | ||
achievements_daily, | ||
achievements_daily_tomorrow, | ||
account, | ||
account_achievements, | ||
account_bank, | ||
account_materials, | ||
account_dailycrafting, | ||
account_dungeons, | ||
account_dyes, | ||
items(ItemId), | ||
item_stats(Option<ItemId>), | ||
// this is nothing to do with a specific item. | ||
// item stats supporst 3 end points/ | ||
// base /itemstats | ||
// id /itemstats/id | ||
// multiple(?) /itemstats?ids=23 | ||
item_stats_all(Option<ItemStatId>), | ||
recipes(Option<RecipeId>), | ||
build, | ||
} | ||
|
||
impl EndPoint { | ||
pub fn requires_auth(self) -> bool { | ||
match self { | ||
// do require auth. | ||
EndPoint::account => true, | ||
EndPoint::account_achievements => true, | ||
EndPoint::account_bank => true, | ||
EndPoint::account_materials => true, | ||
EndPoint::account_dailycrafting => true, | ||
EndPoint::account_dungeons => true, | ||
EndPoint::account_dyes => true, | ||
|
||
// don't require auth | ||
EndPoint::achievements(_) => false, | ||
EndPoint::achievements_daily => false, | ||
EndPoint::achievements_daily_tomorrow => false, | ||
EndPoint::items(_) => false, | ||
EndPoint::item_stats(_) => false, | ||
EndPoint::item_stats_all(_) => false, | ||
EndPoint::recipes(_) => false, | ||
EndPoint::build => false, | ||
} | ||
} | ||
|
||
pub fn uri(&self) -> String { | ||
match self { | ||
EndPoint::account => format!("account"), | ||
EndPoint::account_achievements => format!("account/achievements"), | ||
EndPoint::account_dailycrafting => format!("account/dailycrafting"), | ||
EndPoint::account_dungeons => format!("account/dungeons"), | ||
EndPoint::account_dyes => format!("account/dyes"), | ||
EndPoint::achievements(op_id) => match op_id { | ||
Some(id) => format!("achievements/{}", id.0.to_string()), | ||
None => format!("achievements"), | ||
}, | ||
EndPoint::achievements_daily => "achievements/daily".to_string(), | ||
EndPoint::achievements_daily_tomorrow => "achievements/daily/tomorrow".to_string(), | ||
EndPoint::account_materials => "account/materials".to_string(), | ||
EndPoint::account_bank => "account/bank".to_string(), | ||
EndPoint::items(id) => format!("items/{}", id.0.to_string()), | ||
EndPoint::item_stats(op_stats_id) => match op_stats_id { | ||
Some(stats_id) => format!("itemstats/{}", stats_id.0.to_string()), | ||
None => "itemstats".to_string(), | ||
}, | ||
EndPoint::item_stats_all(op_item_stat_id) => match op_item_stat_id { | ||
Some(item_stat_id) => format!("itemstats/{}", item_stat_id.0.to_string()), | ||
None => "itemstats".to_string(), | ||
}, | ||
EndPoint::recipes(op_recipe_id) => match op_recipe_id { | ||
Some(recipe_id) => format!("recipes/{}", recipe_id.0.to_string()), | ||
None => "recipes".to_string(), | ||
}, | ||
EndPoint::build => "build".to_string(), | ||
} | ||
} | ||
} | ||
|
||
pub struct Requester { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This type is named Requester yet makes no requests. What is the goal of this type? It should be documented so it can be discussed. Currently it seems that this type may not be needed. The only value of its state is specifying a different base url or API version. The base url may have some value, especially for testing. Although to me it makes more sense that the dependency injection happen at a much higher layer then uri building. The API version will never be changeable without huge changes to the library (assuming that a new version of the API ever happens). If eventually it will be making requests then indicate so. There is a lot of design work to talk about in this area. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you elaborate on the dependency injection statement? What dependency are you talking about the base url? I'd picture that the version if accounted for in the beginning of the libraries design would mean that there would be little to zero library changes. I do doubt they'll change the versions however. |
||
version: ApiVersion, | ||
api_key: Option<ApiKey>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This attribute is currently unused. Also I have some use cases where I will be requesting data from many different users. Making a Requester per api key may be kinda odd. Really depends on the goal of this type. If a publicly facing (Rust) API requires an API key I'd prefer that it explicitly takes it as a parameter (and also documents the permission requirements). An optional API key in a requester seems like we will be making requesters per request (which eliminates the potential value add of injecting the base_uri) or we will be sneaking a Option<ApiKey> through all of our code, potentially doing the wrong thing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you expand on how have a Reqeuster per api key seems odd? That seem normal to me, as the binding of a Requester to the api key allows for building uris for that specific key. I picture a sort of API builder that constructs apis based on conveniently provided input from the user, having to have the user specify their api key on each request sounds cumbersome to me, hence the key can be stored within the Requester type. Its ment as optional due to not all the api end points require authentication and thus this is to allow for a user to request api end points that do not require authentication. Sounds like you are suggesting to go towards a compile time check of api key and its permissions per end point? That sounds good me. Not certain what you mean by potentially doing the wrong thing though. |
||
base_uri: String, | ||
} | ||
|
||
impl Requester { | ||
// todo: add in auth key as input parameter. | ||
brandonphelps marked this conversation as resolved.
Show resolved
Hide resolved
|
||
pub fn new(version: ApiVersion, api_key: Option<ApiKey>) -> Requester { | ||
let mut uri_str = String::new(); | ||
uri_str += "https://api.guildwars2.com/v"; | ||
uri_str += &version.0.to_string(); | ||
return Requester { | ||
version: version, | ||
api_key: api_key, | ||
base_uri: uri_str, | ||
}; | ||
} | ||
|
||
pub fn build_uri(&self, end_point: &EndPoint) -> String { | ||
let mut new_uri = self.base_uri.clone(); | ||
new_uri.push_str("/"); | ||
new_uri.push_str(&end_point.uri().clone()); | ||
return new_uri; | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod test { | ||
|
||
use super::*; | ||
|
||
#[test] | ||
fn test_item_construction() { | ||
let p = EndPoint::items(ItemId(3)); | ||
assert_eq!(p.uri(), "items/3"); | ||
let k = EndPoint::items(ItemId(1000)); | ||
assert_eq!(k.uri(), "items/1000"); | ||
} | ||
|
||
#[test] | ||
fn test_uri_achivements() { | ||
let p = EndPoint::achievements(None); | ||
assert_eq!(p.uri(), "achievements"); | ||
} | ||
|
||
#[test] | ||
fn test_uri_achivement_ids() { | ||
let p = EndPoint::achievements(Some(AchievementId(32))); | ||
assert_eq!(p.uri(), "achievements/32"); | ||
} | ||
|
||
// need to wait till can combine specific end points. | ||
fn test_uri_achive_builder() { | ||
// https://wiki.guildwars2.com/wiki/API:2/achievements | ||
let p = EndPoint::achievements(Some(AchievementId(32))); | ||
let k = EndPoint::achievements(Some(AchievementId(40))); | ||
|
||
// would like to use the data refinement thing here to specify | ||
// a specific varient of the EndPoint enum item at compile time. | ||
// it appears to not be implements | ||
// https://github.com/rust-lang/rfcs/issues/754 | ||
// pretty certain order of ids in result do not matter. | ||
fn mock_builder(end_point1: &EndPoint, end_point2: &EndPoint) -> String { | ||
let id_one = match end_point1 { | ||
EndPoint::achievements(t) => match t { | ||
Some(id) => id, | ||
None => { | ||
panic!("shouldn't get here") | ||
} | ||
}, | ||
_ => { | ||
panic!("shouldn't get here") | ||
} | ||
}; | ||
let id_two = match end_point2 { | ||
EndPoint::achievements(t) => match t { | ||
Some(id) => id, | ||
None => { | ||
panic!("shouldn't get here") | ||
} | ||
}, | ||
_ => { | ||
panic!("shouldn't get here") | ||
} | ||
}; | ||
// should return "achivements?ids=id_one,id_two" | ||
// specifically fails cause its not implemented yet. | ||
return "no implemented".to_string(); | ||
} | ||
assert_eq!(mock_builder(&p, &k), "achievements?ids=32,40") | ||
} | ||
|
||
#[test] | ||
fn uri_building() { | ||
let r = Requester::new(ApiVersion(2), None); | ||
let result = r.build_uri(&EndPoint::account_bank); | ||
assert_eq!(result, "https://api.guildwars2.com/v2/account/bank"); | ||
} | ||
|
||
#[test] | ||
fn uri_query() { | ||
let requester = Requester::new(ApiVersion(2), None); | ||
|
||
let r = reqwest::blocking::Client::new() | ||
.get(&requester.build_uri(&EndPoint::items(ItemId(2000)))) | ||
.send() | ||
.unwrap() | ||
.text() | ||
.unwrap(); | ||
println!("{}", r); | ||
|
||
let k: Item = reqwest::blocking::Client::new() | ||
.get(&requester.build_uri(&EndPoint::items(ItemId(2000)))) | ||
.send() | ||
.unwrap() | ||
.json() | ||
.unwrap(); | ||
// todo: what to assert here. | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should allow the merging of feature branches with this line. There appears to be a lot of dead code in this review and seeing the build output would help a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that sounds good to me.
Though without this or non module dead code many of the types that are used for the individual EndPoint enums thing, all get marked as dead code, which i'm still a bit confused by, because they do not get constructed. But they need to exist for the enum to know what their types are? So thats kinda confusing to me.