Simplify server setup

[joachimvh]

Probably going to keep this branch open for a bit until it feels like a finished whole.

Feel free to comment on commits that might be doing something wrong.

[termontwouter]

Hi Joachim, thanks for working on this. 💪

Just a few thoughts at this point of the process. Overall, I agree with most of the changes, and, on the few points that I raised an eyebrow, I'm mostly happy to ignore our different approach and let you take care of those implementations decisions.

My main remark is that a number of changes fail to live up to the title of this PR ☝️ They bring the code more in line with the CSS codebase, but do so at the cost of some of its initial simplicity. For example, one commit simplifies the use of input arguments from context.request.* to request.*, but a later one changes this to input.request.* and input.operation.*; in other places, simple objects with native Node.js HTTP fields are swapped for wrappers that are not strictly necessary (at least for now). While I am okay with that in as far as alignment with the CSS is desired, at least on two points this does not seem to bring any added benefit:

• The first is the 50-line BaseTargetExtractor, which wraps an OriginalUrlExtractor just to change one of its parameters, yet only saves about 5 lines of the original code. If even for those few lines you'd rather use the CSS mechanism, can we not implement the specific IdentifierStrategy alone, and pass it to the OriginalUrlExtractor in config?

• The second is the routing mechanism. I fail to see how the new InteractionRouterHandler (with a trivial MatchAllRoute to make up for its complexity) is better than the original router handlers. They both basically work as a map; the original code does so very simply, the new code in a roundabout way. Something in the back of my head screams a vague "pre-opture matimization" 🙈

We've definitely reached a milestone though: a Handlers.js-less codebase 😄 🚀

[joachimvh]

So "Simplify the server setup" might indeed be a bit too ...simple of a title. Other aspects are robustness, future extensibility, etc. But I agree that this PR currently is not perfect, it also has some aspects that I'm not completely happy with yet.

One of the main reasons for trying to solve as many problems as possible with CSS components is to then potentially extract those from the CSS repo itself into separate repos so we, for example, have a standard library for the initial HTTP server setup. That way, there is less chance of running into the same problems multiple times.

The problem is of course that while CSS was designed to be as modular as possible, there are limits, and some things in there are still very specific for the goal it is trying to achieve. E.g., one of the core aspects is to have streaming data, while in the project here this just introduces extra hassle at some aspects. Same thing with the metadata object being used as context, it is very nice and generic, but perhaps might be a bit too much here. Currently I'm considering getting rid of the Operation object completely here and again use a simpler object with just data headers and some other fields. This all to say that I'm also still figuring out what feels good 😅.

BaseTargetExtractor

This class should just not exist. The functionality of the OriginalUrlExtractor from the CSS is very useful to have a separate block for getting the request URL, but unfortunately it also has a small part to check if the input URL falls within the accepted domain. The class here is to work around that, but what I actually want is to split that up in the CSS so that class can be reused.

InteractionRouterHandler

Yea it might be a bit too much, I'm also not convinced yet. Part of it was also reusing the CSS components when possible again, and also some robustness, but it might be too much. The problem was also, similar to the previous one, that the routing classes from CSS were a bit too specific. I feel like this class also shows the issue of using the RDF metadata object for the context.

[termontwouter]

Perfect! I think we're overall very much on the same line then. Good to know that in your mind this is also still WIP, with more or less the same doubts as I expressed. I fully agree on the importance of modularity and reuse, and have always been a proponent of getting more generic stuff out of the CSS in separate packages. Continue the great work! 🚀

[joachimvh]

I have reverted some of the changes I did and redid them, staying closer to the original source, with the idea that this is already sufficient for the intended goal. Otherwise it would lead me too far trying to change too much.

[termontwouter]

Again, mostly agree with the changes since last time I reviewed.

Not sure about the JsonFormHttpHandler

• Request (content) format and response (accept) format are not necessarily related to each other. Instead of nesting the routes in this single handler, I think it would be better if we split it up in a parsing handler and a serializing handler, and place those in a sequence before and after the routes.
• Merging JSON logic and URL-encoded form logic into a single class feels a bit unclean. Better make two classes out of it in a waterfall system, no?

Not sure why [the validation of the grant_type parameter] was commented out.
I might accidentally discover why at some point in the future after much frustration.

Because the server was (initially) not meant to handle any other grant type anyway 🤷

[joachimvh]

Not sure about the JsonFormHttpHandler

It is the behaviour of this class that caused me to revert some changes because I kept trying to make it perfect. 😀

What I wanted, minimally, was that the route classes did not have to worry about data parsing and could assume they get a JSON object as input, and similarly, that they could just return a JSON object.

On the other hand I wanted the outer server layer to just always receive a buffer or similar and not have to convert. The JsonFormHttpHandler is the middleware in between so nothing else has to worry about that.

It is currently hardcoded to JSON and form data, as those are the two relevant formats and they are quite similar, for now at least, with the idea that if it ever needs to do more it can be replaced. I first thought about importing the whole converter block from CSS but that seemed to much for a "simple" API.

It might indeed be a bit nicer to have a middleware class that takes as input a parser and serializer, but to me it didn't really feel necessary to abstract that part right now.

[joachimvh]

Rebased changes onto the pacsoi branch. All commits are the same as before except the first one which fixes an issue in the pacsoi branch.

[termontwouter]

Very minor nits:

• Not sure where CORS handling happens now 🤔
• Not sure what the reasons are for removing certain TSDocs while cleaning up others 🤔

[termontwouter]

Very minor, mostly a question out of interest: why the preference for destructuring, if you need a cast to make it work?

[joachimvh]

I don't actually remember doing this, so it is even possible this happened accidentally with all the merging of branches 😀. But I do prefer the second option as the first one modifies the token object, so perhaps that is why I did it.

[joachimvh]

• Not sure where CORS handling happens now 🤔

The CSS cors handler is used.

• Not sure what the reasons are for removing certain TSDocs while cleaning up others 🤔

Quite random mostly 😅. I removed those where I noticed my IDE complained that there was a mismatch between the parameters of the docs and the function, or the comments were just wrong. But I didn't focus too much on it under the assumption that things were still going to change.

[termontwouter]

The CSS cors handler is used.

And that handler is configured elsewhere in the default configs? Because I only noticed the removal of the handlersjs one from the router config, but not an inclusion of the CSS one elsewhere 🤔

For me, this branch and the config one can be merged in the PACSOI one when you feel they're ready. Before merging PACSOI into main, I'd prefer to have a branch in-between, though, to remove all the stuff from PACSOI that we don't want in main.

[joachimvh]

And that handler is configured elsewhere in the default configs?

It's currently added here:

user-managed-access/packages/uma/config/default.json

Line 53 in 930c674

"@type": "CorsHandler",

Before merging PACSOI into main, I'd prefer to have a branch in-between, though, to remove all the stuff from PACSOI that we don't want in main.

Any reason to prefer this to removing stuff afterwards after discovering it's not needed? Because at this point I can't really say what is relevant and what isn't. Except for removing the screencast and updating the readme (the latter probably just back to the previous version).

I'm going to keep the branches separate until merging into main to still keep somewhat of an overview.

[termontwouter]

Any reason to prefer this to removing stuff afterwards after discovering it's not needed? Because at this point I can't really say what is relevant and what isn't. Except for removing the screencast and updating the readme (the latter probably just back to the previous version).

I'd prefer main not to be filled with stuff that's irrelevant for the main branch, but I also understand your perspective. Maybe remove already what you see immediately, and leave the rest for when we bump into it?