Implement HTTP API using generics (fixes #380) #14

nutomic · 2020-01-15T15:50:34Z

nutomic commented

2020-01-15 15:50:34 +00:00

The first commit is the working implementation, we can go with that but I dont like that it needs a bunch of boilerplate in the route handler. I tried to simplify that with higher order functions in the second commit, but that is not compiling yet.

nutomic reviewed 2020-01-15 15:51:14 +00:00

server/.rustfmt.toml Outdated

nutomic commented

2020-01-15 15:51:14 +00:00

Had to add this because the git hook was throwing ``async fn is not permitted in the 2015 edition.

Had to add this because the git hook was throwing ``async fn` is not permitted in the 2015 edition`.

nutomic commented

2020-01-15 16:06:01 +00:00

Once this is implemented, we can also use the http api for performance testing and see which specific api calls are slow.

nutomic reviewed 2020-01-16 14:42:45 +00:00

server/src/routes/api.rs

nutomic commented

2020-01-16 14:42:45 +00:00

I would like to put each route in one line without line breaks, because thats much easier to read. But no idea how to tell that to the formatter.

nutomic reviewed 2020-01-16 16:11:46 +00:00

server/src/routes/api.rs

nutomic commented

2020-01-16 16:11:46 +00:00

There is a lot of room for improvement here. For example, the URLs should not contain any verb, it should be like /api/v1/site with POST executing creation, GET doing a get, etc. Also it should be like /api/v1/community/:name, passing the name in the URL. But we can leave that for later.

There is a lot of room for improvement here. For example, the URLs should not contain any verb, it should be like `/api/v1/site` with POST executing creation, GET doing a get, etc. Also it should be like `/api/v1/community/:name`, passing the name in the URL. But we can leave that for later.

dessalines reviewed 2020-01-16 18:22:12 +00:00

server/src/routes/api.rs

dessalines commented

2020-01-16 18:22:12 +00:00

I agree with the first part. The /api/v1/community/:name way of doing things is for explicitly http only things, but since we have to support websocket as well with these, we might as well have the get body json be identical.

Or at least I can't think of a way to have identical JSON request body's for both.

I agree with the first part. The `/api/v1/community/:name` way of doing things is for explicitly http only things, but since we have to support websocket as well with these, we might as well have the get body json be identical. Or at least I can't think of a way to have identical JSON request body's for both.

dessalines reviewed 2020-01-16 18:24:57 +00:00

server/src/websocket/server.rs

dessalines commented

2020-01-16 18:24:57 +00:00

We should strictly type this. The responses can now look like the requests, with {op: op, data: {...}} at the top level. This is easy to change on the front end.

We should strictly type this. The responses can now look like the requests, with `{op: op, data: {...}}` at the top level. This is easy to change on the front end.

dessalines reviewed 2020-01-16 18:36:46 +00:00

server/src/api/comment.rs

dessalines commented

2020-01-16 18:36:46 +00:00

I looked through and verified that I never use the op only the error on APIError. So this should be okay, although I would rather have it there for debugging purposes.

One possibility, is for all these to just return Err(format_err!("the_i18n_error_messag")); Then on the websocket code, we could have a WebSocketError type (that includes the op), and the http could have an HttpErrorType, (that doesn't include it).

Then we could transform those errors into the form they need to be in, given the call.

I looked through and verified that I never use the `op` only the `error` on APIError. So this should be okay, although I would rather have it there for debugging purposes. One possibility, is for all these to just return `Err(format_err!("the_i18n_error_messag"));` Then on the websocket code, we could have a `WebSocketError` type (that includes the op), and the http could have an `HttpErrorType`, (that doesn't include it). Then we could transform those errors into the form they need to be in, given the call.

nutomic reviewed 2020-01-17 12:34:30 +00:00

server/src/routes/api.rs

nutomic commented

2020-01-17 12:34:30 +00:00

Yep, putting paramaters into the path means we have to duplicate all the param structs (or put all parameters into the path). And either way it would need a lot of boilerplate.

nutomic reviewed 2020-01-17 12:48:14 +00:00

server/src/api/comment.rs

nutomic commented

2020-01-17 12:48:14 +00:00

Well the way it works after my changes is that op is only in the websocket, and the api module doesnt know about that at all. However I believe that server::to_json_string should also add the op to all error messages (untested).

Well the way it works after my changes is that `op` is only in the websocket, and the api module doesnt know about that at all. However I believe that `server::to_json_string` should also add the op to all error messages (untested).

nutomic reviewed 2020-01-17 12:50:02 +00:00

server/src/websocket/server.rs

nutomic commented

2020-01-17 12:50:02 +00:00

Can you explain this more, or give an example? I'm not sure what you want to type more strictly, or how.

dessalines reviewed 2020-01-17 23:58:09 +00:00

server/src/websocket/server.rs

dessalines commented

2020-01-17 23:58:09 +00:00

Right now you're manipulating the json below to insert an op into it. We should have an upper level struct for this like

WebsocketResponse<T> {
  op: String,
  data: T,
}

Right now you're manipulating the json below to insert an op into it. We should have an upper level struct for this like ``` WebsocketResponse<T> { op: String, data: T, } ```

nutomic reviewed 2020-01-18 12:51:11 +00:00

server/src/websocket/server.rs

nutomic commented

2020-01-18 12:51:11 +00:00

Good idea, that makes the code much cleaner. Unfortunately its not working on the frontend, because the data is wrapped one level deeper. So this would need some changes on the frontend, or some way of doing this without the data field.

Good idea, that makes the code much cleaner. Unfortunately its not working on the frontend, because the data is wrapped one level deeper. So this would need some changes on the frontend, or some way of doing this without the `data` field. ![](https://dev.lemmy.ml/pictshare/u79mv0.png) ![](https://dev.lemmy.ml/pictshare/kvzabr.png)

dessalines reviewed 2020-01-18 16:18:46 +00:00

server/src/websocket/server.rs

dessalines commented

2020-01-18 16:18:46 +00:00

That's okay, I can alter the front end. It makes sense anyway since the op is only needed for websocket, and should be at the top level.

dessalines commented

2020-01-19 05:23:17 +00:00

I got the front end part of it done, but I don't think you have Pull requests enabled on your fork so I can't add my commits to your http-api branch.

My changes are here: https://yerbamate.dev/dessalines/lemmy/src/branch/dessalines-http-api

I got the front end part of it done, but I don't think you have Pull requests enabled on your fork so I can't add my commits to your `http-api` branch. My changes are here: https://yerbamate.dev/dessalines/lemmy/src/branch/dessalines-http-api

nutomic commented

2020-01-19 11:31:26 +00:00

It looks to me like PRs are enabled, but maybe things are weird because my repo is a fork of yours. Anyway it sounds confusing to merge your changes into my PR, and then back into your repo. Why dont you just squash and merge my changes locally, then merge yours after that?

nutomic added a new dependency 2020-01-19 13:29:20 +00:00

#16 WIP: Use generics to reduce code duplication in websocket

dessalines commented

2020-01-19 14:00:05 +00:00

It looks to me like PRs are enabled, but maybe things are weird because my repo is a fork of yours.

Ya could be. I think github has an easy way for the original master to add commits to PRs, but I couldn't figure out how to do that here.

Sure thing, but there's some other stuff we gotta add before I merge into master, esp the API docs. And I gotta test some of the endpoints yet.

I'll work off this: https://github.com/dessalines/lemmy/pull/442

> It looks to me like PRs are enabled, but maybe things are weird because my repo is a fork of yours. Ya could be. I think github has an easy way for the original master to add commits to PRs, but I couldn't figure out how to do that here. Sure thing, but there's some other stuff we gotta add before I merge into master, esp the API docs. And I gotta test some of the endpoints yet. I'll work off this: https://github.com/dessalines/lemmy/pull/442

👍 1

dessalines commented

2020-01-19 16:51:48 +00:00

I'm working on the docs for this rn.

dessalines commented

2020-01-19 17:25:09 +00:00

This is wrong: .route("/api/v1/post/replies", web::get().to(route::<GetReplies, GetRepliesResponse>))

It needs to be under /api/v1/user/replies

Also these should probably be PUT, any one with either save or edit:

.route("/api/v1/post/save", web::post().to(route::<SavePost, PostResponse>))

.route("/api/v1/comment/save", web::post().to(route::<SaveComment, CommentResponse>))

.route("/api/v1/user/save_user_settings", web::post().to(route::<SaveUserSettings, LoginResponse>));

This should also be singular, IE PUT /user/mention

.route("/api/v1/user/mentions", web::put().to(route::<EditUserMention, UserMentionResponse>))

Some of these are _, while some are dashes: ban-user. They should all be underscore.
/search should probably be a GET, esp if things like /modlog are.

This is wrong: `.route("/api/v1/post/replies", web::get().to(route::<GetReplies, GetRepliesResponse>))` It needs to be under `/api/v1/user/replies` Also these should probably be `PUT`, any one with either save or edit: `.route("/api/v1/post/save", web::post().to(route::<SavePost, PostResponse>))` `.route("/api/v1/comment/save", web::post().to(route::<SaveComment, CommentResponse>))` `.route("/api/v1/user/save_user_settings", web::post().to(route::<SaveUserSettings, LoginResponse>));` * This should also be singular, IE `PUT /user/mention` `.route("/api/v1/user/mentions", web::put().to(route::<EditUserMention, UserMentionResponse>))` - Some of these are _, while some are dashes: ban-user. They should all be underscore. - `/search` should probably be a GET, esp if things like `/modlog` are.

nutomic commented

2020-01-22 22:52:02 +00:00

Damn we are all over the place. Should I fix this on my branch?