Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Overhaul core Tracker: review whitelist functionality (part 2) #1270

Closed
6 tasks done
Tracked by #1181
josecelano opened this issue Feb 14, 2025 · 0 comments · Fixed by #1274
Closed
6 tasks done
Tracked by #1181

Overhaul core Tracker: review whitelist functionality (part 2) #1270

josecelano opened this issue Feb 14, 2025 · 0 comments · Fixed by #1274
Assignees
@josecelano
Labels
- Developer - Torrust Improvement Experience Code Cleanup / Refactoring Tidying and Making Neat

Comments

@josecelano
Copy link
Member

josecelano commented Feb 14, 2025 •
edited
Loading

Parent issue: #1261
Relates to:

This is second part of the implementation of this issue.

I decided to merge the PR for the issue #1268 before continuing working of the refactor.

Sub-tasks

  • In the tracker-core announce handler return a Result<AnnounceData, AnnounceError> when the torrent is not included in the whitelist.
  • In the tracker-core scrape handler return a Result<ScrapeData, ScrapeError> so we are able to return errors in the future without breaking the public API.
  • Move packages::udp_tracker_core::peer_builder::from_request to new package udp-protocol
  • Inline invoke function in handlers when possible.
  • Move tests for the handlers (announce and scrape) from HTTP server to http_tracker_core. *1
  • Move tests for the handlers (announce and scrape) from UDP server to udp_tracker_core. *1

NOTE*1: instead of moving the tests I will create new ones for the http_tracker_core after refactoring all packages. The tests contain logic from the delivery layer that it will not be moved to the http_tracker_core so It will be harder to refactor those tests than created new one for the logic that we will finally move to the http_tracker_core.
@josecelano josecelano added - Developer - Torrust Improvement Experience Code Cleanup / Refactoring Tidying and Making Neat labels Feb 14, 2025
@josecelano josecelano self-assigned this Feb 14, 2025
This was referenced Feb 14, 2025
Merged
Closed
josecelano added a commit that referenced this issue Feb 14, 2025
@josecelano
Merge #1269: Overhaul core Tracker: review whitelist functionality
ec07b78 
eca5c59 refactor: [#1268] move scrape logic from udp server to udp_tracker_core package (Jose Celano)
c0fc390 refactor: [#1268] move announce logic from udp server to udp_tracker_core package (Jose Celano)
37a142e refactor: [#1268] move scrape logic from axum to http_tracker_core package (Jose Celano)
74815ab refactor: [#1268] move announce logic from axum to http_tracker_core package (Jose Celano)
e48aaf5 [#1268] move udp services to udp_tracker_core package (Jose Celano)
73753e3 [#1268] move http services to http_tracker_core package (Jose Celano)
dec742e refactor: [#1268] extract servers::udp::services::scrape service (Jose Celano)
3c07b26 refactor: [#1268] extract servers::udp::services::announce service (Jose Celano)
81825c9 refactor: [#1268] separate UDP handlers into diferent modules (Jose Celano)

Pull request description:

  Overhaul core Tracker: review whitelist functionality.

  ### Sub-tasks

  - [x] Introduce submodules for handlers in UDP: `servers::udp::handlers::{announce, scrape}`.
  - [x] Create the missing services (app layer) in the UDP tracker. There is no intermediary level between handlers and the core tracker. It will moved to its own package `udp-tracker-core` later.
  - [x] Move the service `services::announce::invoke()` in the HTTP tracker to the `http-tracker-core` package.
  - [x] Move the service `services::announce::invoke()` in the UDP tracker to the `udp-tracker-core` package.
  - [x] Move logic from the handler (in the framework level - delivery layer) to the application service in the `http-tracker-core` package.
    - [x] For the `announce` request
    - [x] For the `scrape` request
  - [x] Move logic from the handler (controller level - delivery layer) to the application service in the `udp-tracker-core` package.
    - [x] For the `announce` request
    - [x] For the `scrape` request
  - [ ] ~~Add version module also for the UDP tracker. I don't see any reason to use `v1` in the http tracker but not in the UDP tracker.~~ I will leave this until we introduce a new major version.

  ### Sub-tasks for a new PR

  I've left these tasks for a new [issue](#1270). This PR is just moving things and the new tasks imply changing function signatures.

  - [ ] In the tracker-core announce handler return a `Result<AnnounceData, AnnounceError>` when the torrent is not included in the whitelist.
  - [ ] In the tracker-core scrape handler return a `Result<ScrapeData, ScrapeError>` so we are able to return errors in the future without breaking the public API.

ACKs for top commit:
  josecelano:
    ACK eca5c59

Tree-SHA512: d3ee37ffa806e8a86813fe564e3840fab7bfc44d2072f85bc2eba84ac3402c95c0f6a5beb2725071cb0498415f55915431b656e98c71b3a6bf469de961c37f03
josecelano added a commit to josecelano/torrust-tracker that referenced this issue Feb 17, 2025
@josecelano
refactor: [torrust#1270] return error in tracker core announce handler
e40777f 
when the tracker is running in `listed` mode and the client is trying to
announce a torrents whose infohash is not whitelisted.
@josecelano josecelano linked a pull request Feb 17, 2025 that will close this issue
Overhaul core Tracker: review whitelist functionality (part 2) #1274
Merged
4 tasks
josecelano added a commit to josecelano/torrust-tracker that referenced this issue Feb 17, 2025
@josecelano
refactor: [torrust#1270] return errorin core announce and scrape handler
da1353b 
- In the announce handler, it returns an error when the tracker is
  running in `listed` mode and the infohash is not whitelisted. This was
done only in the delivery layers but not in the domain.
- In the scrape handler, it does not return any errors for now, but It
  will allow us in the future to return errors whithout making breaking
changes.
josecelano added a commit to josecelano/torrust-tracker that referenced this issue Feb 17, 2025
@josecelano
refactor: [torrust#1270] extract fn to new package udp-protocol
dbee7ad
josecelano added a commit to josecelano/torrust-tracker that referenced this issue Feb 17, 2025
@josecelano
refactor: [torrust#1270] inline http tracker announce invoke fn
096d503
josecelano added a commit to josecelano/torrust-tracker that referenced this issue Feb 17, 2025
@josecelano
refactor: [torrust#1270] inline http tracker scrape invoke fn
6651343
josecelano added a commit to josecelano/torrust-tracker that referenced this issue Feb 17, 2025
@josecelano
refactor: [torrust#1270] inline udp tracker announce invoke fn
af28429
josecelano added a commit to josecelano/torrust-tracker that referenced this issue Feb 17, 2025
@josecelano
refactor: [torrust#1270] inline udp tracker scrape invoke fn
d49aebd
@josecelano josecelano mentioned this issue Feb 17, 2025
Closed
josecelano added a commit that referenced this issue Feb 17, 2025
@josecelano
Merge #1274: Overhaul core Tracker: review whitelist functionality (p…
467d50a 
…art 2)

d0e6936 refactor: simplify loop with map (Jose Celano)
d49aebd refactor: [#1270] inline udp tracker scrape invoke fn (Jose Celano)
af28429 refactor: [#1270] inline udp tracker announce invoke fn (Jose Celano)
6651343  refactor: [#1270] inline http tracker scrape invoke fn (Jose Celano)
096d503 refactor: [#1270] inline http tracker announce invoke fn (Jose Celano)
dbee7ad refactor: [#1270] extract fn to new package udp-protocol (Jose Celano)
da1353b refactor: [#1270] return errorin core announce and scrape handler (Jose Celano)

Pull request description:

  Overhaul core Tracker: review whitelist functionality (part 2).

  - [x] Return an error in the announce handler if the tracker is running in `listed` mode and the infohash is not whitelisted. This is already implemented in the higher delivery layer but it should be also validated in the tracker core (domain layer).
  - [x] Change scrape handler signature to return an error even if it's not needed now. That will allow us to return an error in the future whiteout breaking changes.
  - [x] Move `packages::udp_tracker_core::peer_builder::from_request` to new package `udp-protocol`
  - [x] Inline `invoke` function in handlers (`http-tracker-core` and `udp-tracker-core`) when possible.

ACKs for top commit:
  josecelano:
    ACK d0e6936

Tree-SHA512: b4e9356291b35b1cc6a20a5b28a6c0b9136baafbb9bc71638bd83932709970f3ee8bee91a81cc09a775244fd3e774131d238cf4f52920bcc141fed4161b86b03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@josecelano josecelano

Labels
- Developer - Torrust Improvement Experience Code Cleanup / Refactoring Tidying and Making Neat
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Overhaul core Tracker: review whitelist functionality (part 2) josecelano/torrust-tracker
1 participant
@josecelano