Handler for Nationella geodataplattformen for documents on detailed plans

[johnnyblasta]

Config the handler is described in CONFIG.md

Setting up a layer can be done like this (change to your foreign key):

"attributes": [
	{ "title": "<br>Plandokument", "linktext": "planTexts2", "url": "planLinks2", "splitter": ";" }
            ],
            "attachments": {
               "formTitle": "Attachments",
               "url": "http://localhost:3001/origoserver/attachments/",
               "foreignKey": "detaljplan.objektidentitet",
               "groups": [
               {
                  "name": "planer",
                  "linkAttribute": "planLinks2",
                  "fileNameAttribute": "planTexts2"
               }
               ]
            }

Closes #177

[steff-o]

• There should be a retry mechanism if the token has been revoked or otherwise is no longer valid. I.e. all calls using the token should check return code for 403 and retry the call once. If everyone is nice to each other this would not be necessary, but if the consumer_key is used in several places the api will invalidate all other tokes when a new created. Since the token is cached it becomes invalid before it expiration time has expired if someone else is using the same consumer_key.

• Comments are in Swedish!

[steff-o]

Actually tested this time...

• The entire origo server halts when an error is thrown. If this happens because a plan is missing it would be really annoying. Probably due to some miss configuration of routes as it will not reach the default error handler as expected.
• I'm not too fond of the recursive implementation of the retry using a global variable. It makes the code hard to read and in theory if there is an ongoing call to create a new token, another call can assume that it has already tried once and will fail without retrying. It requires some really unfortunate timing and the result is not crucial, but still.

Other than that is works as expected, but the route should probably not be called attachments as this is a special purpose endpoint. In the future someone may actually implement a general purpose attachments server in origo-server. There is no requirement in the origo attachment implementation that requires that the endpoint is called attachments.

[steff-o]

Well, you walked right in to a minefield with the error handling. It seems to work as it doesn't crash, but it is not going through the error handler route as expected. Maybe that is not necessary, but when there is a catch all error route configured in app.js one might expect that to be used. As it is now it falls back to the express internal last handler which turns all unhandled exceptions to HTTP 500, which pretty much is the same as origo-server's error handler so observed behavior is the same.

There are two main reasons why the error does not end up in the error handler when just throwing an error:

1. The handler functions are async and express does not await.
2. The error router is not attached correctly.

The async part could be solved by using the express-async-handler npm package, It is a wrapper for async express handlers that will take care of awaiting and make sure exceptions are unwrapped from async context.

The error handler part is probably because the error handler is created as an express app instead of a router. If set up correctly, throwing an exception in a handler function would end up in routes/error.js second function.

Also it would be nice if the NGP-stuff was given its own router in the same way as auth router in order to keep routes/index.js cleaner.

All of the above is not necessary for functionality, but I think it would greatly increase maintainability.

Still I'm not thrilled about the retry mechanism. Seems like the only difference is that it only prevents calls to different endpoints will work correctly. Multiple calls to e.g. listAll will still use the same shared variable. Also it makes the code even more hard to read. Why not just retry the fetch statement without making a recursive call? Something like:

let response = await fetch(url, { method: 'POST', body: JSON.stringify(postdata), headers: myHeaders });

    if (!response.ok) {
        // Retry once in case the token as been invalid f.e. when same consumer key been used to create token on another server
        if (response.status === 401) {
            await createToken(next);
            response = await fetch(url, { method: 'POST', body: JSON.stringify(postdata), headers: myHeaders });
        }
    }
    if (!response.ok) {
        next(new Error(`Error: ${response.status}`));
    }

But I'd be surprised if it actually happened in real life and if it does, worst case is that a user has to retry fetching.

No sure whether to approve or not. It kind of works, but I have doubts about maintainability.

[johnnyblasta]

Made suggested changes to improve maintainability and readability. Won't do anything more about the error handling.
Appreciate the comments @steff-o I don't need a approval from you to merge, which I will do in a couple of days unless someone streniously objects.