[DISCUSS] Request for Feedback on Beta Release

[jimkring]

We'd love your feedback and help testing the new HTTP Client Abstraction package:

• We just released a beta version of the HTTP Client Abstract base class on vipm.io (here). You can download and install it with VIPM.
• We also released a beta version of a plugin based on the NI Advanced HTTP Client on vipm.io (here).
• Lastly, we're working on a plugin based on JKI's awesome new reqwest-rs-labview library, which is currently located in source code (here) and not yet packaged up -- to try this out, you'll need to clone the reqwest-rs-labview repo.

The goal for this http-client-plugin-base package is to provide a drop-in replacement for the built-in LabVIEW HTTP Client with an identical API (all VIs and connector panes the same).

The means:

• When you're creating a new application, you can use this library, just as you would the built-in HTTP Client, but your code will be "future-proof" allowing you to easily upgrade the actual HTTP Client implementation, should you need to (for performance, platform compatibility, security, etc.).
• If you are maintaining an existing application that uses the built-in HTTP Client, you can easily upgrade/migrate to this new library and it will "just work" the same way it currently does, enabling you to easily switch between the built-in implementation and other implementations for testing/evaluating possible improvements to performance, stability, security, etc.

The palette for the HTTP Client Abstraction is in the Add-ons folder of the functions palette, so it's pretty easy to find. There is also a "VI Tree" that includes and example you can use for testing.

Here is an example of the reqwest-rs-labview implementation running "side-by-side" with the built-in implementation:

Thanks for your feedback and help testing!

[negentropicdev]

1. does reqwest cull the first http response line? Would be a good idea to ensure that portion is normalized. Perhaps peel it off in the default implementation and provide that info separately since technically they're not headers anyways.
2. If the base class isn't going to have any data why not make it an interface?

[negentropicdev]

Yeah, there's no standard way to get the status # back and if it isn't in that headers string in reqwest I don't see any way of getting it. Properly handling response code is critical and not being parsed out automatically by the shipping client in LabVIEW has always irked me.

Are you expecting any behavior in new clients that isn't available in ni adv http? Or is the plan to basically wrap everything and make it adhere to basically what the ni adv http does?

[justiceb]

The advanced HTTP client does actually add status code as a numeric output terminal to all the methods.
I was planning to add this.
I agree that this is very important and a first-class citizen.

I was planning to just parse status code in the same was as the JKI REST library to get status from the header for the base HTTP client implementation.

I did see that the reqest library header response is missing the first line of text, including the HTTP version and code.
In my mind, a rather important intent of this library is to make it a drop-in replacement for the base HTTP client, to make refactoring old code easy.
For this reason, I think it's best to actually add this line of text back in for reqwest's implementation, rather than culling it from the others.

I think this needs to be classes because the implementations have their own handle data.

The base HTTP client is missing the PATCH verb. I saw the NI AWS library get around this by added a "X-HTTP-Method-Override" header with a value of "PATCH". It's unclear to me how well that works, but maybe that's worth adding to this library to hack in PATCH for the base lib?

[jimkring]

Hi @negentropicdev @justiceb.

We could have made this an interface, and that idea was evaluated. However, this would mean that another terminal would be required, in addition to the existing client/session reference, on each method (so that the interface could be wired up to the method VIs. That got messy and it seemed that a class was a "better" way to way to go.

[Update: I don't know what I want thinking -- an interface works great for the base "class".]

Side note: I wish we had traits (with data) in LabVIEW instead of interfaces, but I digress.

Regarding the missing header line for HTTP version and code, I agree with you that we should try to be 1:1 compatible with this behavior of the built-in HTTP Client. I also think it would be good to add the HTTP code as an integer output, just like the NI Advanced HTTP Client -- this is very useful to have.

[Update: Work on the header line for HTTP version and code is happening in this pull request]

I'm open to adding new methods to the base library that are obviously missing (e.g. those that are important for an http client to support, in general) where the method interface is in the same spirit/style of that of the built-in http client. That said, I'm not sure if we want to create/maintain a super-awesome http abstraction that supports all use cases -- at least not right away. My main goal would be to give existing applications (that use the existing built-in client) an easy way to take advantage of the performance improvements of reqwest-rs-labview. Then, once they want things like asynchronous and other advanced capabilities, they can migrate over to using the reqwest library directly.

That's how I'm thinking about it, at least...

[jimkring]

@negentropicdev @justiceb I have added the status code output to all of the HTTP Methods in the abstract class, the built-in implementation (added a support vi for parsing the header to get the status code), and the ni advanced implementation (just wired them up). I just released version 0.2 of both packages on vipm.io. Will add support for this to the reqwest-rs-labview implementation and will consider how to simulate the "HTTP/1.1 200 OK" first line in the response header string output.

[jimkring]

@negentropicdev I now see how making the base class an interface works -- I got my wires mixed up (as I haven't used interfaces much yet, frankly). It actually works great to have the "base class" be an interface, since it has no data that needs to be shared. I did create a default implementation of Open Handle.vi that calls the built-in implementation. This creates a circular dependency since the built-in implementation class inherits from the interface and the interface calls a VI in the built-in implementation. This saves the user some trouble when they wire in the interface constant instead of an implementation class -- they will get the built-in implementation...

How big of a crime is this cyclical dependency?

CC: @justiceb

[jimkring]

Work on adding the header line for HTTP version and code is happening for reqwest-rs-labview in this pull request

[jimkring]

I added support for returning the http version information, but there's no way that I can see to get the full first line of the response, including the reason (like "OK"). Apparently (from the little research I've done) HTTP/2 doesn't support reason phrases at all.

Here's how far I've gotten (you'll note that the reqwest version is missing the "OK" reason phrase):

Where should we go from here?

Will this be acceptable for most users?
I have to think that most users will care more about the status code and now that we're outputting that directly in each of the HTTP method VIs, that'll be a significant help. Side note: I should probably also work to plug this into the JKI HTTP REST Client too, to see if it'll work with the reqwest plugin, right "out of the box".

Should we try to simulate the reason phrase?
This feels like a slippery slope -- it's easy to just add "OK" when the status code is 200 but I'm sure it'll get a lot more complicated. Is it better to simply not support the reason phrase (and break someone's code) than to do a "somewhat simulated" job of it? If their code breaks because of this, then they can just use the status code which will be much less brittle for them.

[negentropicdev]

I don't see that tiny circular dependency as a huge failing. The base HTTP client is always included with LV (Though actually I think you're using Adv as the default?). You don't need that Preserve Run-Time Class in the default implementation as the default class input will always be the base interface and that's guaranteed to be a parent of the default http implementation. If the default is the Adv library then I would avoid the creation of the default as a default implementation and just throw an error.

The status code text is largely canned based on the return code: https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status if you look in my server I always just pick the text based on that status code. You could check if the version is < 2 and add in the text though I think that'd be relatively easy to work around in peoples' code. At least there's a first line that can be stripped off to do header parsing that a bunch of people are likely doing and just assuming the first line is the status.

A personal preference of mine is to have a static VI in each child class that replaces the pattern of class cube + an init vi (open session). They can typically be inlined since in this case they'd just be hard coding a class cube and calling Open Handle.vi but it reduces the footprint in the calling BD significantly and is really convenient with palettes and quick drop just having to drop a single thing. So things like "Reqwest Open Session.vi" and "NI Adv Open Session.vi" etc.