search

vipm-io / http-rest-client

HTTP REST Client Library for LabVIEW
BSD 3-Clause "New" or "Revised" License
30 stars 18 forks source link

Regressions caused by `Add Http PATCH` #27 for Mac and Linux users #29

Open jimkring opened 10 months ago

jimkring commented 10 months ago

Hi @francois-normandin and @ChrisCilinoGC. The changes in #27 have caused problems for Mac and Linux users, since the ni_lib_advanced_http_client_api library is only supported on Windows (at present).

Are there any quick/reasonable fixes to this issue (that will cause the code to be unbroken for Mac/Linux users)? Conditional Disable frame around the Windows-only parts?

jimkring commented 10 months ago

For what it's worth, PR #17 could be a nice long term solution, by changing the main API into an interface and allow us to support several implementation that have the platform support and other features needed.

francois-normandin commented 10 months ago

Indeed. So the work is to replace with an interface and have an advanced http plugin/extension package, correct?

A quick fix might be in order, as you propose, to conditionally disable the feature on Linux and Mac.

I am quite surprised that NI's advanced library is not cross-platform. I admit I had not checked that before making the change. It's actually listed as supporting only Windows in the package released by NI...

@jimkring could you check quickly with Darren Nattinger to see if support for Linux and Mac might be in the works? It's a long shot...

jimkring commented 10 months ago

@francois-normandin I created an issue here https://github.com/dnattinger/ni-adv-http-api/issues/4 and did hear back from @dnattinger in the VIPM discord here.

PS: You'll find a link to a discord invite at the bottom of this page: https://docs.vipm.io

jimkring commented 10 months ago

TLDR; Darren grabbed this from NXG and built the DLL in Win32. He's going to look into whether it's feasible to get a Mac or Linux build going. Ideally, NI would just make the source code open and then we could set up CI/CD as a GitHub Action. We'll see.

Regarding your question: So the work is to replace with an interface and have an advanced http plugin/extension package, correct?

Ya, that's right. I would also start simple and keep everything inside the REST API package as we work on our refactoring of the interfaces. I don't want to create external dependencies and multiple packages, right out of the gate. It will simplify development and testing to have everything in a single repository so that we can detect interface changes/breakage and do more complete unit testing. Then, once the interface has stabilized we can make it more dynamic/plug-able.

francois-normandin commented 10 months ago

I agree, the "default" implementation should be in the main package (probably even after the refactor).

francois-normandin commented 10 months ago

There are three approaches that I can see:

  1. Single HTTP Client interface where REST API is a wrapper on the Interface that contains all the methods
  2. Multiple interfaces: http-client + http-methods (get, put, post, etc.) where the REST API inherits all of them except patch method, and we create an Advanced REST API class that inherits from REST API + http-method-patch.
  3. Match the interface to the REST API methods (exactly) and make the REST API be the implementation (since the public VIs are dynamic dispatch).

Although the second one is elegant, I chose the first approach. (I discarded the 3rd approach because I do not think public VIs should be overridable: it suggests to the developers that the Liskov principle is not important...) The first approach is simple and easy to implement with little refactor. It comes at the cost of having an error when calling the Patch method on Mac and Linux, but I've made it such that it throws a 405 Method Not Allowed error as if the server were to reject the method. It is not perfect, but seems a "good enough" choice under the circumstances.

We can break out the Advanced http-client class into a separate package if desired. With conditional disable structures, I believe it should work if we force to install without the NI Adv Http Client package on Mac/Linux. The code should be runnable (Conditional Disable Structures), but I now doubt that we can use this approach for the release because the package might not be installable on those platforms... (at least not in an automated pipeline ??)

Merge Request is here: https://github.com/vipm-io/HTTP-REST-Client/pull/30

Includes a ready-build package (must rename extension to vip) to try it out. https://github.com/vipm-io/HTTP-REST-Client/files/14074565/jki_lib_rest_client-2.1.1.40.zip

jimkring commented 10 months ago

@francois-normandin. You proposed approach, by first starting with Single HTTP Client interface where REST API is a wrapper on the Interface seems like a good one. I'll review the PRs...