Adding Cors to RestInterface #1299
Conversation
|
need to replace the |
|
hmm, apparently chunkBy and the way I use tuple isn't valid on 2.066.. |
|
Seems like |
|
And |
|
Didn't know about the groupBy, that should make it easier. Will do after lunch. |
| import std.conv : text; | ||
| import std.array : array; | ||
| import vibe.http.common : httpMethodString, httpMethodFromString; | ||
| // NOTE: don't know what is better, to keep this in memory, or generate on each request |
There was a problem hiding this comment.
Should be good as it is now. In case of such small allocations, I'd always opt to reduce the per-request GC pressure (even if there remains a lot to do for the REST module in that regard).
There was a problem hiding this comment.
Yeah that makes sense. In principle I could get the array of methods at compile time, but that entailed more work.
About the GC, I suppose it would be possible to do some static analysis and see which lines of code would allocate. That would be cool. Although it depends entirely on which compiler is used.
There was a problem hiding this comment.
Yeah, that was actually my first thought, too, doing it at compile time. But for a first version, this is totally sufficient.
The long term goal would be to add some @nogc unit tests for cases where there should be no allocations necessary. But there is still a lot to do/annotate before that will work. I made some attempts, but it was mostly impossible due to so many Phobos functions that either allocated, or weren't annotated @nogc.
|
Hmm... groupBy isn't available on 2.066.1 |
|
Huh... I would have never expected this, I thought that that had been included ages ago, but obviously I mixed it up with |
| foreach (i, func; intf.RouteFunctions) { | ||
| auto route = intf.routes[i]; | ||
|
|
||
| // normal handler | ||
| auto handler = jsonMethodHandler!(func, i)(instance, intf); | ||
| auto handler = jsonMethodHandler!(func, i)(instance, intf, settings); |
There was a problem hiding this comment.
There is intf.settings, so settings doesn't need to be passed here as an additional parameter. This also has the advantage of always being !is null, so that all null checks can be removed.
|
Apart from the |
|
In the tests/restclient I depended on directly referencing/changing the settings, with intf.settings that is not possible since it is copied. Will have to change that. |
|
Fixed |
getRoutesGroupedByPattern is used by CORS because it needs a list of methods available per resource. Also add unittest.
Had to modify jsonMethodHandler to take in RestInterfaceSettings since it needs to check the allowedOrigins to determine to handle Cors or not
|
Okay, thanks! Looks good to merge now. It'll not make it into the upcoming release, but I can tag an early alpha release later to make it available for testing. |
Add CORS support to RestInterface. Fixes #546.
This pull request adds cors support for rest.
getRoutesGroupedByPattern()to RestInterface structgetRoutesGroupedByPattern()jsonMethodHandlersignature to takeRestInterfaceSettingsjsonMethodHandlerto handle cors requestregisterRestInterfaceto handle preflight cors requests