New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding Cors to RestInterface #1299

Merged
merged 8 commits into from Nov 3, 2015

Conversation

Projects
None yet
2 participants
@skoppe
Contributor

skoppe commented Oct 20, 2015

This pull request adds cors support for rest.

  • getRoutesGroupedByPattern() to RestInterface struct
  • add unittest for getRoutesGroupedByPattern()
  • extends jsonMethodHandler signature to take RestInterfaceSettings
  • add code to jsonMethodHandler to handle cors request
  • add code to generate OPTIONS routes in registerRestInterface to handle preflight cors requests
  • add test code in tests/restclient
@skoppe

This comment has been minimized.

Show comment
Hide comment
@skoppe

skoppe Oct 20, 2015

Contributor

need to replace the asLowerCase into dmd 2.066 equivalent

Contributor

skoppe commented Oct 20, 2015

need to replace the asLowerCase into dmd 2.066 equivalent

@skoppe

This comment has been minimized.

Show comment
Hide comment
@skoppe

skoppe Oct 20, 2015

Contributor

hmm, apparently chunkBy and the way I use tuple isn't valid on 2.066..

Contributor

skoppe commented Oct 20, 2015

hmm, apparently chunkBy and the way I use tuple isn't valid on 2.066..

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Oct 20, 2015

Member

Seems like groupBy got renamed to chunkBy, but at the same time SortedRange.groupBy was added: https://issues.dlang.org/show_bug.cgi?id=14183

Member

s-ludwig commented Oct 20, 2015

Seems like groupBy got renamed to chunkBy, but at the same time SortedRange.groupBy was added: https://issues.dlang.org/show_bug.cgi?id=14183

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Oct 20, 2015

Member

And tuple didn't yet have support for named fields at all... always fun to be forced to support a range of compiler versions.

Member

s-ludwig commented Oct 20, 2015

And tuple didn't yet have support for named fields at all... always fun to be forced to support a range of compiler versions.

@skoppe

This comment has been minimized.

Show comment
Hide comment
@skoppe

skoppe Oct 20, 2015

Contributor

Didn't know about the groupBy, that should make it easier. Will do after lunch.

Contributor

skoppe commented Oct 20, 2015

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

This comment has been minimized.

@s-ludwig

s-ludwig Oct 20, 2015

Member

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).

@s-ludwig

s-ludwig Oct 20, 2015

Member

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).

This comment has been minimized.

@skoppe

skoppe Oct 21, 2015

Contributor

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.

@skoppe

skoppe Oct 21, 2015

Contributor

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.

This comment has been minimized.

@s-ludwig

s-ludwig Oct 21, 2015

Member

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.

@s-ludwig

s-ludwig Oct 21, 2015

Member

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.

// so we always assume it does
res.headers["Access-Control-Allow-Credentials"] = "true";
res.headers["Access-Control-Max-Age"] = "1728000";
res.headers["Access-Control-Allow-Methods"] = *method;

This comment has been minimized.

@s-ludwig

s-ludwig Oct 20, 2015

Member

Looks like a good argument for starting to design a built-in generic authentication mechanism. Being able to remove @before at some point is another important argument, because that would also enable getting a full list of headers.

@s-ludwig

s-ludwig Oct 20, 2015

Member

Looks like a good argument for starting to design a built-in generic authentication mechanism. Being able to remove @before at some point is another important argument, because that would also enable getting a full list of headers.

This comment has been minimized.

@skoppe

skoppe Oct 20, 2015

Contributor

Its funny, elsewhere I was complaining that you don't get access to the raw request/response headers. Now it is obvious you don't want that. In fact, any dealings with low level http details should be know at compile time (at least with the rest interface)

Having said that, its going to take a long time before anybody complains about allow-credentials being non-configurable. Maybe max-age...

@skoppe

skoppe Oct 20, 2015

Contributor

Its funny, elsewhere I was complaining that you don't get access to the raw request/response headers. Now it is obvious you don't want that. In fact, any dealings with low level http details should be know at compile time (at least with the rest interface)

Having said that, its going to take a long time before anybody complains about allow-credentials being non-configurable. Maybe max-age...

@skoppe

This comment has been minimized.

Show comment
Hide comment
@skoppe

skoppe Oct 22, 2015

Contributor

Hmm... groupBy isn't available on 2.066.1

Contributor

skoppe commented Oct 22, 2015

Hmm... groupBy isn't available on 2.066.1

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Oct 22, 2015

Member

Huh... I would have never expected this, I thought that that had been included ages ago, but obviously I mixed it up with group(). In that case it looks like some classical loops are needed, there seems to be no useful replacement in 2.066.

Member

s-ludwig commented Oct 22, 2015

Huh... I would have never expected this, I thought that that had been included ages ago, but obviously I mixed it up with group(). In that case it looks like some classical loops are needed, there seems to be no useful replacement in 2.066.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Oct 23, 2015

Member

Apart from the settings parameter, looks good to merge now.

Member

s-ludwig commented Oct 23, 2015

Apart from the settings parameter, looks good to merge now.

@skoppe

This comment has been minimized.

Show comment
Hide comment
@skoppe

skoppe Oct 24, 2015

Contributor

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.

Contributor

skoppe commented Oct 24, 2015

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.

@skoppe

This comment has been minimized.

Show comment
Hide comment
@skoppe

skoppe Nov 3, 2015

Contributor

Hmm, I might have done something wrong with rebasing...

I wanted to notch travis to do a recompile (I read somewhere that the libasync was working again), and also figured an update wouldn't hurt. Now all these other commits show up. Oops... Well, at least the CI passes.

What is the normal strategy here?

Should I cleanup this stuff by creating new branch?

Fixed

Contributor

skoppe commented Nov 3, 2015

Hmm, I might have done something wrong with rebasing...

I wanted to notch travis to do a recompile (I read somewhere that the libasync was working again), and also figured an update wouldn't hurt. Now all these other commits show up. Oops... Well, at least the CI passes.

What is the normal strategy here?

Should I cleanup this stuff by creating new branch?

Fixed

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Nov 3, 2015

Member

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.

Member

s-ludwig commented Nov 3, 2015

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.

s-ludwig added a commit that referenced this pull request Nov 3, 2015

Merge pull request #1299 from skoppe/restcors
Add CORS support to RestInterface. Fixes #546.

@s-ludwig s-ludwig merged commit efdca6e into vibe-d:master Nov 3, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment