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

HTTP GetRequest workflow #20

Open
al-fisher opened this issue Nov 28, 2019 · 5 comments
Open

HTTP GetRequest workflow #20

al-fisher opened this issue Nov 28, 2019 · 5 comments

Comments

@al-fisher
Copy link
Member

Have been playing with the OpenStreetMap_Toolkit leveraging the HTTP_Toolkit 😍 😍 😍
(@rolyhudson)

Did lead to some thoughts around potentially consolidating the http requests to more clearly align with other areas of the BHoM. A few notes for comment - @epignatelli @alelom picking up from our chat earlier. Low priority and great to enable these experiments as some thinking needed I think to create a satisfactorily slick, consistent and clear work flow.

There are currently two implementations of HTTP requests -

  1. following the Adapter/Pull work flow
    Feeding a GetRequest into a Pull with also a HTTPAdapter
  2. A simpler workflow mirroring the expression of a single http request string. Through
    BH.Engine.HTTP.Compute.GetRequest(url) which directly returns the string response

The 2. above is neat - but needs to move out of Engine Compute as is externally interfacing.

The challenge we have is that a traditionally formatted http request contains the domain (i.e. source or adapter in BHoM terms) actually embedded in line with the request itself.
Our standard adapter Pull and Push workflows have naturally separated these concepts out.

Useful to align terminology and concepts where we can - but also be intuitive and respect conventions of the software/platforms we are adapting to.

The main comment I discussed with @epignatelli was to ensure clarity that a link (or adapter) with the outside world to BHoM is still being made. Even if not a standard Adapter -> Pull

So few options:

a)
This has redundant information for http as described above.
image

b)
Could separate out domain and rest of request arguments for the http string - but I think this is unintuitive if already familiar with performing http requests. Others opinions here are welcome - but feels we are forcing too hard into BHoM format!?
image

c)
Could allow not specifying adapter - where is implicit from the request?
image

d)
Could then enable implicit casting of correctly such that work flow might allow
image

e)
An option that might make sense is to achieve very close to the original BH.Engine.HTTP.Compute.GetRequest(url) , respecting the current exception of http, but migrating it to an Adapter NameSpace such that we could have something like BH.Adpater.HTTP.PullRequest(url).
image
This would not currently reflect into UI - so considerations needed there.

f)
Another final option would be that we do this as an extension of the Execute...

image

With this option I think we come close to some of the ideas we had before, where we considered implementing Executes with the actual Adapter not needing to be explicitly defined as additional input - being implicit in the Method you were executing.
This came up originally from discussions about executing external Python scripts etc.
Wonder if this might be a way forward?

Sorry for long notes - wanted to capture thoughts and discussions.
Perhaps one to pick up over a call?

@epignatelli @alelom @adecler @rolyhudson

@al-fisher al-fisher changed the title GetRequest HTTP GetRequest workflow Nov 28, 2019
@al-fisher al-fisher added this to the BHoM 3.1 β MVP milestone Nov 28, 2019
@enarhi
Copy link
Member

enarhi commented Jan 27, 2020

Best workflow looks to be a for now with d to follow. See snip below for current working example of creating request>pulling. Pull calls MakeRequest, and handles translation of the request to objects as appropriate. Having just gone through the motions of getting acquainted with this toolkit that is the most intuitive to me. We should then hide the Compute.GetRequest methods that MakeRequest calls, that will eliminate confusion. This also clarifies what the accepted input params for a GetRequest are (string baseUrl, dic headers, dic params). @al-fisher @epignatelli @alelom let me know your thoughts, maybe a call to get this dialed in but we are using HTTP_Toolkit for ongoing workflows so happy to have this settled ASAP.

image

@epignatelli
Copy link
Member

epignatelli commented Feb 6, 2020

Thanks @al-fisher @alelom @enarhi @michaelhoehn for the chat. Here's a summary of the conversation:

The plan is to be able to use the Pull component without and adapter plugged in. This brings us to choosing the option c): not specifying adapter where it implicit from the request.

We can do this by creating an IActionableRequest interface, which inherits from IRequest.
An IActionableRequest is a request that contains all the information to perform a task, i.e. does not required complementary information from an IBHoMAdapter.

All of this can be done by action only on the BHoM_UI, the BHoMAdapter, and the Data_oM. It does not require to refactor all the other adapters.

Are we all happy about it? If so, we woudl need a more detailed plan and raising the issues in the right place.

@IsakNaslundBh
Copy link

Sorry to jump in here a bit late, and without having been part of the wider discussion before, but this was mentioned on the global sprint closure today.

My first thought and question is:

Why is this any different then any other adapter?

Ofc, might have missed some context from calls etc, but I still cant see the argument why you should not provide an adapter? Why hide the complexity in the request itself? It still contains usefull information, just by the fact that it exists, stating "I want to pull from the web".

I understand that currently no information needed is stored in the adapter, but that could change in the future, for things like settings etc?

For arguments sake, I could quite easily add a EtabsBarRequest : IActionableRequest accepting a file path to an Etabs model that would work just fine without the etabs adapter, making it responsible for starting Etabs, extracting bars, converting them, etc., but that is not how any of the adapter work, and I would not want to do that. The same could be said for adding a similar Compute method (I know that has already been ruled out).

Again, might have missed something, but reading through this thread, I do not really get the "why" for this.

@epignatelli
Copy link
Member

epignatelli commented Mar 6, 2020

Minutes from the call @al-fisher @alelom @IsakNaslundBh :

We are going to keep the HTTPAdapter to be plugged in the Pull as it is today. We will simplify the problem in the Request by casting the string to a CustomRequest: a GET if Pull a PUT or POST if Push (you can specify that in the PullType).

We start with this first refactoring and we take it from there.
The idea to move further is to understand how REST, HTTP, or any other protocol (e.g. mongodb://, ftp://, file://) can be summarised together into a Protocol` engine.
This has to be taken to a wider discussion, though.

FYI @enarhi @michaelhoehn

@enarhi
Copy link
Member

enarhi commented Mar 6, 2020

@epignatelli sounds good, will be key to check how the changes effect capabilities in terms of more complex GET request parameters, happy to test as we refactor on my end to check that the current REST GET workflows are maintained / improved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants