Skip to content
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

Implement REST Communication #26

Open
jkeon opened this issue May 4, 2020 · 6 comments
Open

Implement REST Communication #26

jkeon opened this issue May 4, 2020 · 6 comments
Assignees
Labels
effort-intense Sleep on it - 16 to 40 hours priority-medium Standard task, plan as you see fit. status-in-progress Tasks actively being worked on. type-feature New feature or request
Milestone

Comments

@jkeon
Copy link
Member

jkeon commented May 4, 2020

In pure CSharp land, we'll want to use HTTPClient.
But in Unity land, we'll want to use UnityWebRequests.

Is there a nice way to wrap them both into the same Anvil API?

@jkeon jkeon added effort-intense Sleep on it - 16 to 40 hours priority-medium Standard task, plan as you see fit. status-backlog Tasks captured but not yet planned. type-feature New feature or request labels May 4, 2020
@jkeon jkeon added this to the Anvil v1.0 milestone May 4, 2020
@jkeon jkeon added status-in-progress Tasks actively being worked on. and removed status-backlog Tasks captured but not yet planned. labels Jun 3, 2020
@jkeon
Copy link
Member Author

jkeon commented Jun 3, 2020

Here are my thoughts based on what we had before. Phased approaches are totally fine!

Features

  • RESTService was essentially a Command only we didn't have commands when it was written. Since we do have Commands now, I think it probably makes sense to have an AbstractRESTCommand or maybe just a RESTCommand that can be configured with the communication layer.
  • In pure C# land, we'll want to the use HTTPClient and related classes for the actual communication. In Unity we'll want to use UnityWebRequest . Ideally we want the RESTCommand to have the same API to the developer.
  • REST calls usually matter in terms of order to a server. You don't want to send Call A and send Call B and have Call B reach the server first. So we had the concept of a queue where Call A would go and once we got a response then Call B would go. Some calls the order doesn't matter so we were able to opt out of the queue but by default we were a part of it.
  • Support N amount of retries before failing. Should also wait N amount of seconds before retrying so we're not spamming the server.
  • Support N amount of seconds for timeouts. There are timeouts for the sending/uploading and timeouts for getting the response which are separate.
  • Should support Authorization.
  • Should support Cache Busting
  • Should support setting/reading Headers
  • Should support the various sending protocols - GET, PUT, PUSH, DELETE, etc.
  • Should support the various sending data - Query params, post body binary, post body json etc.
  • We had the concept of spoofing a call before. You could force the behaviour to be successful or a failure and supply the data that would come back on that. I don't think I ever used the force failure option, but I did spoof the success data to set up services before the server was ready.
  • Events for Success, Failure
  • There's the part of the communication where it could fail/success at an HTTP level. 404, etc. But there's also the part where the communication is successful (200) but the data returned by the server is bad. So there's a validation step there that was useful.

@jkeon
Copy link
Member Author

jkeon commented Jun 3, 2020

@ChristianPotvin Added some notes.
@mbaker3 Anything to add?

@mbaker3
Copy link
Member

mbaker3 commented Jun 3, 2020

Aside from what @jkeon mentioned I just want to make sure as much can be composed as possible so that any given call or project can select

  • Method of transport HTTPClient vs UnityWebRequest vs WebClient vs Socket - We don't need all the wrappers written, just the ability to build them against an interface and then make use of them on a given call.
  • Retry, timeout, etc.. values
  • Response parsing. We shouldn't assume responses for all projects or even every call in a project are JSON for example. When you're gluing together many 3rd party APIs you can be dealing with a variety of data formats and conventions.

REST calls usually matter in terms of order to a server. You don't want to send Call A and send Call B and have Call B reach the server first. So we had the concept of a queue where Call A would go and once we got a response then Call B would go. Some calls the order doesn't matter so we were able to opt out of the queue but by default we were a part of it.

@jkeon Is this something that needs to be explicitly implemented? Seems like adding into a ParallelCommand vs SequentialCommand would handle this just fine.

Should support Authorization.

Is this in addition to being able to manipulate headers? I thought Auth was all done in the header info.

@jkeon
Copy link
Member Author

jkeon commented Jun 3, 2020

Agreed with Mike's comments minus a couple clarifications below:

@jkeon Is this something that needs to be explicitly implemented? Seems like adding into a ParallelCommand vs SequentialCommand would handle this just fine.

Agreed it would, however that's a lot more work on the developer. Now at first you scoff and say, "no not really, just create your sequential command, fill it up with your calls and fire it off". But then you get into actually implementing in your app and you realize that the Sequential Command needs to be global since many different parts of the app could fire a REST call at anytime and they need to go in order. That Sequential Command also needs to live forever so it's probably better to be a BufferCommand too. And then after all that, why don't we just have that built into the system for you anyway?

Saves you work managing it, reduces race condition bugs where you forget to do it and your calls arrive on the server out of order.

Is this in addition to being able to manipulate headers? I thought Auth was all done in the header info.

You're 100% correct that you can support all Authorization via headers directly but C# has some nice helpers to properly format the header according to the standard:
https://docs.microsoft.com/en-us/dotnet/api/system.net.http.headers.httprequestheaders?view=netcore-3.1
https://docs.microsoft.com/en-us/dotnet/api/system.net.http.headers.authenticationheadervalue?view=netcore-3.1

So maybe it's not something we need as part of the core, but a nice helper for setting common Headers with the common inputs would be nice.

Ex. Bearer Authentication means:

POST - Key: "Authorization", Value: "Bearer <actual token>"
GET - &access_token=<actual token>

Sure you can definitely look it up and manually add it to your calls with a SetHeader call, but it would be much nicer to just say SetAuthorization(AuthType.Bearer, <actual token>) and the RESTCommand takes care of it depending on if you're POST or GET etc.

Same thing with a bunch of headers, we can probably make it really nice with enums. Agreed it's not critical and a nice to have though.

@jkeon
Copy link
Member Author

jkeon commented Jun 4, 2020

In thinking about this more and talking with Mike, it's probably fine to implement without a global queue for V1. We probably should have a good case to warrant adding it. Simple first is probably better.

@ChristianPotvin
Copy link
Contributor

Sounds good. I'll tap you with questions as I dig further into this and start forming a better understanding of what I'm getting into. In all likelihood, it will start simple and be built upon as I go until it resembles something everyone can be happy with. This is largely unexplored territory for me, but it's something I want to learn more about, and there's no better way to learn than to throw yourself into it and see where it takes you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort-intense Sleep on it - 16 to 40 hours priority-medium Standard task, plan as you see fit. status-in-progress Tasks actively being worked on. type-feature New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants