Skip to content

API-compatibility with WebServer.h? #64

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

Closed
jackjansen opened this issue Dec 2, 2019 · 22 comments
Closed

API-compatibility with WebServer.h? #64

jackjansen opened this issue Dec 2, 2019 · 22 comments
Labels

Comments

@jackjansen
Copy link
Contributor

Is there a chance of getting API-compatibility with WebServer.h?

Or, alternatively, could you guide me in the direction of what would be needed to do that (so I could do it myself and share the code, if you're interested in accepting it)?

I think your API is probably superior to the WebServer.h API, but it has the disadvantage that it's only available for ESP32 (correct?).

I maintain a framework that runs both on ESP32 and ESP8266, and it can be built for either http or https support, and for 3 out of the 4 combinations I can use the WebServer.h API (or the very similar ESP8266WebServer.h or ESP8266WebServerSecure.h APIs).

@fhessel
Copy link
Owner

fhessel commented Dec 3, 2019

I'm not a heavy user of WebServer.h, but, to me, the core of the APIs seems to be similar within a certain degree: You have the server instance and you can define handler functions. The main difference with this implementation is that the server can handle requests in parallel, so it passes request and response objects to the handler function. This is somewhat inspired by the way it is done in JS frameworks like Express (which also holds for things like middleware etc.). However, this prevents a direct mapping from WebServer.h code to this server, as reading requests and writing responses has to be done on the request-specific objects, not on the webserver itself.

And this lib is a bit more oriented in the direction of the C++ standard library (usage of std::string etc.) than to some Arduino classes.

I think it could be possible to write a thin wrapper around the library, that does the aforementioned mapping of types and function calls. But I would put that wrapper code into a second library, so that your code can depend on that library, which again depends on the one in this repo. That way, one could switch between WebServer.h and the wrapper transparently, while the more powerful API of this library is still accessible to all users needing it.

I think your API is probably superior to the WebServer.h API, but it has the disadvantage that it's only available for ESP32 (correct?).

Correct. The main goal when I started this project was to have an HTTPS server on one of Espressif's chips, and the ESP32 has the advantage of having hardware support for AES, SHA and multiplication of big numbers, which is handy for the required asymmetric crypto (RSA, DH). In fact, in the beginning the server was HTTPS-only, and as the TLS-support for the ESP8266 was still quite experimental and the performance isn't very well on that chip either, I didn't consider it as target platform.

So the TLS part is what makes this library specific to the ESP32. If you throw out everything that has "HTTPS" or "SSL" in its filename, the plain HTTP part should be working or need only very few modifications. At least, if the lwip network stack does not differ too much between both platforms. And having a compile-time switch to disable HTTPS completely has been on my list for some time now anyway.

So if one would want to proceed in that direction, there are two main tasks: ESP8266 compatibility and wrapping the API. To achieve compatibility, I'd first add that HTTP-only flag, then get HTTP running on the 8266, and finally create the platform-specific code to deal with TLS on the 8266. The wrapper project could be started directly and should work on the ESP8266 once the compatibility has been reached in the main code base.

What do you think of that approach?

@fhessel fhessel added the feature label Dec 3, 2019
@fhessel
Copy link
Owner

fhessel commented Dec 4, 2019

To make it a bit more precise, that wrapper layer could look like this. The hello server example is working on the ESP32 with HTTP (for now without parsing the args on the 404 page).

When finished, that setup allows exposing the underlying instance of this web server while providing API compatibility with WebServer.h. So one could easily migrate to this server, and then make use of its additional functionality.

@jackjansen
Copy link
Contributor Author

Frank, thanks a lot for the clarification! I'll think about it for a while, but it may be better to do the compatibility the other way around (i.e. build a wrapper around WebServer.h that has a similar API to esp32_https_server even though it wouldn't allow parallel handling of requests, etc) because that seems more future-proof.

@fhessel
Copy link
Owner

fhessel commented Dec 5, 2019

I think the other way round (Having the esp32_https_server API but using Webserver.h to manage the traffic) will be hardly possible, at least not without some limitations. Creating the request and response objects could be tricky without a connection context.

From what I've seen when creating the other repo, it should be possible to achieve nearly complete compatibility to the Webserver.h API with an underlying esp32_https_server. By adding functions like HTTPServer* ESPWebServer::getUnderlyingServer() or HTTPRequest *ESPWebServer::getRequest(), it will be possible to access the wohle functionality within a Webserver.h handler. That's no longer backward compatible with the default server, of course. But it could be helpful if one would like to migrate from Webserver.h to esp32_https_server, as all existing handlers can be kept as they are, but new handlers following the void handler(HTTPRequest *req, HTTPResponse *res) scheme can be created in parallel.

If the server implementation should remain interchangable, these additional functions must not be used.

@jackjansen
Copy link
Contributor Author

Good point. I'm making some progress (I forked your esp32_https_server_compat repo), enough progress that I haven't given up yet:-), but I'm running into the first issue, and that is that the compat wrapper needs access to things that aren't exported by esp32_https_server. Specifically, the arg(int) and argName(int) methods would need access to ResourceParameters::_reqParams.

I guess this means I have to fork esp32_https_server too, unless you have an idea about how I could work around this (and similar issues that will come up, undoubtedly) without forking...

@fhessel
Copy link
Owner

fhessel commented Dec 16, 2019

Did you have a look at #65? Maybe that already solves the issue, as you can iterate over the request parameters (now called query parameters). #62 contains the discussion related to that PR.

@jackjansen
Copy link
Contributor Author

Yes, #65 would solve the issue at hand. I'll wait for that to be merged. In the mean time I'll continue with the other functionality that I'll need (such as upload, which I'm really looking forward to:-).

@jackjansen
Copy link
Contributor Author

Turns out I also need to iterate over the headers (in the request). So can I ask for either (a) a similar interface to #65 for iterating over the headers, or (b) a method HTTPHeaders *HTTPRequest::getHTTPHeaders()? I guess the latter is easiest...

@jackjansen
Copy link
Contributor Author

I decided on the latter. I've created pull request #66 which implements this.

@jackjansen
Copy link
Contributor Author

And for the sake of completeness of inter-issue references: I also need #29 in some form to implement WebServer upload handlers.

@fhessel
Copy link
Owner

fhessel commented Dec 22, 2019

Turns out I also need to iterate over the headers (in the request). So can I ask for either (a) a similar interface to #65 for iterating over the headers, or (b) a method HTTPHeaders *HTTPRequest::getHTTPHeaders()? I guess the latter is easiest...

While the latter is easier, I think exposing an iterator interface would be a more consistent way of providing access to the headers. Also, the HTTPHeaders instance wasn't originally meant to be used outside of the server. So exposing it directly could lead to problems with object ownership. I'll have a look at it.

And for the sake of completeness of inter-issue references: I also need #29 in some form to implement WebServer upload handlers.

There is the highly outdated bodyparser branch, which you might already have found through the PR. In case of file uploads, only multipart bodies have to be considered, so we could focus on that first and do not need to add url-encoded forms directly. However, I think maybe the code in that branch could also be changed to provide an interator-like interface, for sake of consistency. I think parsing the body elements can only be done iteratively to avoid storing the whole request body in memory. So a random access method like getBodyPart(std::string name) wouldn't work anyway.

@jackjansen
Copy link
Contributor Author

While the latter is easier, I think exposing an iterator interface would be a more consistent way of providing access to the headers. Also, the HTTPHeaders instance wasn't originally meant to be used outside of the server. So exposing it directly could lead to problems with object ownership. I'll have a look at it.

Hmm, you're right. But practically speaking I presume people will understand that the HTTPHeaders* is valid only as long as the HTTPRequest* is valid. But: you could always rename my suggested getHTTPHeaders() to accessHTTPHeaders() or something like that to drive down the point. Or put the class in a sub-namespace like httpsserver::detail::HTTPHeaders or something like that.

@jackjansen
Copy link
Contributor Author

Got back to this after a good month of being busy with other stuff. I've come across various issues with form handling, and filed pull request #69 to handle one of them. I'm now going to have a look at the bodyparser branch, because that's not only needed for file uploads but also for forms with <textarea> inputs because they'll quickly run afoul of the 128 character URL limit of HTTPS_REQUEST_MAX_REQUEST_LENGTH.

@fhessel
Copy link
Owner

fhessel commented Feb 8, 2020

If you need any support with the compatibility layer, just let me know. I just didn't want to edit my repo because of the amount of commits that your fork was ahead already.

@jackjansen
Copy link
Contributor Author

The compatibility layer is coming along fine. I've given you access to my fork of the repo, just in case. I think the last bit of work is really getting the bodyparser branch to work, possibly after thinking a bit about the API. It's just slow going because I'm working on far too many projects, and this one isn't really in the critical path for any work-related ones.

@jackjansen
Copy link
Contributor Author

One last issue: is there a way I can easily get multiple path components when I create a ResourceNode? For static file serving, if the caller calls serveStatic("/public/", ...) the esp8266 WebServer will take the whole path after /public/, slashes and all. ResourceNode("/public/*", ...) will only match one level.

@fhessel
Copy link
Owner

fhessel commented Mar 19, 2020

The compatibility layer is coming along fine. I've given you access to my fork of the repo, just in case.

That sounds great, I just feel a bit reserved about merging things back as you've clearly done most of the work.

One last issue: is there a way I can easily get multiple path components when I create a ResourceNode? [...]

At the moment there is none, maybe something like ending a path with "/** could be an option, where the remaining path up to an optional ? is written to the path parameter?

@jackjansen
Copy link
Contributor Author

That sounds great, I just feel a bit reserved about merging things back as you've clearly done most of the work.

I thought about that, but my feeling is that you're better suited as the primary maintainer than me, because future changes will probably be mostly due to changes in esp32_https_server. And I don't need the GitHub geek points anyway given my history:-). But: I'm not trying to push work over the fence, so I'd be perfectly happy to (co)maintain.

At the moment there is none, maybe something like ending a path with "/** could be an option, where the remaining path up to an optional ? is written to the path parameter?

I was thinking of something along those lines. I didn't bother yet (unsure how much of an issue it really is, for the compatibility layer) but I could give it a try and create a patch. Let me know.

@jackjansen
Copy link
Contributor Author

@fhessel shall we try and close this one? The main issue is deciding whether you want to host esp32_https_server_compat (possibly with me as a co-maintainer, if you want), or if you would prefer not to and then I'll host it (with the caveats given above). I'd like to make some progress with this, because I'm getting ready for a new release of https://github.com/cwi-dis/iotsa but I'd really like the dependencies like esp32_https_server_compat to be in their final resting place with the platformio and arduino library URLs pointing to the right location before I do so...

@fhessel
Copy link
Owner

fhessel commented Apr 8, 2020

Sorry for the lack of feedback from my side. Then let's do this.

I will need a moment to check the code in the compat repository, but I'll start now. If you want to have a fully working build process and resolve dependencies by version number, that means that I need to draft a release for this repo as well, right? Currently that would be a major release as the API to request and path parameters has changed. That will take a moment but as there are a lot of pending changes which aren't released yet, 1.0.0 is overdue anyway.

From then on I would keep the version of both repos in sync, with an exception to the patch level, and use esp32_https_server~1.0.0 in the dependencies section in the library.json of the compat lib. That should assure that the build process doesn't break even if the master in this repo changes.

So what I'll do then:

  • Check Implemented compatibility layer esp32_https_server_compat#1 and merge it → see comment in PR
  • Create release 1.0.0 for this repository (Usually takes a day or two until the PlatformIO crawler picks it up) → done
  • Create release 1.0.0 for esp32_https_server_compat
  • Register esp32_https_server_compat with PlatformIO's and Arduino's library registry (Arduino sometimes takes a bit)

Once PlatformIO did pick it up, you can esp32_https_server_compat=1.0.0 as dependency without referring to the repository and you'll get a stable build.

I can maintain both repos, but I'll only be able to keep the compat layer up to this lib from a functional perspective. Adding the actions from this repo to build at least the examples for every push will help by providing a basic sanity check.
Sadly, I didn't find a good way to do the integration testing, at least by now. All tools that I tried somehow allow modifying the testing script by submitting a PR to this repo, so I would need basically a sandboxed WiFi deployment for that, which could be a project on its own.

@jackjansen
Copy link
Contributor Author

The new library has appeared in the platformio registry. I think we can close this one. Thanks a million!

@fhessel
Copy link
Owner

fhessel commented Apr 13, 2020

Thanks to you too for all the work on getting the body parsing done and implementing the compatibility layer!

I'll close this for now and keep an eye on the Arduino registry.

@fhessel fhessel closed this as completed Apr 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants