refactor: Improve MagnetUri field types for spec compliance

[artrixdotdev]

#168

Summary by CodeRabbit

• Refactor
  • Improved magnet-link handling to support multiple sources, exact sources, and peers and to handle larger length values for more robust link processing.
• Tests
  • Added tests validating parsing of multi-valued magnet URI parameters to ensure correct behavior.

[coderabbitai]

Walkthrough

MagnetUri fields were changed to support multi-valued parameters and a larger length type: length from Option<u32> → Option<usize>, and source, exact_source, peer from Option<String> → Vec<String> with serde rename attributes and default. A test for multi-valued magnet params was added.

Changes

Cohort / File(s)	Summary
MagnetUri struct field updates crates/libtortillas/src/metainfo/magnet.rs	Changed MagnetUri.length from Option<u32> → Option<usize>; changed source (as), exact_source (xs), and peer (x.pe) from Option<String> → Vec<String> and added #[serde(rename = "...", default)] for each.
Tests crates/libtortillas/src/metainfo/magnet.rs (tests section)	Added test_parse_magnet_uri_multi_valued_params to validate parsing of multi-valued as, xs, and x.pe parameters.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

• Areas needing attention:
  • All call sites that read MagnetUri.source, exact_source, or peer must handle Vec<String> (iteration, empty vector) instead of Option<String>.
  • Verify places assuming Option::None now interpret empty Vec as equivalent where appropriate.
  • Confirm MagnetUri.length uses remain valid with usize (platform/range implications and any serialization expectations).
  • Ensure serde behavior with rename and default matches existing wire-format expectations.

Possibly related issues

• Improve MagnetUri field types for spec compliance #168 — Implements the same multi-valued serde changes for as, xs, and x.pe and adjusts xl/length, so this PR can be linked as addressing that objective.

Possibly related PRs

• fix: Parse multiple webseeds #72 — Also converts single-valued magnet URI fields to Vec<String> with serde defaults for multi-valued parameters; strongly related implementation-wise.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title "refactor: Improve MagnetUri field types for spec compliance" is directly related to the main changes in the changeset. The PR modifies the MagnetUri struct by changing field types from Option<T> to Vec<T> for multi-valued parameters and adjusting the length field type, along with adding serde annotations and a test for multi-valued parameter parsing. The title accurately captures that the core objective is improving field types in the MagnetUri struct for specification compliance, and it is concise, clear, and free from vague or generic language. A teammate scanning the commit history would immediately understand that this PR is about refactoring MagnetUri field types to better align with magnet URI specifications.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch refactor/magnet-uri-spec-compliance

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a6d7f7d and e2c8b33.

📒 Files selected for processing (1)

• crates/libtortillas/src/metainfo/magnet.rs (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

crates/libtortillas/src/metainfo/magnet.rs (2)

crates/libtortillas/src/metainfo/mod.rs (2)

• MetaInfo (13-16)
• MetaInfo (18-57)

crates/libtortillas/src/metainfo/file.rs (1)

• TorrentFile (17-32)

🔇 Additional comments (2)

crates/libtortillas/src/metainfo/magnet.rs (2)

33-34: No issues detected with the exact_source field type change.

The field is only populated through automatic serde deserialization with ParseMode::Duplicate, which correctly handles multiple values. This change aligns with the Magnet URI specification and matches the pattern used by other multi-valued fields in the struct (web_seed, source, peer). Since there are no direct code usages of this field elsewhere in the codebase, all references are implicitly updated through the schema change.

---

30-31: No direct usages of the source field were found in the codebase.

The type change from Option<String> to Vec<String> is appropriate and safe:

• The serde configuration with rename = "as" and default correctly handles the Magnet URI spec's multiple as parameters
• ParseMode::Duplicate on line 70 properly collects multiple values into the vector
• The public struct is only used through methods like parse() and info_hash(), not through direct field access
• Serialization and deserialization are handled automatically by serde

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

crates/libtortillas/src/metainfo/magnet.rs (1)

22-22: Portability concern: usize is platform-dependent.

The previous review suggested using u64 instead of usize for consistent large-file support across all platforms. This concern remains unaddressed—on 32-bit systems, usize limits file sizes to ~4GB, the same constraint as the original u32.

🧹 Nitpick comments (1)

crates/libtortillas/src/metainfo/magnet.rs (1)

131-140: Enhance test to validate actual values and edge cases.

The test correctly validates that multiple parameters are collected into vectors. However, it only checks lengths without verifying the actual values were parsed correctly.

Consider enhancing the test to validate actual values and edge cases:

 #[tokio::test]
 #[traced_test]
 async fn test_parse_magnet_uri_multi_valued_params() {
    let uri = "magnet:?xt=urn:btih:xyz&as=seed1&as=seed2&xs=exact1&xs=exact2&x.pe=peer1&x.pe=peer2&dn=name";
    let magnet = MagnetUri::try_from(uri.to_string()).unwrap();

    assert_eq!(magnet.source.len(), 2);
+   assert_eq!(magnet.source[0], "seed1");
+   assert_eq!(magnet.source[1], "seed2");
    assert_eq!(magnet.exact_source.len(), 2);
+   assert_eq!(magnet.exact_source[0], "exact1");
+   assert_eq!(magnet.exact_source[1], "exact2");
    assert_eq!(magnet.peer.len(), 2);
+   assert_eq!(magnet.peer[0], "peer1");
+   assert_eq!(magnet.peer[1], "peer2");
 }
+
+#[tokio::test]
+#[traced_test]
+async fn test_parse_magnet_uri_single_and_empty_params() {
+   // Test with single value and absent parameters
+   let uri = "magnet:?xt=urn:btih:xyz&as=single&dn=name";
+   let magnet = MagnetUri::try_from(uri.to_string()).unwrap();
+   
+   assert_eq!(magnet.source.len(), 1);
+   assert_eq!(magnet.source[0], "single");
+   assert_eq!(magnet.exact_source.len(), 0); // default empty vec
+   assert_eq!(magnet.peer.len(), 0); // default empty vec
+}

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e2c8b33 and 89453f8.

📒 Files selected for processing (1)

• crates/libtortillas/src/metainfo/magnet.rs (3 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

crates/libtortillas/src/metainfo/magnet.rs (1)

crates/libtortillas/src/hashes.rs (1)

• len (344-346)

🔇 Additional comments (3)

crates/libtortillas/src/metainfo/magnet.rs (3)

33-34: Breaking change: Verify downstream usage of exact_source field.

The change from Option<String> to Vec<String> is confirmed and aligns with the multi-valued parameter support across source, exact_source, and peer fields. Codebase analysis found only test usage (line 138) already expecting Vec behavior via .len() calls. No production code breakage detected within this repository.

However, since libtortillas is a published library (v0.0.0), external consumers accessing magnet.exact_source will need to adapt their code. While this pre-release version permits breaking changes, library maintainers should confirm downstream impact.

---

45-46: Test coverage added; internal usage is safe.

The change correctly implements multi-valued parameter support for the x.pe (peer) parameter. Test coverage validating the Vec<String> type has been added at lines 131-140.

No production code in the internal codebase accesses the peer field, so this change introduces no breaking changes within the repository scope.

---

30-31: Breaking change correctly implemented; no migration needed in codebase.

Verification confirms there are no usage patterns within the codebase that require migration. The only .source field access found (line 137 test) already uses the Vec-compatible .len() method. No Option-based patterns (if let Some, unwrap, expect) exist on this field.

This is a breaking change for external API consumers (requires SEMVER major bump), but the codebase itself is fully compatible with the new Vec<String> type and #[serde(default)] implementation.