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

REST collections #1268

Merged
merged 8 commits into from Nov 10, 2015

Conversation

Projects
None yet
2 participants
@s-ludwig
Member

s-ludwig commented Sep 17, 2015

This PR adds syntactic support for item collections in REST interfaces. It does so in a backwards compatible manner w.r.t interface definition (i.e. existing interfaces stay the same except for defining an additional item key type/name).

Open issues:

  • @path annotations currently have to be used manually to get nice URLs (see test project)
  • ... don't remember, but there was something else

The current ItemID TypeTuple is going to be replaced by a struct that at the same time defines the types and names of the key parameters. The interface generator will then validate all methods to have the right parameters and will automatically generate a proper method path (":key/method").

This PR depends on #1266

Show outdated Hide outdated source/vibe/web/rest.d
@Geod24

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Sep 27, 2015

Contributor

Sure looks interesting from a quick glance (and is definitely needed).
I'll do a full review once #1266 is in.

Contributor

Geod24 commented Sep 27, 2015

Sure looks interesting from a quick glance (and is definitely needed).
I'll do a full review once #1266 is in.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 27, 2015

Member

I don't know why I didn't have this idea earlier, but I got stuck with an approach where there would be an auto-generated class that sits between the original API interface and the implementation. This didn't really work out well due to performance and clarity issues. This approach with a Collection type on the other hand is really quite straight forward and is a much easier transition target for existing code.

Member

s-ludwig commented Sep 27, 2015

I don't know why I didn't have this idea earlier, but I got stuck with an approach where there would be an auto-generated class that sits between the original API interface and the implementation. This didn't really work out well due to performance and clarity issues. This approach with a Collection type on the other hand is really quite straight forward and is a much easier transition target for existing code.

@s-ludwig s-ludwig changed the title from [WIP] REST collections to REST collections Oct 10, 2015

exitEventLoop(true);
}
int main()

This comment has been minimized.

@Geod24

Geod24 Oct 14, 2015

Contributor

Why the custom main ?

@Geod24

Geod24 Oct 14, 2015

Contributor

Why the custom main ?

This comment has been minimized.

@s-ludwig

s-ludwig Oct 15, 2015

Member

Not really important. It was added to the "restclient" test long ago and I used that as a template.

@s-ludwig

s-ludwig Oct 15, 2015

Member

Not really important. It was added to the "restclient" test long ago and I used that as a template.

@Geod24

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Oct 14, 2015

Contributor

I'm concerned by the lack of example / documentation.
Collections are important, if not central, to REST apis, and this change leave it almost undocumented.

Contributor

Geod24 commented Oct 14, 2015

I'm concerned by the lack of example / documentation.
Collections are important, if not central, to REST apis, and this change leave it almost undocumented.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Oct 15, 2015

Member

I've added some more thorough documentation for Collection!I. There should probably also be an example in the examples/ folder, but the current "rest" example seems already a bit too crowded. Maybe I'll just take the one from "tests/rest-collections" and adjust it a little.

Member

s-ludwig commented Oct 15, 2015

I've added some more thorough documentation for Collection!I. There should probably also be an example in the examples/ folder, but the current "rest" example seems already a bit too crowded. Maybe I'll just take the one from "tests/rest-collections" and adjust it a little.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Oct 15, 2015

Member

I think I remember the issue that I mentioned in the PR description. Property functions currently don't map well to this, because they have one or more parameters when they really should have none:

interface Item {
    struct CollectionIndices { int _id; }
    @property string name(int _id); // OOPS: this is now actually a setter property...
}

So this currently either means that you'd write string getName(int _id);, or that you'd do @method(HTTPMethod.GET) string name(int _id).

A possible way to solve this would be to automatically convert getXXX(...) in the interface to @property xxx() in Collection!I. Or we could introduce another UDA à la @collectionProperty string name(...); to make this conversion explicit. Any other ideas?

Member

s-ludwig commented Oct 15, 2015

I think I remember the issue that I mentioned in the PR description. Property functions currently don't map well to this, because they have one or more parameters when they really should have none:

interface Item {
    struct CollectionIndices { int _id; }
    @property string name(int _id); // OOPS: this is now actually a setter property...
}

So this currently either means that you'd write string getName(int _id);, or that you'd do @method(HTTPMethod.GET) string name(int _id).

A possible way to solve this would be to automatically convert getXXX(...) in the interface to @property xxx() in Collection!I. Or we could introduce another UDA à la @collectionProperty string name(...); to make this conversion explicit. Any other ideas?

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Oct 15, 2015

Member

Another point would be if we should have a shortcut syntax for collections that contain plain values:

interface Values {
    struct CollectionIndices { int index; }
    float getValue(int index);
}

auto val = values[0].get(); // current syntax
auto val = values[0]; // possible shortcut syntax

// same could be done for set() and opIndexAssign()
Member

s-ludwig commented Oct 15, 2015

Another point would be if we should have a shortcut syntax for collections that contain plain values:

interface Values {
    struct CollectionIndices { int index; }
    float getValue(int index);
}

auto val = values[0].get(); // current syntax
auto val = values[0]; // possible shortcut syntax

// same could be done for set() and opIndexAssign()
@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Oct 17, 2015

Member

Example is there now + made the index parameter check more rigid + rebased.

Since both, the @property issue and possible shortcut syntaxes, can be implemented in a backwards-compatible manner, I'd exclude those from this PR, if there are no objections.

Member

s-ludwig commented Oct 17, 2015

Example is there now + made the index parameter check more rigid + rebased.

Since both, the @property issue and possible shortcut syntaxes, can be implemented in a backwards-compatible manner, I'd exclude those from this PR, if there are no objections.

@Geod24

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Oct 29, 2015

Contributor

As we discussed, I tried it with a FS abstraction. It's still a WIP but I already have a bit of feedback.

I obviously forgot CollectionIndices on the first run, and got the following error:

../vibe.d/source/vibe/web/rest.d(532,35): Error: no property 'CollectionIndices' for type 'app.IFSItem'
../vibe.d/source/vibe/web/rest.d(534,22): Error: no property 'CollectionIndices' for type 'app.IFSItem'
../vibe.d/source/vibe/web/rest.d(540,2): Error: can only slice tuple types, not _error_
../vibe.d/source/vibe/web/rest.d(541,2): Error: can only slice tuple types, not _error_
../vibe.d/source/vibe/web/rest.d(624,6): Error: static assert  __error
source/app.d(41,12):        instantiated from here: Collection!(IFSItem)

Oups.

It seems that there is no support for listing objects in the collection ? Also, how do you plan to support collections that holds different types of object. z.b. an "animals" collection which hold a turtle and a dolphin ? In regular D you could just cast, but here you'll get null every time.

Contributor

Geod24 commented Oct 29, 2015

As we discussed, I tried it with a FS abstraction. It's still a WIP but I already have a bit of feedback.

I obviously forgot CollectionIndices on the first run, and got the following error:

../vibe.d/source/vibe/web/rest.d(532,35): Error: no property 'CollectionIndices' for type 'app.IFSItem'
../vibe.d/source/vibe/web/rest.d(534,22): Error: no property 'CollectionIndices' for type 'app.IFSItem'
../vibe.d/source/vibe/web/rest.d(540,2): Error: can only slice tuple types, not _error_
../vibe.d/source/vibe/web/rest.d(541,2): Error: can only slice tuple types, not _error_
../vibe.d/source/vibe/web/rest.d(624,6): Error: static assert  __error
source/app.d(41,12):        instantiated from here: Collection!(IFSItem)

Oups.

It seems that there is no support for listing objects in the collection ? Also, how do you plan to support collections that holds different types of object. z.b. an "animals" collection which hold a turtle and a dolphin ? In regular D you could just cast, but here you'll get null every time.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Oct 29, 2015

Member

It seems that there is no support for listing objects in the collection ?

You can define a get method that takes all but the last index as parameters. The result will be available as GET /animals/ or similar. Maybe we could have an automated alternative, but that sounds like it could get hairy in terms of required flexibility/complexity vs. benefits.

Also, how do you plan to support collections that holds different types of object. z.b. an "animals" collection which hold a turtle and a dolphin ? In regular D you could just cast, but here you'll get null every time.

That would have to be done using a "super"-interface that combines all methods and reacts with a 404 if a method is called on something that doesn't match the derived type. Do you have an idea how to improve this? We could define some kind of automated helper to at least make the interface definition part easier (interface SuperInterface(Interface, DerivedInterfaces...), but I'm not sure if there is a reasonable way to go further, as that would require leaking the internal representation of the collection AFAICS.

Member

s-ludwig commented Oct 29, 2015

It seems that there is no support for listing objects in the collection ?

You can define a get method that takes all but the last index as parameters. The result will be available as GET /animals/ or similar. Maybe we could have an automated alternative, but that sounds like it could get hairy in terms of required flexibility/complexity vs. benefits.

Also, how do you plan to support collections that holds different types of object. z.b. an "animals" collection which hold a turtle and a dolphin ? In regular D you could just cast, but here you'll get null every time.

That would have to be done using a "super"-interface that combines all methods and reacts with a 404 if a method is called on something that doesn't match the derived type. Do you have an idea how to improve this? We could define some kind of automated helper to at least make the interface definition part easier (interface SuperInterface(Interface, DerivedInterfaces...), but I'm not sure if there is a reasonable way to go further, as that would require leaking the internal representation of the collection AFAICS.

@Geod24

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Oct 31, 2015

Contributor

You can define a get method that takes all but the last index as parameters. The result will be available as GET /animals/ or similar. Maybe we could have an automated alternative, but that sounds like it could get hairy in terms of required flexibility/complexity vs. benefits.

Could you add it to the examples ? Being able to list ressources in a collection (and to customize the listing) is mandatory for collections (else it isn't delf-describing).

That would have to be done using a "super"-interface that combines all methods and reacts with a 404 if a method is called on something that doesn't match the derived type.

That could be a good thing. We could also couple that with opCast maybe ?

Contributor

Geod24 commented Oct 31, 2015

You can define a get method that takes all but the last index as parameters. The result will be available as GET /animals/ or similar. Maybe we could have an automated alternative, but that sounds like it could get hairy in terms of required flexibility/complexity vs. benefits.

Could you add it to the examples ? Being able to list ressources in a collection (and to customize the listing) is mandatory for collections (else it isn't delf-describing).

That would have to be done using a "super"-interface that combines all methods and reacts with a 404 if a method is called on something that doesn't match the derived type.

That could be a good thing. We could also couple that with opCast maybe ?

s-ludwig added some commits Sep 16, 2015

Fix REST client to process URL placeholders in all parts of the path.
The RestInterfaceSettings.baseURL part of the full URL got ignored before. This affected interface getters with a path annotation that contained placeholders.

This commit also adjusts the collections example to manually annotate the collection getters properly. In the future, these annotations should be the default.
Use structs to define collection ID types and compute proper default …
…paths.

The collection ID is now part of the path by default, if the id name has an underscore prefixed.

TODO: validate that all interface methods use the right names for ID parameters.
@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Nov 3, 2015

Member

Added an example project (including the definition of a get() member).

Member

s-ludwig commented Nov 3, 2015

Added an example project (including the definition of a get() member).

@Geod24

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Nov 4, 2015

Contributor

What seems very weird to me in it's usage is that GET /threads/ returns a list of indices, and not a list of object.

Example here: https://api.github.com/users/s-ludwig/repos

In this example, it is even worst because you cannot do https://api.github.com/users/s-ludwig/repos/:id. You're supposed to list repository this way and then use links from the object to access it's property (I even think this serves a "short representation" of the object, but I'm not 100% sure).

So to list issues in a repository you would access https://api.github.com/repos/s-ludwig/FrameworkBenchmarks/issues .

Would that be representable with the current P.R. ?

Contributor

Geod24 commented Nov 4, 2015

What seems very weird to me in it's usage is that GET /threads/ returns a list of indices, and not a list of object.

Example here: https://api.github.com/users/s-ludwig/repos

In this example, it is even worst because you cannot do https://api.github.com/users/s-ludwig/repos/:id. You're supposed to list repository this way and then use links from the object to access it's property (I even think this serves a "short representation" of the object, but I'm not 100% sure).

So to list issues in a repository you would access https://api.github.com/repos/s-ludwig/FrameworkBenchmarks/issues .

Would that be representable with the current P.R. ?

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Nov 4, 2015

Member

You can return anything you want from the get() method. Returning IDs is just the possibility that seemed to make most sense in the example scenario. Returning URLs would have been another alternative, but that would just have wasted a lot of bandwidth. Similarly, returning the full thread objects could mean a huge amount of data in a real-world scenario.

Another common approach is to return a list of partial resources (e.g. thread name, date, poster and contents of the first post).

Member

s-ludwig commented Nov 4, 2015

You can return anything you want from the get() method. Returning IDs is just the possibility that seemed to make most sense in the example scenario. Returning URLs would have been another alternative, but that would just have wasted a lot of bandwidth. Similarly, returning the full thread objects could mean a huge amount of data in a real-world scenario.

Another common approach is to return a list of partial resources (e.g. thread name, date, poster and contents of the first post).

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Nov 10, 2015

Member

Regarding recursive collections, I've thought about them and support can definitely be added on top later. However, it requires a more elaborate implementation, because the router is of no help here (can't define routes with recursive patterns). But for the specific case of a file system structure it would suffice to simply make the "*"-part of a route accessible as a parameter (and adjust the client generator accordingly). I think we should definitely do that, regardless of collections.

Regarding this PR, I'd want to merge this now. We have a full release period ahead to make any required changes, or even to remove it if necessary. But having it in master gives it a lot more exposure for some actual real-world testing.

Member

s-ludwig commented Nov 10, 2015

Regarding recursive collections, I've thought about them and support can definitely be added on top later. However, it requires a more elaborate implementation, because the router is of no help here (can't define routes with recursive patterns). But for the specific case of a file system structure it would suffice to simply make the "*"-part of a route accessible as a parameter (and adjust the client generator accordingly). I think we should definitely do that, regardless of collections.

Regarding this PR, I'd want to merge this now. We have a full release period ahead to make any required changes, or even to remove it if necessary. But having it in master gives it a lot more exposure for some actual real-world testing.

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

@s-ludwig s-ludwig merged commit 4818bfc into master Nov 10, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@s-ludwig s-ludwig deleted the rest-collections branch Nov 10, 2015

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Nov 10, 2015

Member

That would have to be done using a "super"-interface that combines all methods and reacts with a 404 if a method is called on something that doesn't match the derived type.

That could be a good thing. We could also couple that with opCast maybe ?

The problem with opCast is that it would require sending type information over the wire, either directly when requesting a resource, or using a special HTTP request that happens within opCast. This sounds like something that requires more thought to avoid

Member

s-ludwig commented Nov 10, 2015

That would have to be done using a "super"-interface that combines all methods and reacts with a 404 if a method is called on something that doesn't match the derived type.

That could be a good thing. We could also couple that with opCast maybe ?

The problem with opCast is that it would require sending type information over the wire, either directly when requesting a resource, or using a special HTTP request that happens within opCast. This sounds like something that requires more thought to avoid

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment