Skip to content

Conversation

sandlbn
Copy link
Contributor

@sandlbn sandlbn commented Oct 7, 2025

After adding Merkle tree support, the API response format changed:

  • New fields added: content_format, sequence_number, hash, signature
  • Field renamed: manifest_json → exposed as manifest_json in API
  • Old manifests without Merkle fields failed to deserialize
  • Broke existing client code expecting the original format

Added backward-compatible API response format while preserving Merkle tree functionality:

  1. Field renaming: Use #[serde(rename = "manifest")] on manifest_json field
  2. Hide Merkle fields by default: Use #[serde(skip_serializing_if)] to hide metadata fields
  3. Optional metadata exposure: Added ?include_metadata=true query parameter
  4. Handle old manifests: Added #[serde(default)] for deserialization of legacy data

@sandlbn sandlbn requested a review from marcelamelara October 7, 2025 15:46
@sandlbn sandlbn self-assigned this Oct 7, 2025
Copy link
Contributor

@marcelamelara marcelamelara left a comment

Choose a reason for hiding this comment

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

Thanks @sandlbn ! I have only a minor comment about renaming include_metadata to something like include_tlog_metadata for clarity, otherwise this LGTM.

RUN groupadd -r appuser && useradd -r -g appuser appuser

# Create data directory and set ownership BEFORE switching to appuser
RUN mkdir -p /data && chown -R appuser:appuser /data
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this duplicated from #7 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is

src/main.rs Outdated

#[derive(Debug, Deserialize)]
struct GetManifestQuery {
include_metadata: Option<bool>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume include_metadata needs to be renamed to include_tlog_metadata in this file as well, if we make this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

sandlbn and others added 4 commits October 16, 2025 07:38
Co-authored-by: Marcela Melara <marcela.melara@intel.com>
Co-authored-by: Marcela Melara <marcela.melara@intel.com>
Co-authored-by: Marcela Melara <marcela.melara@intel.com>
@sandlbn sandlbn merged commit 343dff3 into main Oct 16, 2025
2 checks passed
@sandlbn sandlbn deleted the ms-fix-get-manifest branch October 16, 2025 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants