Skip to content

Conversation

@josecelano
Copy link
Member

@josecelano josecelano commented May 9, 2024

Relates to: #790

Refactor: enrich field types in configuration structs.

This will cause the app to fail earlier while loading invalid configurations and simplify the code by reducing conversions to get the rich type from the primitive when it's used.

  • HealthCheckApi
  • UdpTracker
  • HttpTracker
  • HttpApi
  • Configuration

Output example when you provide an invalid socket address:

$ cargo run
    Finished `dev` profile [optimized + debuginfo] target(s) in 0.10s
     Running `target/debug/torrust-tracker`
Loading default configuration file: `./share/default/config/tracker.development.sqlite3.toml` ...
thread 'main' panicked at src/bootstrap/config.rs:45:32:
called `Result::unwrap()` on an `Err` value: ConfigError { source: LocatedError { source: Error { tag: Tag(Default, 2), profile: Some(Profile(Uncased { string: "default" })), metadata: Some(Metadata { name: "TOML source string", source: None, provide_location: Some(Location { file: "packages/configuration/src/v1/mod.rs", line: 397, col: 14 }), interpolater:  }), path: ["health_check_api", "bind_address"], kind: Message("invalid socket address syntax"), prev: None }, location: Location { file: "packages/configuration/src/v1/mod.rs", line: 400, col: 41 } } }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

You get that error proving this config option:

[health_check_api]
bind_address = "127.0.0.1-1313"

The error contains all the information needed, although it could be more user-friendly. Maybe we can map that error to a simpler explanation like this:

Configuration error: invalid socket address provided in toml file PATH in section `health_check_api` option `bind_address`.

@josecelano josecelano requested a review from da2ce7 May 9, 2024 16:22
@josecelano josecelano added the Code Cleanup / Refactoring Tidying and Making Neat label May 9, 2024
@codecov
Copy link

codecov bot commented May 9, 2024

Codecov Report

Attention: Patch coverage is 89.90826% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 78.75%. Comparing base (a20c9d7) to head (b545b33).

Files Patch % Lines
src/bootstrap/logging.rs 44.44% 5 Missing ⚠️
src/app.rs 0.00% 2 Missing ⚠️
packages/test-helpers/src/configuration.rs 93.33% 1 Missing ⚠️
src/bootstrap/jobs/health_check_api.rs 0.00% 1 Missing ⚠️
src/bootstrap/jobs/torrent_cleanup.rs 0.00% 1 Missing ⚠️
src/bootstrap/jobs/udp_tracker.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #854      +/-   ##
===========================================
- Coverage    78.76%   78.75%   -0.01%     
===========================================
  Files          168      169       +1     
  Lines         9291     9293       +2     
===========================================
+ Hits          7318     7319       +1     
- Misses        1973     1974       +1     

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

@josecelano josecelano force-pushed the 852-config-overhaul-replace-primitive-types-in-configuration-with-reacher-types branch from 6cafec5 to 7b3330a Compare May 10, 2024 12:54
@josecelano josecelano force-pushed the 852-config-overhaul-replace-primitive-types-in-configuration-with-reacher-types branch from 7b3330a to 7002f8a Compare May 10, 2024 13:02
If the next major config version the `TslConfig` shoud always contain
valid file paths and the whole field should be optional in the parent
strcut `HttpTracker`:

```rust
pub struct HttpTracker {
    pub enabled: bool,
    pub bind_address: SocketAddr,
    pub ssl_enabled: bool,
    #[serde(flatten)]
    pub tsl_config: Optional<TslConfig>,
}

pub struct TslConfig {
    pub ssl_cert_path: PathBuf,
    pub ssl_key_path: PathBuf,
}
```

That mean, the user could provide it or not, but if it's provided file
paths can't be empty.
We are using `String` to represent a filepath. We are refactoring to enrich types in configuration. Filepath should be represented with `PathBuf` but it allows non UTF-8 chars, so it can't be serialized.

Since we need to serialize config options (toml, json) is valid UTF-8 strings, we need a type that represents a valid fileptah but also is a valid UTF-8 strings. That makes impossible to use non-UTF8 fielpath. It seems that's a restriction accepted for a lot of projects including Rust `cargo`.
@josecelano josecelano force-pushed the 852-config-overhaul-replace-primitive-types-in-configuration-with-reacher-types branch from 7002f8a to 7a2f2a6 Compare May 10, 2024 15:04
@josecelano josecelano marked this pull request as ready for review May 10, 2024 15:05
@josecelano josecelano force-pushed the 852-config-overhaul-replace-primitive-types-in-configuration-with-reacher-types branch from 7a2f2a6 to adc21f8 Compare May 10, 2024 15:13
@josecelano josecelano force-pushed the 852-config-overhaul-replace-primitive-types-in-configuration-with-reacher-types branch from adc21f8 to b545b33 Compare May 10, 2024 15:20
@josecelano
Copy link
Member Author

ACK b545b33

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Code Cleanup / Refactoring Tidying and Making Neat

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Config overhaul: replace primitive types in configuration with richer types

1 participant