Reorganize api endpoints (fixes #2022) #5216

Nutomic · 2024-11-20T13:43:43Z

These changes are mainly to discuss what the new API v4 routes should look like. Once we come to an agreement, I will adjust it so that the existing API v3 remains usable with the same paths.

Nutomic · 2024-11-21T11:10:50Z

src/api_routes_http.rs

+          )
+          .route("/change_password", web::put().to(change_password))
+          .route("/totp/generate", web::post().to(generate_totp_secret))
+          .route("/totp/update", web::post().to(update_totp)),


I tried to organize these so that they share a single scope with the same rate limit.

dessalines

At first I was confused about the account vs person/user , but it makes sense to me now. account is things I do to my account, while person/user should be things anyone can do to it (currently only reading).

Suggestions below.

Additional context: #4428

src/api_routes_http.rs

dessalines · 2024-11-21T12:59:03Z

src/api_routes_http.rs

          .wrap(rate_limit.import_user_settings())
+          .route(web::get().to(export_settings))
          .route(web::post().to(import_settings)),


I don't like that import is just a POST to account/export. Should probably be separate account/export and account/import endpoints to be clearer.

It needs to have a common path so that the rate limit can apply to both. I changed it to GET /account/settings/export and POST /account/settings/import now.

You could just override the rate limit for those specifically. export / import are actions, not settings, but I don't feel too strongly about that either way.

Then it requires two separate blocks like this which is too verbose:

.service( web::resource("/user/export_settings") .wrap(rate_limit.export_user_settings()) .route(web::get().to(export_settings)), ) .service( web::resource("/user/import_settings") .wrap(rate_limit.import_user_settings()) .route(web::post().to(import_settings)), )

That's pretty minor imo. They could be put under a account/takeout or something tho, especially since we'll eventually need to add a full data export too (even though that won't be importable anywhere).

src/api_routes_http.rs

dessalines · 2024-11-21T13:01:13Z

src/api_routes_http.rs

          .route(
-            "/mark_all_as_read",
+            "/mention/mark_all_as_read",
            web::post().to(mark_all_notifications_read),
          )
          .route("/save_user_settings", web::put().to(save_user_settings))


Maybe just save ? I dunno.

Made it /account/settings/save

This one too, burying it under settings doesn't seem right, when all the other actions aren't under settings either.

Its specifically about settings so it makes sense (same place as import/export). And buried is the wrong word, its still easy to find in docs etc.

Nutomic · 2024-11-21T13:48:22Z

src/api_routes_http.rs

@@ -161,33 +161,25 @@ use lemmy_utils::rate_limit::RateLimitCell;

 pub fn config(cfg: &mut web::ServiceConfig, rate_limit: &RateLimitCell) {
  cfg.service(
-    web::scope("/api/v3")
+    web::scope("/api/v4")
+      .wrap(rate_limit.message())


Moved the message rate limit here so it doesnt have to be applied separately to many routes. This means a route like POST /api/v3/comment will have both message and post rate limits applied, but that shouldnt be a problem.

(moved here so that rate limit doesnt apply to image or federation endpoints)

Nutomic · 2024-11-22T08:57:34Z

Also moved SiteView.my_user to a separate endpoint (/api/v3/account/my_user) and removed SiteView.taglines/custom_emojis which were already deprecated.

dessalines · 2024-11-22T14:02:58Z

crates/api_crud/src/user/my_user.rs

I like this.

SleeplessOne1917 · 2024-11-22T16:09:22Z

Beyond the bit of reorganizing done here, I think it would be good to handle fetching individual resources by including the id in the route instead of the POST body, e.g. fetching a single comment would be /comments/<id> instead of /comment with an id in the body, and fetching multiple comments would just be GET /comments.

Besides that, I believe it could be worth looking into making some datatypes specific for returning as responses. We already have a few types like that, but many of them return views (e.g. CommentView, PostView, etc.) that have a lot of data that's not needed in the context of the part of a client that's making an API call, making the JSON response bigger than it needs to be.

There's probably more to be said, but these two points are the biggest on my mind so far.

dessalines · 2024-11-22T17:59:50Z

All the params for GET requests are currently like:

/comment?id={}
/comment/list?type={}&sort={}

It'd probably require a bunch of strange manipulations to parse the required params into a distinct part of the route like /ID/ rather than just doing them uniformly like we're currently doing.

Besides that, I believe it could be worth looking into making some datatypes specific for returning as responses. We already have a few types like that, but many of them return views (e.g. CommentView, PostView, etc.) that have a lot of data that's not needed in the context of the part of a client that's making an API call, making the JSON response bigger than it needs to be.

I'd be perfectly good with creating slimmer versions of CommentView and PostView, that are used in different places, as long as they are predictably and strongly-typed with different names, and we're sure that these slimmer versions aren't going to be missing anything that clients need. Feel free to open an issue for that, as well as the redundant / unecessary columns in certain places.

SleeplessOne1917 · 2024-11-22T18:29:15Z

It'd probably require a bunch of strange manipulations to parse the required params into a distinct part of the route like /ID/ rather than just doing them uniformly like we're currently doing.

actix extractors make that pretty straightforward: there is an extractor for that that works basically the same as the query and body extractors Lemmy already uses.

Besides that, I'll gladly pour some time into tweaking the API design. It's something I've been interested in for awhile. Sometime in the next month I was thinking of opening a PR similar to this with some annotations as to why I made the choices I did.

Nutomic · 2024-11-26T12:06:39Z

Im not sure its worth changing the way request parameters are passed, because that would require a lot of work for client devs to make things compatible, for little or no benefit. With this PR all thats needed is changing a few paths while parameters are the same. Also there are cases like GetPost where the same item can be fetched in different ways.

But in general, this PR is only one part of the changes for API v4. If there are other changes you want to include, make a PR so we can discuss it (best change only a small part as example first, and then make full changes later once we agree that it makes sense).

Nutomic · 2024-11-26T14:30:59Z

Updated with routes for api v3 and api v4. For api v3 I added the my_user field back to /api/v3/site for backwards compat.

On the other hand I removed endpoints for unreleased features from api v3, so that client devs have an incentive to upgrade. This includes the new endpoints for taglines and emojis which were removed from /api/v3/site, so it may be necessary to add that back in.

Edit: Strangely the api tests fail with dependency 0.20.0-api-v4.0, but pass fine with file:../../lemmy-js-client/. Both have the exact same code. Any idea what might cause this?

dessalines · 2024-11-27T14:58:46Z

It must be failing at register user for some reason. The file... is never reliable in my experience, I've only ever gotten it to work with yalc when testing, not pnpm link

Also I'm seeing a few v3 references still:

rg v3 crates/ api_tests/

Its probably fine to keep the v3 API routes, as long as we say that we will not support them in the future.

src/api_routes_v4.rs

dessalines

We need some feedback from other ppl on these too.

crates/api_common/src/site.rs

dessalines · 2024-11-28T23:06:42Z

crates/api_common/src/site.rs

@@ -448,14 +448,10 @@ pub struct GetSiteResponse {
  pub site_view: SiteView,
  pub admins: Vec<PersonView>,
  pub version: String,
-  #[cfg_attr(feature = "full", ts(optional))]
+  #[cfg_attr(feature = "full", ts(skip))]
  pub my_user: Option<MyUserInfo>,


This should just be removed now right? Or just add deprecated like the other fields, and always have it be None in the code.

This is used for get_site_v3 below.

dessalines · 2024-11-28T23:11:19Z

crates/api_crud/src/site/read.rs

 use std::sync::LazyLock;

 #[tracing::instrument(skip(context))]
-pub async fn get_site(
+pub async fn get_site_v3(


These backward-compatibility hacks (and serving up api/v3 completely) should really be handled in client libraries IMO, not piling them on top of each other in the lemmy codebase.

Not only is it going to get increasingly complicated, but we're going to be on the hook to support both of these alongside each other, with all the object changes. That might be okay for a bigger dev team, but not with our current dev resources.

IMO since this is a breaking changes release, we should just increment the api version, but don't support the old one.

If we remove the v3 api directly in 0.20, it means that all clients and frontends will be completely broken until the devs rewrite their code for v4, which could take months. Even just dropping GetSiteResponse.my_user would probably break all authentication functionality, or even lead to crashes.

The backwards compatibility is really easy to implement in this case, and it prevents a lot of problems for users. Lemmy has a huge userbase now which relies on a certain level of stability, so even in major releases we cannot break everything just to save a little bit of maintenance work.

I think dropping v3 support should wait for whenever we release Lemmy 1.0.0. A breaking change of that size corresponds well with what's communicated in the version number: a major version update that breaks backwards compatibility.

dessalines · 2024-11-28T23:14:22Z

src/api_routes_v4.rs

+        scope("/account/settings")
+          .wrap(rate_limit.import_user_settings())
+          .route("/export", get().to(export_settings))
+          .route("/import", post().to(import_settings)),


I like the idea of using a heading name like takeout for this. Eventually we'll need to add #4540 here anyway.

We dont need to implement that in the backend, and then that would be under a different endpoint.

IMO that one does need to be in the back-end, to avoid ppl having to page through their comments.

dessalines · 2024-11-28T23:15:31Z

src/api_routes_v4.rs

+            "/mention/mark_all_as_read",
+            post().to(mark_all_notifications_read),
+          )
+          .route("/settings/save", put().to(save_user_settings))


I still think this should just be /save, but all of this needs feedback from others also.

/account/save? Its not saving the account but the settings.

Nutomic · 2024-11-29T21:43:24Z

Tests are all passing now. I also moved the different user block endpoints to similar routes:

/account/block/user
/account/block/community
/account/block/instance

Also changed /admin/block_instance to /admin/instance/block and /admin/allow_instance to /admin/instance/allow.

dessalines · 2024-11-29T22:35:07Z

There are major API changes here, and since this needsto be fairly future proof, we need feedback from @phiresky @SleeplessOne1917 @dullbananas

SleeplessOne1917 · 2024-11-30T00:33:02Z

There are major API changes here, and since this needsto be fairly future proof, we need feedback from @phiresky @SleeplessOne1917 @dullbananas

The general way things are grouped make sense. As far as how the endpoints are laid out, I don't think it needs to be blocked, but I didn't comb through the rest of the code much yet. I'll defer to the others you tagged for if this needs to be blocked.

I plan on making a draft PR with some more breaking changes some time mid December once the semester for my masters degree is over.

dullbananas · 2024-11-29T23:56:04Z

src/api_routes_v4.rs

+          .route("/delete", post().to(delete_post))
+          .route("/remove", post().to(remove_post))
+          .route("/mark_as_read", post().to(mark_post_as_read))
+          .route("/mark_many_as_read", post().to(mark_posts_as_read))


Do something like this. If we later want to create a function that creates both bulk and non-bulk routes under the given path ("/mark_as_read" in this case), this will allow it to be done more cleanly.

Suggested change

.route("/mark_many_as_read", post().to(mark_posts_as_read))

.route("/mark_as_read/many", post().to(mark_posts_as_read))

Done, also changed /account/mention/mark_all_as_read to /account/mention/mark_as_read/all

dullbananas · 2024-11-30T00:14:28Z

src/api_routes_v4.rs

+          .route("/delete", post().to(delete_post))
+          .route("/remove", post().to(remove_post))


Is there any idea for better distinguishing what's currently called "delete" and "remove"? Changing it later would be less convenient.

Could do something like /user_delete and /mod_remove but it doesnt sound good.

I think delete (for content you created) vs remove (for content others created) is probably fine.

I thought about whether we should move all of the mod related actions to a mod subheading, but its probably better to keep them at the item level like they are now (/comment/remove), or else we'd just have to replicate the same things but under a different heading (/mod/comment/remove)

dullbananas · 2024-11-30T00:19:41Z

src/api_routes_v4.rs

+          .route("/report", post().to(create_pm_report))
+          .route("/report/resolve", put().to(resolve_pm_report))
+          .route("/report/list", get().to(list_pm_reports)),
+      )


Consider moving thinks like distinguish and lock to the update enpoints, and moving things like save and like to new "actions" update endpoints.

You mean /comment/update/distinguish? Not sure that makes sense, because edits use /comment directly and there is no /comment/update. And /comment/action/ sounds too generic, it could cover anything.

dullbananas · 2024-11-30T00:23:27Z

src/api_routes_v4.rs

+      )
+      .service(
+        scope("/account")
+          .route("/my_user", get().to(get_my_user))


This would be less confusing

Suggested change

.route("/my_user", get().to(get_my_user))

.route("", get().to(get_my_user))

dullbananas · 2024-11-30T00:27:27Z

src/api_routes_v4.rs

+            "/registration_application",
+            get().to(get_registration_application),
+          )
+          .route("/list_all_media", get().to(list_all_media))


Both "/account/list_media" and "/admin/list_all_media" should be moved to something like "/media/list"

Then how to distinguish between admin and user endpoints? They need to be separate because admins may also want to view their personal uploads.

Either one works for me, but I think I like the /admin/list_all_media a little better than /media/list_all

Nutomic · 2024-12-03T09:48:05Z

Sounds like we can merge it then. For additional changes we can make more pull requests.

dessalines · 2024-12-03T18:32:53Z

I'm good with that. There's no reason to keep this a long standing PR, and we can open up things to fix specific issues in the future.

Nutomic · 2024-12-04T09:48:16Z

@dullbananas Still needs your approval

Reorganize api endpoints (fixes #2022)

9072bb3

Nutomic force-pushed the reorganize-api-endpoints branch from df193dc to 9072bb3 Compare November 20, 2024 13:47

scopes

48e817b

Nutomic commented Nov 21, 2024

View reviewed changes

move message rate limit

12ef9a0

dessalines reviewed Nov 21, 2024

View reviewed changes

move rate limit

b59e19d

Nutomic commented Nov 21, 2024

View reviewed changes

Nutomic added 3 commits November 21, 2024 14:53

apply suggestions

0c58088

move my_user to separate endpoint

88b0aba

remove taglines and custom emojis from siteview

b365d7f

dessalines reviewed Nov 22, 2024

View reviewed changes

crates/api_crud/src/user/my_user.rs Outdated

Copy link

Member

dessalines Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this.

Nutomic added 2 commits November 26, 2024 13:08

Merge branch 'main' into reorganize-api-endpoints

0889d38

routes for api v3 and v4

a6fd213

Nutomic mentioned this pull request Nov 26, 2024

Changes for api v4 LemmyNet/lemmy-js-client#410

Draft

remove new features from api v3

b7fdc41

Nutomic marked this pull request as ready for review November 26, 2024 14:48

Nutomic requested review from phiresky, dullbananas and SleeplessOne1917 as code owners November 26, 2024 14:48

js client

d2a39cc

dessalines reviewed Nov 27, 2024

View reviewed changes

src/api_routes_v4.rs Outdated Show resolved Hide resolved

Merge branch 'main' into reorganize-api-endpoints

c2ccff0

Nutomic added 4 commits November 28, 2024 12:12

replace getSite().my_user with getMyUser()

0583505

update

6eaa569

update config

a7942e5

remove web::

8ac9e2b

Nutomic force-pushed the reorganize-api-endpoints branch from 139a5e7 to 8ac9e2b Compare November 28, 2024 14:58

Nutomic added 3 commits November 28, 2024 16:07

update

026d583

prettier

e907f11

lockfile

3cec5e9

dessalines reviewed Nov 28, 2024

View reviewed changes

v

d14f8dc

Nutomic force-pushed the reorganize-api-endpoints branch from e6d32a2 to d14f8dc Compare November 29, 2024 10:12

Nutomic added 6 commits November 29, 2024 11:27

Merge branch 'main' into reorganize-api-endpoints

4257092

fix settings

f6037af

move block endpoints

0fa23d7

more changes

9c6ecbf

fmt

e620e94

update

7b423a0

dullbananas requested changes Nov 30, 2024

View reviewed changes

Nutomic added 2 commits December 2, 2024 11:34

change some routes

52f1728

lockfile

4b26446

dessalines approved these changes Dec 3, 2024

View reviewed changes

Nutomic added 2 commits December 4, 2024 10:29

Add comment about deprecation

c495df3

Merge branch 'main' into reorganize-api-endpoints

cc9d52f

Nutomic enabled auto-merge (squash) December 4, 2024 09:48

	.route("/mark_many_as_read", post().to(mark_posts_as_read))
	.route("/mark_as_read/many", post().to(mark_posts_as_read))

		.route("/delete", post().to(delete_post))
		.route("/remove", post().to(remove_post))

	.route("/my_user", get().to(get_my_user))
	.route("", get().to(get_my_user))

Reorganize api endpoints (fixes #2022) #5216

Are you sure you want to change the base?

Reorganize api endpoints (fixes #2022) #5216

Conversation

Nutomic commented Nov 20, 2024

Choose a reason for hiding this comment

dessalines left a comment • edited Loading

Choose a reason for hiding this comment

dessalines Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nutomic commented Nov 22, 2024

Choose a reason for hiding this comment

SleeplessOne1917 commented Nov 22, 2024

dessalines commented Nov 22, 2024

SleeplessOne1917 commented Nov 22, 2024

Nutomic commented Nov 26, 2024

Nutomic commented Nov 26, 2024 • edited Loading

dessalines commented Nov 27, 2024 • edited Loading

dessalines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dessalines Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nutomic Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nutomic commented Nov 29, 2024

dessalines commented Nov 29, 2024

SleeplessOne1917 commented Nov 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nutomic Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nutomic commented Dec 3, 2024

dessalines commented Dec 3, 2024

Nutomic commented Dec 4, 2024

dessalines left a comment •

edited

Loading

dessalines Nov 21, 2024 •

edited

Loading

Nutomic commented Nov 26, 2024 •

edited

Loading

dessalines commented Nov 27, 2024 •

edited

Loading

dessalines Nov 28, 2024 •

edited

Loading

Nutomic Nov 29, 2024 •

edited

Loading

Nutomic Dec 2, 2024 •

edited

Loading