CORS

[Jason2605]

I’m currently using this for a project I’m working on to attempt to “smartify” my house and automatically turn appliances on based on solar generation / weather forecast! Really enjoyed using it so thanks for creating!

Something to think about is CORS and the ability to deal with OPTIONS pre-flight requests. In my version I’ve just bodged the CORS headers to return * but it could be good to think of a way to deal with certain config values almost!

[briandowns]

Would love your feedback on the implementation here. We've got a lot of different ways to do this and I was hoping to get your feedback.

@Controller("/api/v1/books")
class Controller {
    init() {}

    @Options("/all") // have a simple base set of headers and resp code
    @Get("/all") {
        return "get";
    }

    @Cors({"access-control-allow-origin" : "*"})
    @Post
    add() {
        return "add";
    }
}

I'm struggling to get a couple different things working though. Suggestions are very welcome.

[Jason2605]

For a single route I do like the @Cors annotation, but for something a little more "global" I think a middleware which adds the headers is probably pretty useful

class Cors < Middleware {
    init (private allowedOrigin, private allowedHeaders) {}

    handle(request, response) {
        response.headers["Access-Control-Allow-Origin"] = this.allowedOrigin;
        response.headers["Access-Control-Allow-Headers"] = this.allowedHeaders.join(",");
    }
}

What this does mean (and probably for the better?) is we will need to change how responses are built as currently the response object is created after middleware is ran so it can't be modified as part of the middleware. If we create it before pass it to the middleware we can then modify the response a little easier, so dispatch becomes something like this:

private dispatch(request) {
        for (var i = 0; i < this.routes.len(); i += 1) {
            if (this.routes[i].route == request.route and this.routes[i].verb == request.verb) {
                const response = Response();

                const mwCount = this.routes[i].middleware.len();
                if (mwCount > 0) {
                    for (var j = 0; j < mwCount; j += 1) {
                        const middlewareResponse = this.routes[i].middleware[j].handle(request, response);
                        if (middlewareResponse != nil) {
                            return middlewareResponse;
                        }
                    }
                }

                const routeData = this.routes[i].callback(request);

                // if (type(response) == "string") {
                //     response = Response(response);
                // } else if (["list", "number", "nil", "bool", "dict"].contains(type(response))) {
                //     response = JsonResponse(response);
                // } else if (type(response) == "result" and not response.success()) {
                //     response = ErrorResponse();
                // }

                return response.build(routeData);
            }
        }

        return this.notFoundHandler(request);
    }

Response.build would essentially be your data checking if statements that build it to the correct response with the right status code and data type being returned etc. If a response is returned from the middleware we can still exit out the chain early (i.e some sort of Auth check for example)

[Jason2605]

Something I just thought of while looking at dispatch as well (sorry not related to this) could the routes be a dictionary since the paths should always be unique and point to a given method? Would save looping through

[briandowns]

All of that sounds great and makes sense. Creating the response value beforehand would probably allow me to finish the Cache middleware I've been meaning to get done.

As for class vs dict vs class, lemme give that some thought and play with it a bit. I did both and class was better but can't seem to remember why.

Will mess around with this some more this weekend. Thanks!

[Jason2605]

Sorry wasn't much detail on what I meant with the routes, using a class for the actual value is fine, but at the minute all the routes are pushed to a list here, I was curious if it could be

<path>: <RouteClass>

Then in dispatch you don't need the loop:

const route = this.routes.get(request.route);

// Rather than

this.routes[i]

I can make this as a separate issue and take a look into it if you like?

[briandowns]

Sounds good to me. Go for it!