REST enhancement #1: Allow one to get parameters from headers #908

Geod24 · 2014-12-01T20:01:39Z

As the title say, I reopen #867 here.
I accidentally deleted my (remote) branch, and Github won't let me re-open the P.R.

I re-pushed original code to #867 . This one is rebased (because of #871 ).

The only change from #867 is that I renamed @fromHeader to @headerParam.
I can revert that if needed, but I think we have 2 advantages here:

@fromHeader only makes sense in a server context. The (D) interface is meant to describe a REST interface. @headerParam makes sense in a server and client context.
@HeaderParam is what's used in Java Jersey (Example 3.14, scroll down a bit here: https://jersey.java.net/documentation/latest/user-guide.html#d0e1887 ).

Pinging @Dicebot .

… same UDA

- Add a string[string] headers__ local variable to the generated function; - If there's one and only one WebParamAttribute with the name of the parameter we're currently processing (as parameter gets processed in order), we add it to the AA, the key being the 'field' in WebParamAttribute; - headers__ gets later passed to request() which will add all it's field to the headers;

Geod24 · 2014-12-02T17:56:05Z

For the record, my unittests are here:
https://github.com/Geod24/vibe.d/tree/rest-unittests

(under tests/rest/parameter/)

Not complete yet though. And I got a cryptic linker failure when running dub test (DMD 2.066, SEGV with GDC 4.9...).

/usr/include/dlang/dmd/std/array.d:62: error: undefined reference to '_D3std9algorithm463__T12FilterResultS4364vibe3web4rest109__T21registerRestInterfaceTC5tests4rest10parameters6source3app16__unittestL167_2FZ24TestHeaderParamMisconfigZ21registerRestInterfaceFC4vibe4http6router9URLRouterC5tests4rest10parameters6source3app16__unittestL167_2FZ24TestHeaderParamMisconfigC4vibe3web4rest21RestInterfaceSettingsZ8addRouteMFE4vibe4http6common10HTTPMethodAyaDFC4vibe4http6server17HTTPServerRequestC4vibe4http6server18HTTPServerResponseZvAAyaZ9__lambda5TAAyaZ12FilterResult7opSliceMFNaNbNiNfZS3std9algorithm463__T12FilterResultS4364vibe3web4rest109__T21registerRestInterfaceTC5tests4rest10parameters6source3app16__unittestL167_2FZ24TestHeaderParamMisconfigZ21registerRestInterfaceFC4vibe4http6router9URLRouterC5tests4rest10parameters6source3app16__unittestL167_2FZ24TestHeaderParamMisconfigC4vibe3web4rest21RestInterfaceSettingsZ8addRouteMFE4vibe4http6common10HTTPMethodAyaDFC4vibe4http6server17HTTPServerRequestC4vibe4http6server18HTTPServerResponseZvAAyaZ9__lambda5TAAyaZ12FilterResult'
/usr/include/dlang/dmd/std/array.d:62: error: undefined reference to '_D3std9algorithm463__T12FilterResultS4364vibe3web4rest109__T21registerRestInterfaceTC5tests4rest10parameters6source3app16__unittestL167_2FZ24TestHeaderParamMisconfigZ21registerRestInterfaceFC4vibe4http6router9URLRouterC5tests4rest10parameters6source3app16__unittestL167_2FZ24TestHeaderParamMisconfigC4vibe3web4rest21RestInterfaceSettingsZ8addRouteMFE4vibe4http6common10HTTPMethodAyaDFC4vibe4http6server17HTTPServerRequestC4vibe4http6server18HTTPServerResponseZvAAyaZ9__lambda5TAAyaZ12FilterResult5emptyMFNaNbNdNiNfZb'
/usr/include/dlang/dmd/std/array.d:62: error: undefined reference to '_D3std9algorithm463__T12FilterResultS4364vibe3web4rest109__T21registerRestInterfaceTC5tests4rest10parameters6source3app16__unittestL167_2FZ24TestHeaderParamMisconfigZ21registerRestInterfaceFC4vibe4http6router9URLRouterC5tests4rest10parameters6source3app16__unittestL167_2FZ24TestHeaderParamMisconfigC4vibe3web4rest21RestInterfaceSettingsZ8addRouteMFE4vibe4http6common10HTTPMethodAyaDFC4vibe4http6server17HTTPServerRequestC4vibe4http6server18HTTPServerResponseZvAAyaZ9__lambda5TAAyaZ12FilterResult5frontMFNaNbNcNdNiNfZAya'
/usr/include/dlang/dmd/std/array.d:62: error: undefined reference to '_D3std9algorithm463__T12FilterResultS4364vibe3web4rest109__T21registerRestInterfaceTC5tests4rest10parameters6source3app16__unittestL167_2FZ24TestHeaderParamMisconfigZ21registerRestInterfaceFC4vibe4http6router9URLRouterC5tests4rest10parameters6source3app16__unittestL167_2FZ24TestHeaderParamMisconfigC4vibe3web4rest21RestInterfaceSettingsZ8addRouteMFE4vibe4http6common10HTTPMethodAyaDFC4vibe4http6server17HTTPServerRequestC4vibe4http6server18HTTPServerResponseZvAAyaZ9__lambda5TAAyaZ12FilterResult8popFrontMFNaNbNiNfZv'
collect2: error: ld returned 1 exit status

Will check what's going on.

EDIT 437: Sooooooo, Vibe.d doesn't compile with DMD master. Will work on it.

mihails-strasuns · 2014-12-02T18:19:36Z

Re-reading (because I have forgotten everything, doh)

Geod24 · 2014-12-05T07:06:18Z

@Dicebot : Was the code that bad ? ;-)

mihails-strasuns · 2014-12-05T18:44:24Z

No, sorry, I have read it through once to actually remember what we are speaking about and now I need time to actually do line-by-line implementation review. Hopefully this weekend.

mihails-strasuns · 2014-12-06T17:38:11Z

Good. There are plenty of comments but those are mostly matter of style and naming - actual feature looks useful and solid.

Geod24 · 2014-12-06T23:50:27Z

Thanks for the review ! I fixed the style issues.

What's left is:

moving UdaSearchResult out of findNextUda (Geod24@4961a3f#commitcomment-8863012);
Possible improvement to findNextUda by using UDA ArgList slices (Geod24@0ae1803#commitcomment-8863029);
Dead code in vibe.web.rest (Geod24@d0d42c0#commitcomment-8863056).

mihails-strasuns · 2014-12-07T15:52:34Z

LGTM @s-ludwig

btw, @Geod24 I am thinking that after majority of your enhancements are merged we should go for separating/rewriting http://vibed.org/docs#rest-interface-generator into detailed guide. Something that will help answer the question "how am I supposed to use all that stuff?". Willing to join? :)

Geod24 · 2014-12-07T17:19:31Z

Sure ! :-)
Any specific milestone in mind ?

mihails-strasuns · 2014-12-07T17:28:07Z

As soon as your planned enhancements are complete so that next vibe.d release comes with both features and explanation for them.

mihails-strasuns · 2014-12-07T17:28:57Z

I'll probably start writing something based on existing stuff I know and let you eventually take it over

Geod24 · 2014-12-07T17:49:39Z

I have something on my mind about REST paths. I'll try to formalize that and e-mail you next week. That would be one of the 2 breaking change I plan for this module (The other one being the serialization).

s-ludwig · 2014-12-10T07:40:39Z

source/vibe/web/rest.d

@@ -348,7 +350,7 @@ class RestInterfaceClient(I) : I
 		import vibe.data.json : Json;
 		import vibe.textfilter.urlencode;

-		Json request(string verb, string name, Json params, bool[string] param_is_json) const
+		Json request(string verb, string name, Json params, bool[string] param_is_json, string[string] hdrs) const


Minor: I'd use a vibe.inet.message.InetHeaderMap here for efficiency (doesn't allocate for few or no headers).

in ref InetHeaderMap that is of course..

s-ludwig · 2014-12-10T07:44:25Z

Generally looks good AFAICS.

…e rest generator

Geod24 · 2014-12-11T09:00:38Z

@s-ludwig : Thanks for the suggestion, I switched to InetHeaderMap.

s-ludwig · 2014-12-11T16:23:01Z

Okay thanks, merging (finally)!

REST enhancement #1: Allow one to get parameters from headers

Geod24 · 2014-12-11T16:39:20Z

Yay, thanks !

MartinNowak · 2014-12-15T07:06:38Z

This broke compilation on anything but dmd-2.066.1, see https://travis-ci.org/rejectedsoftware/vibe.d/builds/44058739.

Geod24 · 2014-12-15T08:02:15Z

Did it ?

From the error:

source/vibe/internal/meta/uda.d(30): Error: template vibe.internal.meta.uda.findNextUDA matches more than one template declaration:
    source/vibe/internal/meta/uda.d(61):findNextUDA(alias UDA, alias Symbol, long idx, bool allow_types = false)
and
    source/vibe/internal/meta/uda.d(95):findNextUDA(UDA, alias Symbol, long idx, bool allow_types = false)

It looks like it was already broken (but not noticed) before:
https://github.com/rejectedsoftware/vibe.d/blob/2dfacf9863a79ed5a749bdb647b08686ca08b110/source/vibe/internal/meta/uda.d#L28
https://github.com/rejectedsoftware/vibe.d/blob/2dfacf9863a79ed5a749bdb647b08686ca08b110/source/vibe/internal/meta/uda.d#L66

From a quick look, the fix seems trivial, will submit P.R. today. Thanks for your work on this !

MartinNowak · 2014-12-15T16:02:30Z

see #927

Geod24 added 6 commits December 1, 2014 21:00

Adding example for future rest UDA @headerParam

2498170

Generalize findFirstUDA to findNextUDA

0ae1803

Adding funcattr and typetuple to vibe.internal.meta.all

8a9191e

Adding vibe.internal.meta.UDATuple to work with multiple usage of the…

4961a3f

… same UDA

Add attributes for parameter origin

797afca

Server side handling of @headerParam

d0d42c0

Geod24 mentioned this pull request Dec 1, 2014

REST enhancement #1: Allow one to get parameters from headers #867

Closed

Geod24 force-pushed the rest-enh-unsplit-2 branch from 635f272 to ca950a3 Compare December 1, 2014 21:07

Geod24 added 2 commits December 6, 2014 19:36

Fix indentation in vibe.internal.meta.uda

3e8a1bf

Local import for vibe.internal.meta.uda

843c780

Geod24 force-pushed the rest-enh-unsplit-2 branch 3 times, most recently from 42fc57b to f6f41cd Compare December 6, 2014 23:40

Geod24 added 2 commits December 9, 2014 07:05

Style fix

1487210

Small indentation fix for vibe.web.rest

73ed222

Geod24 force-pushed the rest-enh-unsplit-2 branch from f6f41cd to 73ed222 Compare December 9, 2014 06:09

Geod24 mentioned this pull request Dec 9, 2014

Avoid unecessary HTTPMethod -> string -> HTTPMethod conversion #921

Merged

s-ludwig reviewed Dec 10, 2014
View reviewed changes

Small optimization: Use InetHeaderMap instead of string[string] in th…

306e819

…e rest generator

s-ludwig added a commit that referenced this pull request Dec 11, 2014

Merge pull request #908 from Geod24/rest-enh-unsplit-2

643cf77

REST enhancement #1: Allow one to get parameters from headers

s-ludwig merged commit 643cf77 into vibe-d:master Dec 11, 2014

Geod24 deleted the rest-enh-unsplit-2 branch December 11, 2014 17:26

Geod24 mentioned this pull request Dec 14, 2014

REST enhancement #1.1: Refactoring for client side code generator #926

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REST enhancement #1: Allow one to get parameters from headers #908

REST enhancement #1: Allow one to get parameters from headers #908

Geod24 commented Dec 1, 2014

Geod24 commented Dec 2, 2014

mihails-strasuns commented Dec 2, 2014

Geod24 commented Dec 5, 2014

mihails-strasuns commented Dec 5, 2014

mihails-strasuns commented Dec 6, 2014

Geod24 commented Dec 6, 2014

mihails-strasuns commented Dec 7, 2014

Geod24 commented Dec 7, 2014

mihails-strasuns commented Dec 7, 2014

mihails-strasuns commented Dec 7, 2014

Geod24 commented Dec 7, 2014

s-ludwig Dec 10, 2014

s-ludwig Dec 10, 2014

s-ludwig commented Dec 10, 2014

Geod24 commented Dec 11, 2014

s-ludwig commented Dec 11, 2014

Geod24 commented Dec 11, 2014

MartinNowak commented Dec 15, 2014

Geod24 commented Dec 15, 2014

MartinNowak commented Dec 15, 2014

REST enhancement #1: Allow one to get parameters from headers #908

REST enhancement #1: Allow one to get parameters from headers #908

Conversation

Geod24 commented Dec 1, 2014

Geod24 commented Dec 2, 2014

mihails-strasuns commented Dec 2, 2014

Geod24 commented Dec 5, 2014

mihails-strasuns commented Dec 5, 2014

mihails-strasuns commented Dec 6, 2014

Geod24 commented Dec 6, 2014

mihails-strasuns commented Dec 7, 2014

Geod24 commented Dec 7, 2014

mihails-strasuns commented Dec 7, 2014

mihails-strasuns commented Dec 7, 2014

Geod24 commented Dec 7, 2014

s-ludwig Dec 10, 2014

Choose a reason for hiding this comment

s-ludwig Dec 10, 2014

Choose a reason for hiding this comment

s-ludwig commented Dec 10, 2014

Geod24 commented Dec 11, 2014

s-ludwig commented Dec 11, 2014

Geod24 commented Dec 11, 2014

MartinNowak commented Dec 15, 2014

Geod24 commented Dec 15, 2014

MartinNowak commented Dec 15, 2014