refactor(binding-http/server): introduce routes for each TD endpoint

[relu91]

Hi all, the goal of this PR is to declutter our HttpServer implementation. Over the years, it got a lot of new functionalities but it never had a second look at its software architecture. My intention was to maintain as much as possible intact the current code base, but also move the logic of handling requests to dedicated functions. Furthermore, as mentioned in the note below this PR introduce a routing library (taken from the well-known library Fastify). This simplified a lot the refactoring with the additional benefit to provide more flexibility in the future. The library is compact, small, and well-supported, so it shouldn't be a problem in the future.

There are plenty of other changes that I would like to add, but I didn't want to overload the changes shown here. I also spotted some bugs/discrepancies in the handlers of the different TD affordances, which proves again the benefit of these refactors. I'll fix the problems in the following PRs.

Note: that we are now use find-my-way for routing, which is a bit more complex than the previous approach. However, it is also more flexible and in the future, we can easily add more routes or let users configure them. Another caveat is the trailing slashes in the routes now map to a totally different path. For example, /things and /things/ are now different routes. In the future, we might review this behavior.

[relu91]

Took the opportunity to also remove the deprecated url.parse call. :)

[codecov-commenter]

Codecov Report

Merging #1030 (83f1d5b) into master (a005e1f) will decrease coverage by 0.19%.
The diff coverage is 51.05%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master    #1030      +/-   ##
==========================================
- Coverage   75.61%   75.43%   -0.19%     
==========================================
  Files          72       80       +8     
  Lines       14887    15184     +297     
  Branches     1431     1434       +3     
==========================================
+ Hits        11257    11454     +197     
- Misses       3597     3697     +100     
  Partials       33       33              

Impacted Files	Coverage Δ
packages/binding-http/src/routes/event.ts	18.51% <18.51%> (ø)
...ckages/binding-http/src/routes/property-observe.ts	20.40% <20.40%> (ø)
...kages/binding-http/src/routes/thing-description.ts	48.02% <48.02%> (ø)
packages/binding-http/src/routes/action.ts	54.54% <54.54%> (ø)
packages/binding-http/src/routes/common.ts	56.79% <56.79%> (ø)
packages/binding-http/src/routes/properties.ts	64.55% <64.55%> (ø)
packages/binding-http/src/routes/property.ts	67.85% <67.85%> (ø)
packages/binding-http/src/routes/things.ts	68.88% <68.88%> (ø)
packages/binding-http/src/http-server.ts	84.33% <100.00%> (+20.57%)	⬆️

... and 1 file with indirect coverage changes

[danielpeintner]

I think restructering and decluttering the code is good 👍 but also dangerous 🤔

You have comments about credentials checking saying
// TODO: refactor this part to move into a common place
Do you plan to move it into a common place in this PR? I think we should...

In general it is quite difficult to review all the changes and whether something is missing. I hope we have somehow good enough test coverage 😉

What concerns me a bit is that /things and /things/ are now different routes and I wonder whether this might break existing apps when updating?

[relu91]

Thank you @danielpeintner for reviewing and sorry for answering late.

I think restructering and decluttering the code is good 👍 but also dangerous 🤔

Totally agree. What I tried to do is to keep the code as much as intact as it was and verifying the counter example was working both in "headless" mode and in the browser. The tests are also passing, but as you know the coverage is not that great. Something to improve for sure in the future. I believe that this refactor would help introduce new tests and also new features.

You have comments about credentials checking saying
// TODO: refactor this part to move into a common place
Do you plan to move it into a common place in this PR? I think we should...

Yes, I plan to do so, but I didn't include the changes in this PR because, doing it right, would result in additional refactoring changes that I wanted to avoid cause it touches something delicate such as authentication. Plus this change would impact also #873. If you have a strong opinion I would defer it to another PR, as you said this is already a lot to digest.

What concerns me a bit is that /things and /things/ are now different routes and I wonder whether this might break existing apps when updating?

The router has a simple flag to change the current behavior. I was not sure that having /things and /things/ point to the same resource was correct (see also vaimee/zion#7 and Ben's comment). But I guess to make the number of breaking changes low I can allow the flag. Sending a commit soon.

[JKRhb]

Thank you for this PR, @relu91! :) In my opinion, the changes look pretty good, although while going through the code I found a lot of potential for further refactoring – maybe I'll keep the comments I prepared so far for myself for now, though, in order not to make things overly complicated for this PR and add them later or to a separate PR. If everything (or the things we can currently test for) is working as before, then I think we could go forward with this PR.

Regarding the trailing slashes: From my understanding, /things and /things/ should in fact be treated differently (I think the trailing slash introduces an empty path segment), but I am not entirely sure about it. Having an option to switch the behavior sounds very good, though!

[relu91]

Thank you for this PR, @relu91! :) In my opinion, the changes look pretty good, although while going through the code I found a lot of potential for further refactoring

I bet you found them! As I said in the intro:

There are plenty of other changes that I would like to add, but I didn't want to overload the changes shown here. I also spotted some bugs/discrepancies in the handlers of the different TD affordances, which proves again the benefit of these refactors. I'll fix the problems in the following PRs.

Keep your comments and maybe we can discuss your points in our Discord channel to further coordinate.

Regarding the trailing slashes: From my understanding, /things and /things/ should in fact be treated differently (I think the trailing slash introduces an empty path segment), but I am not entirely sure about it. Having an option to switch the behavior sounds very good, though!

So what are you suggesting here? to keep the current status (e.g. treating them as before) and expose the Router option to the user?

[JKRhb]

So what are you suggesting here? to keep the current status (e.g. treating them as before) and expose the Router option to the user?

I guess that could be a good solution to not introduce a breaking change for now. For a new major (or at this stage: minor) release, we could then simply switch the default.

Keep your comments and maybe we can discuss your points in our Discord channel to further coordinate.

That sounds good! :)

[danielpeintner]

I agree, once we have other breaking changes we can integrate this breaking change as well

[relu91]

@danielpeintner @JKRhb given our discussion, we should be ready to merge this.

[JKRhb]

Thank you, @relu91, LGTM :)

[danielpeintner]

LGTM as well. @relu91 feel free to merge and we can continue to work on the remaining TODOs left in this PR.

BTW, I created #1037 to keep track of using the default option for not ignoring slashes

[relu91]

LGTM as well. @relu91 feel free to merge and we can continue to work on the remaining TODOs left in this PR.

BTW, I created #1037 to keep track of using the default option for not ignoring slashes

Thanks! 🥳