Refactor API: replace Warp with Axum by josecelano · Pull Request #152 · torrust/torrust-tracker

josecelano · 2023-01-02T16:47:10Z

UPDATED: 12/01/2022

The tracker API uses the web framework Warp.
This PR replaces Warp with Axum.

Tasks

Endpoints subtasks

Stats:

GET /api/stats

Torrents:

GET /api/torrents?offset=:u32&limit=:u32
GET /api/torrent/:info_hash

Whitelisted torrents:

POST /api/whitelist/:info_hash
DELETE /api/whitelist/:info_hash

Whitelist commands:

GET /api/whitelist/reload

Keys:

POST /api/key/:seconds_valid
DELETE /api/key/:key

Key commands

GET /api/keys/reload

We are going to reimplement the API with Axum.

- Test scaffolding - Dummy entrypoint

Code for API testing have been reorganized.

da2ce7 · 2023-01-03T06:36:48Z

src/setup.rs

+    if config.http_api.enabled {
+        // Temporarily running the new API in the 1313 port
+        let bind_address = config.http_api.bind_address.clone();
+        let mut bind_socket: SocketAddr = bind_address.parse().unwrap();


prefer expects over unwrap?

I've changed it, but we have a lot of "unwraps". For example like this:

InfoHash::from_str(&hash).unwrap();

I think it would not be very pleasant to write a message for all of them:

InfoHash::from_str(&hash).expect("string should be a valid infohash");

I have to think about it, but I would say it makes sense when the case is unique.

Maybe the problem is we are duplicating code, and we should add a function like this:

fn parse_socket_addr_or_fail(bind_address: &str) -> SocketAddr { bind_address.parse().expect("the bind address string should contain a valid socket address like this 127.0.1.1:8080") }

- Extract domain logic: `Tracker::get_torrents_metrics`. - Move domain logic from web framework controllers to domain services: `get_metrics`. - Remove duplicate code in current Warp API and new Axum API.

It keeps the same contract of the API. It returns 500 status code with error message in "debug" format.

It was added to test Axum configuration.

josecelano · 2023-01-04T14:33:34Z

hi @da2ce7 @WarmBeer It seems Axum does not support SSL directly. I can use the axum-server, which is the one used in this fork.

The new API implementation uses Axum. Axum does not support SSL configuration. The "axum-server" crate provides it.

josecelano · 2023-01-04T17:38:13Z

hi @da2ce7 @WarmBeer It seems Axum does not support SSL directly. I can use the axum-server, which is the one used in this fork.

I did it using axum-server.

josecelano · 2023-01-05T08:01:02Z

I'm working on the GET /api/torrent/:info_hash endpoint. I'm getting an error in the serialization of an u128 field.

#[derive(Serialize, Deserialize, Debug, PartialEq, Eq)]
pub struct Peer {
    pub peer_id: Id,
    pub peer_addr: String,
    #[deprecated(since = "2.0.0", note = "please use `updated_milliseconds_ago` instead")]
    pub updated: u128,
    pub updated_milliseconds_ago: u128,
    pub uploaded: i64,
    pub downloaded: i64,
    pub left: i64,
    pub event: String,
}

I have commented on this issue serde-rs/json#502

I'm going to find out why it's working in the current API (if it's)

…ndpoint Not all cases finished yet. Not found case is pending.

josecelano · 2023-01-05T11:45:14Z

The happy path for the endpoint GET /api/torrent/:info_hash is done. I've realized I do not have a test for the case where the torrent does not exist. I will add the test for the current API and implement it in the new Axum API.

…ET /api/torrent/:info_hash endpoint

…nt. Not found case

josecelano · 2023-01-09T17:10:57Z

I'm working on the GET /api/torrent/:info_hash endpoint. I'm getting an error in the serialization of an u128 field.
#[derive(Serialize, Deserialize, Debug, PartialEq, Eq)]
pub struct Peer {
    pub peer_id: Id,
    pub peer_addr: String,
    #[deprecated(since = "2.0.0", note = "please use `updated_milliseconds_ago` instead")]
    pub updated: u128,
    pub updated_milliseconds_ago: u128,
    pub uploaded: i64,
    pub downloaded: i64,
    pub left: i64,
    pub event: String,
}
I have commented on this issue serde-rs/json#502

I'm going to find out why it's working in the current API (if it's)

It works if I do not use serde directly:

pub async fn get_torrent(State(tracker): State<Arc<Tracker>>, Path(info_hash): Path<String>) -> Response {
    let optional_torrent_info = get_torrent_info(tracker.clone(), &InfoHash::from_str(&info_hash).unwrap()).await;

    match optional_torrent_info {
        Some(info) => Json(Torrent::from(info)).into_response(),
        None => Json(json!("torrent not known")).into_response(),
    }
}

Probably for the same reason it's working on the old Warp implementation.

…invalid

josecelano · 2023-01-12T12:28:13Z

hi, @WarmBeer I think I've found a bug in both the current API implementation and the new one with Axum. It seems the token must be the first query param. Curiously enough, I've reproduced the exact error in the new implementation:

curl -v http://127.0.0.1:1212/api/torrents?limit=1&token=MyAccessToken -> 500 unauthorized
curl -v http://127.0.0.1:1212/api/torrents?token=MyAccessToken&limit=1 -> 200 OK

curl -v http://127.0.0.1:1313/api/torrents?limit=1&token=MyAccessToken -> 500 unauthorized
curl -v http://127.0.0.1:1313/api/torrents?token=MyAccessToken&limit=1 -> 200 OK

I suppose that is not the expected behavior.

josecelano · 2023-01-12T13:34:07Z

hi, @WarmBeer I think I've found a bug in both the current API implementation and the new one with Axum. It seems the token must be the first query param. Curiously enough, I've reproduced the exact error in the new implementation:

curl -v http://127.0.0.1:1212/api/torrents?limit=1&token=MyAccessToken -> 500 unauthorized
curl -v http://127.0.0.1:1212/api/torrents?token=MyAccessToken&limit=1 -> 200 OK

curl -v http://127.0.0.1:1313/api/torrents?limit=1&token=MyAccessToken -> 500 unauthorized
curl -v http://127.0.0.1:1313/api/torrents?token=MyAccessToken&limit=1 -> 200 OK

I suppose that is not the expected behavior.

It seems the problem is only with curl. I've added a new test, and it does not fail.

…rrent twice Previous behavior: When you try to remove a non exisinting whitelisted torrent the response is 500 error. New bahavior: The enpoints checks if the torrent is included in the whitelist. If it does not, then it ignores the reqeust returning a 200 code. It should return a 204 or 404 but the current API only uses these codes: 200, 400, 405, 500. In the new API version we are planning to refctor all endpoints.

…aram

josecelano · 2023-01-13T17:57:14Z

hi @da2ce7 @WarmBeer this is ready to review. I would like to refactor a little bit more, but I think the PR is getting too big. We can merge it as it is and I will create more issues.

I have added some todos and code-reviews in the code.

I also want to make some improvements, like reorganising the API mod structure. Right now, the folder structure is like this:

src/apis/
├── handlers.rs
├── middlewares
│   ├── auth.rs
│   └── mod.rs
├── mod.rs
├── resources
│   ├── auth_key.rs
│   ├── mod.rs
│   ├── peer.rs
│   ├── stats.rs
│   └── torrent.rs
├── responses.rs
├── routes.rs
└── server.rs

It's an implementation-detail or technical-detail organization. I would like to use something more like this:

└── src
    ├── apis
    │   ├── contexts
    │   │   ├── auth_key
    │   │   │   ├── handlers.rs
    │   │   │   ├── mod.rs
    │   │   │   ├── resources.rs
    │   │   │   ├── responses.rs
    │   │   │   └── routes.rs
    │   │   ├── mod.rs
    │   │   ├── stats
    │   │   │   ├── handlers.rs
    │   │   │   ├── mod.rs
    │   │   │   ├── resources.rs
    │   │   │   ├── responses.rs
    │   │   │   └── routes.rs
    │   │   ├── torrent
    │   │   │   ├── handlers.rs
    │   │   │   ├── mod.rs
    │   │   │   ├── resources.rs
    │   │   │   ├── responses.rs
    │   │   │   └── routes.rs
    │   │   └── whitelist
    │   │       ├── handlers.rs
    │   │       ├── mod.rs
    │   │       ├── resources.rs
    │   │       ├── responses.rs
    │   │       └── routes.rs
    │   ├── middlewares
    │   │   ├── auth.rs
    │   │   └── mod.rs
    │   ├── mod.rs
    │   ├── routes.rs
    │   └── server.rs
    └── mod.rs

I like the folder structure to be more domain-oriented, and it's also more scalable. If you agree I can add a new issue for that and we can implement it before implementing the new API version.

josecelano · 2023-01-16T09:35:37Z

ACK 0c3ca87

feat: [torrust#143] add axum dependency

901bc34

We are going to reimplement the API with Axum.

josecelano force-pushed the current-api-with-axum branch 2 times, most recently from 9c8c3e2 to 7471820 Compare January 2, 2023 18:01

josecelano linked an issue Jan 2, 2023 that may be closed by this pull request

API overhaul: reimplement the current API using Axum #143

Closed

1 task

josecelano added 2 commits January 2, 2023 18:13

feat(api): [torrust#143] scaffolding for new API using Axum

cbf8837

- Test scaffolding - Dummy entrypoint

refactor(api): [torrust#143] extract mods for API testing

5ee3f93

Code for API testing have been reorganized.

josecelano force-pushed the current-api-with-axum branch from 2fb0612 to 892f285 Compare January 2, 2023 18:13

feat(api): [torrust#143] axum api, GET /stats endpoint

6a9e2d5

josecelano force-pushed the current-api-with-axum branch from 892f285 to 6a9e2d5 Compare January 2, 2023 18:17

da2ce7 reviewed Jan 3, 2023

View reviewed changes

josecelano added 4 commits January 3, 2023 17:30

refactor: [torrust#143] replace unwrap with expect

7331c82

refactor(api): [torrust#143] remove duplicate code

0615c9f

- Extract domain logic: `Tracker::get_torrents_metrics`. - Move domain logic from web framework controllers to domain services: `get_metrics`. - Remove duplicate code in current Warp API and new Axum API.

refactor: [torrust#143] remove duplicate or unneeded code

0f99f7b

test: [torrust#143] add tests for extracted functions

1c6db6e

josecelano force-pushed the current-api-with-axum branch from de65a97 to 1c6db6e Compare January 3, 2023 17:34

josecelano added 2 commits January 4, 2023 14:12

feat(api): [torrust#143] authentication with GET param for Axum API

43dbed9

It keeps the same contract of the API. It returns 500 status code with error message in "debug" format.

refactor(api): [torrust#143] remove dummy api endpoint

1395945

It was added to test Axum configuration.

josecelano added 2 commits January 4, 2023 17:19

feat(api): [torrust#143] add new cargo dependency: axum-server

af51f77

The new API implementation uses Axum. Axum does not support SSL configuration. The "axum-server" crate provides it.

feat(api): [torrust#143] SSL support for the new Axum API

fe4303c

feat(api): [torrust#143] axum api, WIP. GET /api/torrent/:info_hash e…

16d438d

…ndpoint Not all cases finished yet. Not found case is pending.

josecelano added 3 commits January 9, 2023 16:53

test(api): [torrust#143] add test for torrent not known response in G…

2aebf9a

…ET /api/torrent/:info_hash endpoint

feat(api): [torrust#143] axum api. GET /api/torrent/:info_hash endpoi…

a649fe8

…nt. Not found case

fix: clippy errors

ded4d11

refactor(api): [torrust#143] use extracted service in the Warp handler

a806179

josecelano requested a review from mickvandijke January 9, 2023 17:12

fix(api): [torrust#143] fix new Axum API enpoint when URL params are …

517ffde

…invalid

josecelano added 7 commits January 12, 2023 13:37

test(api): [torrust#143] add tests for authenticaation

2da0719

refactor(api): [torrust#143] normalize test names for errrors

3bcbbc9

test(api): [torrust#143] add test for invalid infohash URL path param

39c15c6

test(api): [torrust#143] add test for invalid key duration URL path p…

2c222ee

…aram

test(api): [torrust#143] add more tests for invalid key id URL path p…

072f3d7

…aram

refactor(api): [torrust#143] extract and rename functions

8d32628

josecelano force-pushed the current-api-with-axum branch from 436bc96 to 8d32628 Compare January 13, 2023 16:28

josecelano added 6 commits January 13, 2023 16:36

feat(api): [torrust#143] replace Warp API with Axum implementation

337e12e

refactor(api): [torrust#143] remove Warp API implementation

77ec521

refactor(api): [torrust#143] move API resources mod

6dd3c48

refactor(api): extract mod for API responses

2a92b0a

refactor(api): [torrust#143] extract api mods

0940463

docs(api): [torrust#143] move comment

6ddbdd9

josecelano marked this pull request as ready for review January 13, 2023 17:49

josecelano requested a review from da2ce7 January 13, 2023 17:49

josecelano added 5 commits January 13, 2023 18:10

docs(api): [torrust#143] remove comment

6955666

refactor(api): [torrust#143] rename response functions

02dfe3e

refactor(api): [torrust#143] change fn return type

b7c5144

docs(api): [torrust#143] remove deprecated comment

1c72ac0

refactor(api): [torrust#143] remove duplicate code

0c3ca87

josecelano merged commit c45f862 into torrust:develop Jan 16, 2023

This was referenced Jan 16, 2023

API overhaul: refactor API folder structure #157

Closed

Fix opaque type warnings #148

Closed

josecelano deleted the current-api-with-axum branch January 16, 2023 12:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor API: replace Warp with Axum#152

Refactor API: replace Warp with Axum#152
josecelano merged 48 commits intotorrust:developfrom
josecelano:current-api-with-axum

josecelano commented Jan 2, 2023 •

edited

Loading

Uh oh!

da2ce7 Jan 3, 2023

Uh oh!

josecelano Jan 3, 2023

Uh oh!

josecelano commented Jan 4, 2023 •

edited

Loading

Uh oh!

josecelano commented Jan 4, 2023

Uh oh!

josecelano commented Jan 5, 2023

Uh oh!

josecelano commented Jan 5, 2023

Uh oh!

josecelano commented Jan 9, 2023

Uh oh!

josecelano commented Jan 12, 2023

Uh oh!

josecelano commented Jan 12, 2023 •

edited

Loading

Uh oh!

josecelano commented Jan 13, 2023 •

edited

Loading

Uh oh!

josecelano commented Jan 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

josecelano commented Jan 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tasks

Endpoints subtasks

Uh oh!

da2ce7 Jan 3, 2023

Choose a reason for hiding this comment

Uh oh!

josecelano Jan 3, 2023

Choose a reason for hiding this comment

Uh oh!

josecelano commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josecelano commented Jan 4, 2023

Uh oh!

josecelano commented Jan 5, 2023

Uh oh!

josecelano commented Jan 5, 2023

Uh oh!

josecelano commented Jan 9, 2023

Uh oh!

josecelano commented Jan 12, 2023

Uh oh!

josecelano commented Jan 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josecelano commented Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josecelano commented Jan 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

josecelano commented Jan 2, 2023 •

edited

Loading

josecelano commented Jan 4, 2023 •

edited

Loading

josecelano commented Jan 12, 2023 •

edited

Loading

josecelano commented Jan 13, 2023 •

edited

Loading