Skip to content

Conversation

@mamoreau-devolutions
Copy link
Contributor

Problem

The live productinfo.json from https://devolutions.net/productinfo.json contains a Date field (e.g., "Date": "2025-11-27") in each channel object that wasn't present in the Rust struct definition. This was causing parsing failures when the agent tried to deserialize the JSON for automatic updates.

Additionally, the parser had no resilience for future field additions and provided poor error diagnostics when parsing failed.

Solution

This PR improves the productinfo.json parser to be more robust and forward-compatible:

🔧 Changes Made

  1. Added optional Date field to ChannelData struct

    • Handles the Date field present in live productinfo.json
    • Remains optional for backward compatibility with test data
  2. Forward compatibility via #[serde(flatten)]

    • Unknown fields at product/channel level captured in _other HashMap
    • Future additions (e.g., ReleaseNotes, marketing fields) won't break parsing
  3. Input validation

    • Rejects empty URLs and hashes
    • Validates files array is not empty
    • Ensures required architecture exists
  4. Better error diagnostics

    • Structured logging with warn!() for each failure point
    • Logs product name, error details, and available architectures
    • Helps debugging in production environments
  5. Stricter ProductFile parsing

    • Added #[serde(deny_unknown_fields)] to catch typos in critical fields
    • Ensures structural integrity of file metadata

🧪 Testing

Added comprehensive test coverage:

  • ✅ Test parsing with Date field present
  • ✅ Test forward compatibility with unknown fields
  • ✅ Test validation rejects empty URLs/hashes
  • ✅ All existing tests pass

🎯 Impact

  • Fixes immediate parsing failures with live productinfo.json
  • Future-proof against schema additions
  • Better operational visibility when updates fail
  • No breaking changes to existing functionality

Issue: Reported by tester following PR #1591

- Add optional Date field to ChannelData struct to handle live productinfo.json
- Add forward compatibility via flattened _other HashMap for unknown fields
- Add validation for empty URLs and hashes
- Add detailed warning logs for parsing failures with context
- Add deny_unknown_fields to ProductFile for safety
- Add comprehensive tests for Date field, unknown fields, and validation
@github-actions
Copy link

github-actions bot commented Dec 1, 2025

Let maintainers know that an action is required on their side

  • Add the label release-required Please cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module) when you request a maintainer to cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module)

  • Add the label release-blocker Follow-up is required before cutting a new release if a follow-up is required before cutting a new release

  • Add the label publish-required Please publish libraries (`Devolutions.Gateway.Utils`, OpenAPI clients, etc) when you request a maintainer to publish libraries (Devolutions.Gateway.Utils, OpenAPI clients, etc.)

  • Add the label publish-blocker Follow-up is required before publishing libraries if a follow-up is required before publishing libraries


/// Information about a product file available for download
#[derive(Debug, Clone, Deserialize, Serialize)]
#[serde(deny_unknown_fields)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Does we need to make it stricter here?

Comment on lines +33 to +37
// Allow unknown fields at channel level for forward compatibility.
// New marketing fields or metadata won't break parsing.
#[serde(flatten)]
#[serde(skip_serializing)]
pub _other: HashMap<String, serde_json::Value>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: I suppose it’s to keep information when performing round-trip encoding?

@CBenoit
Copy link
Member

CBenoit commented Dec 1, 2025

@copilot Reviewing this PR, here is my feedback, citing the original PR’s body. Please, think and apply the suggestions.

Additionally, the parser had no resilience for future field additions and provided poor error diagnostics when parsing failed.

By default, serde_json will ignore the unknown fields: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=d33e97c76db39125f27ab5c2dd2963d2

use serde::Deserialize;

#[derive(Debug, Deserialize)]
struct MyData {
    // We only know about this one field
    known: i32,
}

fn main() {
    // JSON has an extra field `unknown` that does not exist in `MyData`
    let json = r#"{ "known": 1, "unknown": 42 }"#;

    // This succeeds: by default, unknown fields are ignored.
    let data: MyData = serde_json::from_str(json).unwrap();

    assert_eq!(data.known, 1);
    println!("{:?}", data); // MyData { known: 1 }
}

I don’t think the _other field is helping, unless we want to re-encode back (round-trip property). I am confident we do not need that at all. Actually, ProductFile and the other structs in db.rs do not even need the Serialize trait which can be removed safely to improve compilation speed.

Added #[serde(deny_unknown_fields)] to catch typos in critical fields. Ensures structural integrity of file metadata

serde_json already fails in case of a typo, stating that a required field is missing. If anything, this seems to hurt forward compatibility.

Better error diagnostics
Structured logging with warn!() for each failure point
Logs product name, error details, and available architectures
Helps debugging in production environments

Logging warnings in the FromStr impl is not clean. impl FromStr for ProductInfoDb should be modified to return a more specific error than UpdaterError, that is properly logged down the road. Either use anyhow::Error or create a new error type using thiserror. UpdaterError::ProductInfo variant should hold that more detailed error as a source. That way, the detailed error will be logged down the road, as appropriate. That way, tests can also be updated to check for the various failure points. Suggestion: using expect_test crate and the Debug formatting assertions, add tests showing we get the correct diagnostics.

Copy link
Contributor

Copilot AI commented Dec 1, 2025

@CBenoit I've opened a new pull request, #1595, to work on those changes. Once the pull request is ready, I'll request review from you.

@CBenoit
Copy link
Member

CBenoit commented Dec 1, 2025

Superseded by #1595

@CBenoit CBenoit closed this Dec 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants