feat: http client and eco support

[CosminPerRam]

A lot of games that have a proprietary protocol hosts their query capabilities via a simple http server, on a GET query.

In this (unfinished, concept) PR, I have attempted on adding Eco, a game that provides its query information on its ip and port (+1 for query) and /frontpage, resulting in http://{ip}:{port}/frontpage.

I added HTTP support via Reqwest, as I find it easy to use and provides a blocking API (supports async for future use).
I thought initially on using Hyper, but I find it quite complicated compared to Reqwest.

Some caviats:

• Reqwest doesn't support separate read and write timeouts, they group these 2 as one timeout setting.
• • I for one find this to be understandable, I personally never encountered a situation where I needed separate timeouts for these and would suggest doing the same for the socket timeouts.
• serde_derive dependency, as almost all http queries return json data, we need structs with the Deserialize derive. [EDIT: we could leave it as is now and think about this later on]
• • We could go around this, just collect data as HashMap<String, String> then parse every field, sounds alright to me but I'm not really sure on it (how manageable would this be).
• [Mentioned by @Douile]: How do we set if the HTTP request is secured?
• • (Some possible solutions in my first comment)

Open to talk about possibilities and problems on this, as it's something that'll be used a lot.

[Douile]

I think this is a great way to start with HTTP protocols: make it as easy as possible for new games that use it to be added. In my opinion the biggest downside to using reqwest is that it pulls in all the tokio dependencies but I don't think that should get in the way of progress (especially since we might make gamedig async in the future anyway).

The only thing I'd say about the current implementation (I appreciate its a draft so I might be getting in early with comments, sorry) is that concat_path might be a bit limiting: other users of HTTP might need to construct the "address" part of the URL differently. For example if a game uses https (which reqwest does support) this currently doesn't allow that, or for master server requests (like Epic Online Services) the address of the URL won't necessarily be the IP of the server.

rust-gamedig/crates/lib/src/http.rs

Lines 21 to 31 in 285bd7f

	Ok(Self {
	client,
	address: format!("http://{}:{}", address.ip(), address.port()),
	})
	}
	pub fn concat_path(&self, path: &str) -> String { format!("{}{}", self.address, path) }
	pub fn request<T: DeserializeOwned>(&mut self, path: &str) -> GDResult<T> {
	self.client
	.get(self.concat_path(path))

Perhaps we allow the caller of request just provide the full URL. I can imagine there is some fancy trait way to have a per-protocol Server address to Url function but the simplest is probably the best way to start.

Nice work, I think this will be a great addition.

[CosminPerRam]

In my opinion the biggest downside to using reqwest is that it pulls in all the tokio dependencies [...]

Agreed, but I guess we could easily swap it eventually if we want to (saying this as to just get it working, then optimizing later).

Same goes with the 2nd caviat (serde_derive dependency), alright for now, we could (?) swap it with a HashMap eventually).

For example if a game uses https [...]

Well said, I'm thinking of 2 ways we could do this:

• Add an additional field in ExtraRequestSettings, something like secured, which would apply only for http requests, defaults to false (as there are only a handful of https ones (like EOS)).
• Swap IpAddr (in address: &IpAddr) or SocketAddr with a wrapper for the additional possibility of a URL.

[...] or for master server requests (like Epic Online Services) the address of the URL won't necessarily be the IP of the server.

I don't exactly understand whats the problem here, in the case of when we get a server info via a master server, we always have the master address beforehand (constant) and we just require the IP of the server to find in the response list, am I missing something here?

[Douile]

I guess we could easily swap it eventually if we want to

Yes this should be pretty easy to do once we have the HTTPClient API in place.

Same goes with the 2nd caviat (serde_derive dependency), alright for now, we could (?) swap it with a HashMap eventually).

I don't see the need for this, serde_derive is doing the work of extracting the fields into a struct for us and we already use it in other places. Unless the server returns fields that don't follow a set schema I don't think we should return a HashMap.

Swap IpAddr (in address: &IpAddr) or SocketAddr with a wrapper for the additional possibility of a URL.

I like this option as it is more flexible.

I don't exactly understand whats the problem here, in the case of when we get a server info via a master server, we always have the master address beforehand (constant) and we just require the IP of the server to find in the response list, am I missing something here?

I guess I was thinking about how this API would be consumed in a different way:

• I was thinking for servers that query a master server the query function would construct a new HttpClient with the address of the server queried and not the master server. But you're absolutely right in this case we would just create a HttpClient with the address of the master server and then provide the IP to request in the path.

---

It would be useful to be able to put domain names into the URL I think (some servers and/or reverse proxies will only return the correct result if the Host header is set correctly, and for HTTPS it would be required if the certificate is issued for a domain) but that would be solved by one of the solutions you mentioned before. Or perhaps we could re-use ExtraRequestSettings.hostname for that.

Add an additional field in ExtraRequestSettings, something like secured, which would apply only for http requests, defaults to false (as there are only a handful of https ones (like EOS)).

Paired with this we could have something like this:

struct HttpRequestSettings {
  secure: bool,
  hostname: Option<String>,
}

impl HttpClient {
 // ...

 /// Create a URL for use in request based on the set address, provided path, and request settings
 fn prepare_url(&self, path: &str, settings: &HttpRequestSettings) -> String {
   format!("{}://{}{}",
     if settings.secure {
       "https"
     } else {
       "http"
     },
     settings.hostname.unwrap_or_else(|| format!("{}", self.address)),
     path,
   )
 }
}

The downside to this is you always need to provide an address, but if you also provide a hostname that won't be used which means we might do a useless DNS lookup.

We could always use the provided address in the URL and then use the hostname for the Host header in the request but I'm not sure if that would work with HTTPS and reqwest.

But overall the other solution using something other than SocketAddr for address might be the better option.

[CosminPerRam]

I unfortunately didn't have the time to continue to do said changes and I don't think I will this week at least, if anyone would be up to continue I'd be glad.

[Douile]

@CosminPerRam I made some changes to HTTP client (unfortunately I couldn't separate it into atomic commits), more details are in the commit message: 723f2f5

Let me know what you think :).

[guilhermewerner]

I was taking a look at the code to see how I'm going to add EOS support, and a question came up.

I saw that to create the HttpClient an IpAddr is passed, which in the case of the ECO game makes sense, but in the case of games that use the Epic protocol, it is the epic api address that is passed, and not the server's IP, how would I do this then?

Like, I need to be able to create the HttpClient with a hostname (api.epicgames.dev) and not the IpAddr directly.

[CosminPerRam]

<Authors cant review their own PR's>
Great works, very clean, good to go from me.

[Douile]

I saw that to create the HttpClient an IpAddr is passed, which in the case of the ECO game makes sense, but in the case of games that use the Epic protocol, it is the epic api address that is passed, and not the server's IP, how would I do this then?

In this case with the current implementation you would need to lookup the IP for epics server using ToSocketAddrs and use this for the IP address when creating the HttpClient (remembering to set the servername to the correct hostname). This is what would happen internally when you pass a domain name to most other HTTP clients.

It would be possible to instead take hostnames as the main parameter and then have a forced IP address as an extra setting its just a case of which use case we want to make easier.

good to go from me.

Thanks, there's a few formatting and conditional compilation issues I will push a fix for next week. And also we should discuss the API changes above. After that I'll ping you to make it a non-draft PR because I don't think I can do that.

[guilhermewerner]

In this case with the current implementation you would need to lookup the IP for epics server using ToSocketAddrs and use this for the IP address when creating the HttpClient (remembering to set the servername to the correct hostname). This is what would happen internally when you pass a domain name to most other HTTP clients.

It would be possible to instead take hostnames as the main parameter and then have a forced IP address as an extra setting its just a case of which use case we want to make easier.

I got it.

Another thing that would be necessary in the case of epic are headers, to pass the authentication token.

[Douile]

Another thing that would be necessary in the case of epic are headers, to pass the authentication token.

I'm not sure the best way to do this. Should we have per-client headers or per-request, or both?

Do we also need to access the response headers for things like rate-limiting or pagination?

[Douile]

I've added support for headers to http and now allow passing extra settings for eco queries. This is looking like it's almost ready, let me know thoughts on the latest changes.

We might consider removing the serde feature as the conditional compilation is getting overly complex (and might be broken right now). Or maybe we just have a serde feature that enables serde derivations for all our types but also pull in the dependencies (I think the proc-macro derivations can be quite slow).

[CosminPerRam]

We might consider removing the serde feature as the conditional compilation is getting overly complex

Well said, as we already use serde, we will only continue relaying on it (as many games will need json parsing).

Or maybe we just have a serde feature that enables serde derivations for all our types

Sounds like a good solution.

[Douile]

Sounds like a good solution.

Unfortunately its not as cut and dry as I initially thought. The types for games that require JSON parsing e.g. eco will always need serde derive.

[CosminPerRam]

Hey, not a complain, but I got a curious about this, why have you inlined these functions?

[Douile]

To hopefully reduce call cost of the request functions as they are all essentially wrappers of these (at the cost of binary size). It's a bit of a pre-mature optimization I have done no tests to confirm that this is better (the compiler might've inlined these anyway).

[Douile]

@guilhermewerner do you have any comments before I merge?

Your use-case would now be something like this:

let client = HttpClient::from_url("https://api.eos.com", &None, Some(vec![("X-Client-Token","Token")]));

let response = client.get_json("/server_state")?;

[guilhermewerner]

@guilhermewerner do you have any comments before I merge?

Your use-case would now be something like this:

let client = HttpClient::from_url("https://api.eos.com", &None, Some(vec![("X-Client-Token","Token")]));

let response = client.get_json("/server_state")?;

I think everything is ok. Another necessary thing is to be able to create a form post, but I managed to add this when I was testing.