Cleanup Announce Response #563

da2ce7 · 2024-01-01T12:33:42Z

Ready For Review.

codecov · 2024-01-03T13:36:34Z

Codecov Report

Attention: 30 lines in your changes are missing coverage. Please review.

Comparison is base (3dd1402) 80.57% compared to head (0e6fe33) 80.75%.

Files	Patch %	Lines
src/servers/http/v1/responses/announce.rs	89.81%	11 Missing ⚠️
src/core/peer.rs	70.58%	10 Missing ⚠️
src/core/torrent/repository.rs	25.00%	9 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #563      +/-   ##
===========================================
+ Coverage    80.57%   80.75%   +0.18%     
===========================================
  Files          117      117              
  Lines         7854     7873      +19     
===========================================
+ Hits          6328     6358      +30     
+ Misses        1526     1515      -11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

josecelano

Hi @da2ce7, in general I'm OK with this PR because there was a duplication which had to be removed, however, I have the feeling that empty traits and structs should be avoided (if possible).

For me, traits should represent behavior, although I've seen this pattern: Marker Trait.

Same for structs. Empty implementations look like inheritance.

In general, it seems there is a "hidden" enum in this implementation.

If the goal was just to remove duplication I think we could do it without adding empty traits and empty implementations. I've explored that path a little bit here.

Additionally, I think both responses are the same except for the peer list. I think we should take advantage of that property. I think we don't need to mark the parent struct as normal or compact with a different type than the one we use for the peer list. What makes the response compact is the peer list, if I'm not wrong.

The alternative I've proposed It's also aligned with other structs that already exist like AnnouncePolicy and SwarmStats. So we could simplify it even more.

josecelano · 2024-01-08T12:01:27Z

src/servers/http/v1/responses/announce.rs

+/// Trait that defines the Announce Response Format
+pub trait Response: From<AnnounceData> + axum::response::IntoResponse {
+    /// Returns the Body of the Announce Response
+    ///
+    /// # Errors
+    ///
+    /// If unable to generate the response, it will return an error.
+    fn body(&self) -> Result<Vec<u8>, responses::error::Error>;
+}


Hi @da2ce7 I like this trait. A response is something that can bi built from the AnnounceData and converted into an Axum response.

da2ce7 · 2024-01-08T22:26:51Z

@josecelano

I used

pub struct Announce<E, P>
where
    E: Encoding<P>,
    P: PeerEncoding,

An Announce is not typed based upon the PeerEncoding, but a general Encoding.

I would have liked to have the signature something like:

pub struct Announce<E>
where
    E: Encoding,

However, I wanted to have the types being auto-detected based upon supplying form of the Encoding of the peers:
https://github.com/torrust/torrust-tracker/pull/563/files#diff-a73aff059d2186689d35963b70948e4c9e98739953692f7d3d817ecc7e2884c4R481-R494

This is where some of the trait complexity comes from in my implementation.

josecelano · 2024-01-09T09:32:21Z

Hi @da2ce7, there is something I don't like about this PR and mine. When I wrote part of that code I wanted to have a clear representation of what an announce encoded response contains (normal and compact) so the Rust structs have the same structure as the bencoded format.

Notice that we already have an AnnounceData struct which is a generic struct holding the data needed by any announce response format.

My idea was to have DTOs that could be the exact representation of the encoded data. After removing the duplicity it's not clear anymore what the response contains. You have to read the serialization function.

I thought it was a good idea to have a direct mapping between:

Rust struct <-> Bencoded response <-> Json representation of the bencoded format (like in here)

In fact, maybe we have the need to remove duplication because of the comments. If we consider these structures just DTOs the duplication would not be bad, in fact, it could be good, since those structures (even having the same fields) represent different concepts defined in different BEPs. For example, if in the future, a new BEP is introduced and they add a new field to the compact response but not to the normal response we would need to undo a lot of what it's done here.

And regarding my implementation there is still some duplicity, but I think it could be removed making some functions generic. I just wanted to explore a little bit that option. But as I mentioned I've started feeling I'm removing duplication but introducing "domain" entities and making less clear what the encoded response contains.

da2ce7 · 2024-01-10T05:10:47Z

@josecelano

I have reworked the code to try and better take into account you concerns. Now there is are Normal and Compact structs that contain their own prepared data that is ready for serialization.

It is possible to add new forms of serialization, or to remove or add fields without modifying the other structures.

josecelano

Hi @da2ce7 I like this version. I think it's self-explanatory and a good documentation for BEPs. It would be very easy to see what the response is returning and also make changes if needed.

There is only one more thing that I could do. Maybe I would convert the announce.rs into a folder with three files:

mod.rs
normal.rs
compact.rs

That would make even easier to read only one response type, I guess.

josecelano · 2024-01-10T07:34:45Z

src/core/databases/mod.rs

@@ -134,7 +134,7 @@ pub trait Database: Sync + Send {
    /// # Errors
    ///
    /// Will return `Err` if unable to save.
-    async fn save_persistent_torrent(&self, info_hash: &InfoHash, completed: u32) -> Result<(), Error>;
+    async fn save_persistent_torrent(&self, info_hash: &InfoHash, downloaded: u32) -> Result<(), Error>;


Hi @da2ce7 I see you have renamed stats fields.

SwarmMetadata: https://github.com/torrust/torrust-tracker/blob/develop/src/core/torrent/mod.rs#L55-L67

SwarmStats: https://github.com/torrust/torrust-tracker/blob/develop/src/core/torrent/mod.rs#L76-L87

SwarmMetadata contains the names defined in the BRP 48. And SwarmStats the names we were using in the Tracker.

josecelano · 2024-01-10T07:35:28Z

src/core/mod.rs

+    pub stats: SwarmStats,
+    pub policy: AnnouncePolicy,


josecelano · 2024-01-10T07:51:54Z

src/core/torrent/mod.rs

I like removing this duplication but we are using the other names in many places. Maybe we could use methods with the other names instead of the comment.

pub struct SwarmMetadata { /// The number of peers that have ever completed downloading pub downloaded: u32, /// The number of active peers that have completed downloading (seeders) pub complete: u32, /// The number of active peers that have not completed downloading (leechers) pub incomplete: u32, } impl SwarmMetadata { /// Alias for the fn seeders(&self) -> u32 { self.complete } }

But I don't like how similar are complete (seeders) and completed (downloaded).

I don't like this. I would remove the SwarmStats, and rename all the variables to match the SwarmMetadata format... but that can happen another time.

Hi @WarmBeer I'm not sure. I think it's a good thing to follow BEP names but on the other hand "seeders" and "leechers" are also widely adopted, but apparently not in BEPs.

And "downloaded" and "complete" could be confusing, you do not know by the name which one is still active.

When I was thinking about this I decided that maybe the best option would be to use "seeders/leechers" wording in the whole app except for the scrape response.

josecelano · 2024-01-10T07:57:07Z

src/servers/http/v1/responses/announce.rs

Awesome! @da2ce7 I really like this version.

josecelano · 2024-01-10T08:02:26Z

ACK 7bf84f2

(da2ce7): Merge `SwarmStats` into `SwarmMetadata`.

da2ce7 · 2024-01-12T04:15:55Z

@josecelano Ready for Merge

josecelano · 2024-01-12T07:52:37Z

ACK 0e6fe33

da2ce7 temporarily deployed to coverage January 1, 2024 12:33 — with GitHub Actions Inactive

da2ce7 temporarily deployed to coverage January 2, 2024 05:48 — with GitHub Actions Inactive

da2ce7 force-pushed the 20240101_cleanup_response_announce branch from ac3004c to afaee77 Compare January 3, 2024 13:30

da2ce7 temporarily deployed to coverage January 3, 2024 13:30 — with GitHub Actions Inactive

da2ce7 mentioned this pull request Jan 3, 2024

Config Overhaul #557

Closed

5 tasks

da2ce7 force-pushed the 20240101_cleanup_response_announce branch from afaee77 to f667eac Compare January 3, 2024 14:30

da2ce7 temporarily deployed to coverage January 3, 2024 14:30 — with GitHub Actions Inactive

da2ce7 force-pushed the 20240101_cleanup_response_announce branch from f667eac to 3bc13dd Compare January 4, 2024 02:23

da2ce7 temporarily deployed to coverage January 4, 2024 02:23 — with GitHub Actions Inactive

da2ce7 requested a review from josecelano January 4, 2024 02:26

da2ce7 temporarily deployed to coverage January 4, 2024 05:09 — with GitHub Actions Inactive

josecelano mentioned this pull request Jan 8, 2024

Remove duplication in HTTP announce response #585

Closed

josecelano reviewed Jan 8, 2024

View reviewed changes

da2ce7 force-pushed the 20240101_cleanup_response_announce branch from b480b0e to f57e7dd Compare January 9, 2024 00:53

da2ce7 had a problem deploying to coverage January 9, 2024 00:53 — with GitHub Actions Failure

da2ce7 force-pushed the 20240101_cleanup_response_announce branch from f57e7dd to 05e11e3 Compare January 9, 2024 00:59

da2ce7 temporarily deployed to coverage January 9, 2024 00:59 — with GitHub Actions Inactive

da2ce7 force-pushed the 20240101_cleanup_response_announce branch from 05e11e3 to ab77ced Compare January 10, 2024 04:57

da2ce7 temporarily deployed to coverage January 10, 2024 04:57 — with GitHub Actions Inactive

da2ce7 force-pushed the 20240101_cleanup_response_announce branch from ab77ced to 7bf84f2 Compare January 10, 2024 05:06

da2ce7 temporarily deployed to coverage January 10, 2024 05:06 — with GitHub Actions Inactive

josecelano previously approved these changes Jan 10, 2024

View reviewed changes

da2ce7 added the Needs Rebase Base Branch has Incompatibilities label Jan 11, 2024

josecelano added 2 commits January 12, 2024 11:11

refactor: rename non-compact to normal

cca17d5

refactor: extract config struct AnnouncePolicy

d4adfa7

josecelano and others added 2 commits January 12, 2024 12:13

refactor: extract struct SwarmStats

58a57d3

(da2ce7): Merge `SwarmStats` into `SwarmMetadata`.

dev: Announce Responce Cleanup

0e6fe33

da2ce7 dismissed josecelano’s stale review via 0e6fe33 January 12, 2024 04:14

da2ce7 force-pushed the 20240101_cleanup_response_announce branch from 7bf84f2 to 0e6fe33 Compare January 12, 2024 04:14

da2ce7 temporarily deployed to coverage January 12, 2024 04:14 — with GitHub Actions Inactive

da2ce7 removed the Needs Rebase Base Branch has Incompatibilities label Jan 12, 2024

josecelano merged commit 1d9d4f3 into torrust:develop Jan 12, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup Announce Response #563

Cleanup Announce Response #563

da2ce7 commented Jan 1, 2024 •

edited

Loading

codecov bot commented Jan 3, 2024 •

edited

Loading

josecelano left a comment •

edited

Loading

josecelano Jan 8, 2024

da2ce7 commented Jan 8, 2024

josecelano commented Jan 9, 2024

da2ce7 commented Jan 10, 2024

josecelano left a comment

josecelano Jan 10, 2024

josecelano Jan 10, 2024

josecelano Jan 10, 2024

da2ce7 Jan 11, 2024

josecelano Jan 11, 2024

josecelano Jan 10, 2024

josecelano commented Jan 10, 2024

da2ce7 commented Jan 12, 2024

josecelano commented Jan 12, 2024

Cleanup Announce Response #563

Cleanup Announce Response #563

Conversation

da2ce7 commented Jan 1, 2024 • edited Loading

codecov bot commented Jan 3, 2024 • edited Loading

Codecov Report

josecelano left a comment • edited Loading

Choose a reason for hiding this comment

josecelano Jan 8, 2024

Choose a reason for hiding this comment

da2ce7 commented Jan 8, 2024

josecelano commented Jan 9, 2024

da2ce7 commented Jan 10, 2024

josecelano left a comment

Choose a reason for hiding this comment

josecelano Jan 10, 2024

Choose a reason for hiding this comment

josecelano Jan 10, 2024

Choose a reason for hiding this comment

josecelano Jan 10, 2024

Choose a reason for hiding this comment

da2ce7 Jan 11, 2024

Choose a reason for hiding this comment

josecelano Jan 11, 2024

Choose a reason for hiding this comment

josecelano Jan 10, 2024

Choose a reason for hiding this comment

josecelano commented Jan 10, 2024

da2ce7 commented Jan 12, 2024

josecelano commented Jan 12, 2024

da2ce7 commented Jan 1, 2024 •

edited

Loading

codecov bot commented Jan 3, 2024 •

edited

Loading

josecelano left a comment •

edited

Loading