Add synced app-defined metadata for remote resources#75
Draft
hahn-kev wants to merge 3 commits into
Draft
Conversation
…teResource<TMetadata>. Apps opt in with ResourceService<TMetadata> and AddCrdtRemoteResources<TMetadata> so metadata participates in CRDT sync. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/SIL.Harmony/ResourceService.cs (1)
126-146:⚠️ Potential issue | 🟠 Major | ⚡ Quick winServer-returned metadata not persisted during pending upload completion.
When
UploadPendingResourcesprocesses pending uploads, theuploadResultmay contain server-authoredMetadata(similar toAddLocalResourceat line 80). However, onlyRemoteResourceUploadedChangeis created, which sets theRemoteIdbut doesn't persist any metadata returned by the server.If the server can return authoritative metadata during upload (e.g., computed hashes, timestamps), this would be lost for resources that were initially created as pending uploads.
Consider adding a
SetRemoteResourceMetadataChangewhenuploadResult.Metadatais not null:Proposed fix
foreach (var localResource in pendingUploads) { var uploadResult = await remoteResourceService.UploadResource(localResource.Id, localResource.LocalPath); changes.Add(new RemoteResourceUploadedChange<TMetadata>(localResource.Id, uploadResult.RemoteId)); + if (uploadResult.Metadata is not null) + { + changes.Add(new SetRemoteResourceMetadataChange<TMetadata>(localResource.Id, uploadResult.Metadata)); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/SIL.Harmony/ResourceService.cs` around lines 126 - 146, In the UploadPendingResources method, after creating the RemoteResourceUploadedChange for each uploaded resource, check if uploadResult.Metadata is not null. When metadata exists, also create a SetRemoteResourceMetadataChange with the server-returned metadata and add it to the changes list alongside the RemoteResourceUploadedChange. This ensures that any authoritative metadata returned by the server during upload is persisted in the same way as handled in the AddLocalResource method.
🧹 Nitpick comments (1)
src/SIL.Harmony/CrdtConfig.cs (1)
89-96: 💤 Low valueConsider using consistent JSON serialization options.
The
Metadataproperty conversion uses(JsonSerializerOptions?)null(default options), while the rest of the CRDT system uses configuredJsonSerializerOptionsfromCrdtConfig.JsonSerializerOptions. If custom type resolvers or converters are needed for metadata types, this could cause serialization mismatches.If this is intentional to keep metadata serialization simple and independent, consider adding a brief comment explaining the design choice.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/SIL.Harmony/CrdtConfig.cs` around lines 89 - 96, The Metadata property conversion is using default JSON serialization options (null) for both JsonSerializer.Serialize and JsonSerializer.Deserialize calls, while the rest of the CRDT system uses configured options from CrdtConfig.JsonSerializerOptions. Either update both the Serialize and Deserialize conversions to use CrdtConfig.JsonSerializerOptions instead of null to maintain consistency, or if intentional, add a brief comment in the HasConversion block explaining why Metadata serialization is kept simple and independent from the rest of the CRDT configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/SIL.Harmony.Tests/ResourceTests/RemoteServiceMock.cs`:
- Around line 47-52: The _metadata dictionary is being written with normalized
keys using Path.GetFullPath(localPath) in the SetUploadMetadata method on line
52, but read with raw localPath in the TryGetValue call on line 47. This causes
relative and absolute path variants to have mismatched keys and fail lookups.
Fix this by normalizing the key in the TryGetValue call on line 47 to use
Path.GetFullPath(localPath) instead of raw localPath, ensuring consistency
between how metadata is stored in SetUploadMetadata and retrieved in the upload
result method.
In `@src/SIL.Harmony/Resource/RemoteResource.cs`:
- Around line 47-50: The CloneMetadata method lacks validation to ensure
TMetadata can be safely serialized and deserialized to JSON, which can cause
runtime crashes during copy operations when non-serializable types are used. Add
fail-fast validation in the CloneMetadata method to check whether the metadata
instance can be successfully serialized before attempting the JSON round-trip
serialization, and throw a clear, descriptive exception if serialization fails.
This ensures deterministic error handling and prevents silent failures during
sync flows.
In `@src/SIL.Harmony/ResourceService.cs`:
- Around line 157-164: The UploadPendingResource method does not persist server
metadata from the uploadResult when available. When creating the
RemoteResourceUploadedChange<TMetadata> object, you need to extract and include
the metadata from uploadResult (in addition to the localResource.Id and
uploadResult.RemoteId that are already being passed). Check the
RemoteResourceUploadedChange<TMetadata> constructor to understand what metadata
parameter it expects, then update the method to pass the appropriate metadata
from uploadResult to ensure server-provided metadata is properly persisted in
the change log.
---
Outside diff comments:
In `@src/SIL.Harmony/ResourceService.cs`:
- Around line 126-146: In the UploadPendingResources method, after creating the
RemoteResourceUploadedChange for each uploaded resource, check if
uploadResult.Metadata is not null. When metadata exists, also create a
SetRemoteResourceMetadataChange with the server-returned metadata and add it to
the changes list alongside the RemoteResourceUploadedChange. This ensures that
any authoritative metadata returned by the server during upload is persisted in
the same way as handled in the AddLocalResource method.
---
Nitpick comments:
In `@src/SIL.Harmony/CrdtConfig.cs`:
- Around line 89-96: The Metadata property conversion is using default JSON
serialization options (null) for both JsonSerializer.Serialize and
JsonSerializer.Deserialize calls, while the rest of the CRDT system uses
configured options from CrdtConfig.JsonSerializerOptions. Either update both the
Serialize and Deserialize conversions to use CrdtConfig.JsonSerializerOptions
instead of null to maintain consistency, or if intentional, add a brief comment
in the HasConversion block explaining why Metadata serialization is kept simple
and independent from the rest of the CRDT configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40f5d8c9-4321-4140-a9d9-35674e8f033c
📒 Files selected for processing (20)
src/SIL.Harmony.Core/IRemoteResourceService.cssrc/SIL.Harmony.Sample/CrdtSampleKernel.cssrc/SIL.Harmony.Sample/MediaMetadata.cssrc/SIL.Harmony.Tests/DbContextTests.VerifyModel.verified.txtsrc/SIL.Harmony.Tests/ResourceTests/RemoteResourcesMetadataTests.cssrc/SIL.Harmony.Tests/ResourceTests/RemoteResourcesTests.cssrc/SIL.Harmony.Tests/ResourceTests/RemoteServiceMock.cssrc/SIL.Harmony.Tests/ResourceTests/WordResourceTests.cssrc/SIL.Harmony/CrdtConfig.cssrc/SIL.Harmony/CrdtKernel.cssrc/SIL.Harmony/Resource/CreateRemoteResourceChange.cssrc/SIL.Harmony/Resource/CreateRemoteResourcePendingUpload.cssrc/SIL.Harmony/Resource/DeleteRemoteResourceChange.cssrc/SIL.Harmony/Resource/HarmonyResource.cssrc/SIL.Harmony/Resource/RemoteResource.cssrc/SIL.Harmony/Resource/RemoteResourceNotEnabledException.cssrc/SIL.Harmony/Resource/RemoteResourceUploadedChange.cssrc/SIL.Harmony/Resource/SetRemoteResourceMetadataChange.cssrc/SIL.Harmony/ResourceService.cssrc/SIL.Harmony/SyncHelper.cs
Comment on lines
+47
to
+52
| _metadata.TryGetValue(localPath, out var metadata); | ||
| return new UploadResult<MediaMetadata>(remoteId, metadata); | ||
| } | ||
|
|
||
| public void SetUploadMetadata(string localPath, MediaMetadata metadata) => | ||
| _metadata[Path.GetFullPath(localPath)] = metadata; |
Contributor
There was a problem hiding this comment.
Normalize metadata lookup key in upload path handling.
Line 47 reads _metadata using raw localPath, but Line 52 writes using Path.GetFullPath(localPath). Equivalent relative/absolute inputs can miss metadata and skip expected override behavior.
Proposed fix
- _metadata.TryGetValue(localPath, out var metadata);
+ _metadata.TryGetValue(Path.GetFullPath(localPath), out var metadata);
return new UploadResult<MediaMetadata>(remoteId, metadata);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/SIL.Harmony.Tests/ResourceTests/RemoteServiceMock.cs` around lines 47 -
52, The _metadata dictionary is being written with normalized keys using
Path.GetFullPath(localPath) in the SetUploadMetadata method on line 52, but read
with raw localPath in the TryGetValue call on line 47. This causes relative and
absolute path variants to have mismatched keys and fail lookups. Fix this by
normalizing the key in the TryGetValue call on line 47 to use
Path.GetFullPath(localPath) instead of raw localPath, ensuring consistency
between how metadata is stored in SetUploadMetadata and retrieved in the upload
result method.
Comment on lines
+47
to
+50
| private static TMetadata? CloneMetadata(TMetadata? metadata) | ||
| { | ||
| if (metadata is null) return null; | ||
| return JsonSerializer.Deserialize<TMetadata>(JsonSerializer.Serialize(metadata)); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Configured metadata types via AddCrdtRemoteResources<TMetadata>:"
rg -nPo 'AddCrdtRemoteResources<\s*\K([A-Za-z_][A-Za-z0-9_\.]*)' --type=cs \
| sed -E 's/^[^:]+:[0-9]+://' | sort -u
echo
echo "Declaration scan (to detect interface/abstract metadata registrations):"
types=$(rg -nPo 'AddCrdtRemoteResources<\s*\K([A-Za-z_][A-Za-z0-9_\.]*)' --type=cs \
| sed -E 's/^[^:]+:[0-9]+://' | sed 's/.*\.//' | sort -u)
for t in $types; do
echo "== $t =="
rg -nP "^\s*(public|internal|protected|private)?\s*(abstract\s+class|interface|class|record)\s+${t}\b" --type=cs || true
done
echo
echo "Locate metadata clone call sites:"
rg -nP 'CloneMetadata|JsonSerializer\.Deserialize<\s*TMetadata\s*>\(' --type=csRepository: sillsdev/harmony
Length of output: 887
🏁 Script executed:
# Check RemoteResource<TMetadata> class definition and TMetadata constraints
rg -nA 20 "class RemoteResource<TMetadata>" --type=cs | head -60
# Also check if TMetadata has any where clauses
rg -nB 5 -A 20 "RemoteResource<TMetadata>" --type=cs | grep -E "(where|class|record)" | head -20Repository: sillsdev/harmony
Length of output: 3207
🏁 Script executed:
# Examine CrdtConfig.cs to understand how AddCrdtRemoteResources enforces constraints
rg -nB 5 -A 20 "AddCrdtRemoteResources" --type=csRepository: sillsdev/harmony
Length of output: 3227
🏁 Script executed:
# Check if there are any tests or comments documenting metadata type requirements
rg -ni "(metadata.*constraint|seriali.*metadata|json.*metadata)" --type=cs | head -20Repository: sillsdev/harmony
Length of output: 1016
CloneMetadata lacks validation for JSON-serializable metadata types, risking runtime crashes during copy operations.
RemoteResource<TMetadata> is constrained only to where TMetadata : class, which permits any reference type—including interfaces, abstract classes, and non-serializable concrete types. The Copy() method calls CloneMetadata() which silently assumes TMetadata can survive JSON round-trip serialization. Without validation, sync flows fail with unclear exceptions.
Add fail-fast validation and deterministic error handling:
Proposed hardening diff
private static TMetadata? CloneMetadata(TMetadata? metadata)
{
if (metadata is null) return null;
- return JsonSerializer.Deserialize<TMetadata>(JsonSerializer.Serialize(metadata));
+ if (typeof(TMetadata).IsInterface || typeof(TMetadata).IsAbstract)
+ {
+ throw new InvalidOperationException(
+ $"Metadata type '{typeof(TMetadata).FullName}' must be a concrete JSON-serializable type.");
+ }
+
+ try
+ {
+ return JsonSerializer.Deserialize<TMetadata>(JsonSerializer.Serialize(metadata))
+ ?? throw new InvalidOperationException(
+ $"Failed to clone metadata of type '{typeof(TMetadata).FullName}'.");
+ }
+ catch (Exception ex) when (ex is JsonException or NotSupportedException)
+ {
+ throw new InvalidOperationException(
+ $"Metadata type '{typeof(TMetadata).FullName}' is not JSON-serializable for copy operations.", ex);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private static TMetadata? CloneMetadata(TMetadata? metadata) | |
| { | |
| if (metadata is null) return null; | |
| return JsonSerializer.Deserialize<TMetadata>(JsonSerializer.Serialize(metadata)); | |
| private static TMetadata? CloneMetadata(TMetadata? metadata) | |
| { | |
| if (metadata is null) return null; | |
| if (typeof(TMetadata).IsInterface || typeof(TMetadata).IsAbstract) | |
| { | |
| throw new InvalidOperationException( | |
| $"Metadata type '{typeof(TMetadata).FullName}' must be a concrete JSON-serializable type."); | |
| } | |
| try | |
| { | |
| return JsonSerializer.Deserialize<TMetadata>(JsonSerializer.Serialize(metadata)) | |
| ?? throw new InvalidOperationException( | |
| $"Failed to clone metadata of type '{typeof(TMetadata).FullName}'."); | |
| } | |
| catch (Exception ex) when (ex is JsonException or NotSupportedException) | |
| { | |
| throw new InvalidOperationException( | |
| $"Metadata type '{typeof(TMetadata).FullName}' is not JSON-serializable for copy operations.", ex); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/SIL.Harmony/Resource/RemoteResource.cs` around lines 47 - 50, The
CloneMetadata method lacks validation to ensure TMetadata can be safely
serialized and deserialized to JSON, which can cause runtime crashes during copy
operations when non-serializable types are used. Add fail-fast validation in the
CloneMetadata method to check whether the metadata instance can be successfully
serialized before attempting the JSON round-trip serialization, and throw a
clear, descriptive exception if serialization fails. This ensures deterministic
error handling and prevents silent failures during sync flows.
Comment on lines
+157
to
164
| public async Task UploadPendingResource(LocalResource localResource, Guid clientId, | ||
| IRemoteResourceService<TMetadata> remoteResourceService) | ||
| { | ||
| ValidateResourcesSetup(); | ||
| var uploadResult = await remoteResourceService.UploadResource(localResource.Id, localResource.LocalPath); | ||
| await _dataModel.AddChange(clientId, new RemoteResourceUploadedChange(localResource.Id, uploadResult.RemoteId)); | ||
| await _dataModel.AddChange(clientId, | ||
| new RemoteResourceUploadedChange<TMetadata>(localResource.Id, uploadResult.RemoteId)); | ||
| } |
Contributor
There was a problem hiding this comment.
Same issue: server metadata not persisted in single-resource upload.
UploadPendingResource also doesn't persist metadata from uploadResult when it's provided by the server.
Proposed fix
var uploadResult = await remoteResourceService.UploadResource(localResource.Id, localResource.LocalPath);
-await _dataModel.AddChange(clientId,
- new RemoteResourceUploadedChange<TMetadata>(localResource.Id, uploadResult.RemoteId));
+var changes = new List<IChange> { new RemoteResourceUploadedChange<TMetadata>(localResource.Id, uploadResult.RemoteId) };
+if (uploadResult.Metadata is not null)
+{
+ changes.Add(new SetRemoteResourceMetadataChange<TMetadata>(localResource.Id, uploadResult.Metadata));
+}
+await _dataModel.AddChanges(clientId, changes);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/SIL.Harmony/ResourceService.cs` around lines 157 - 164, The
UploadPendingResource method does not persist server metadata from the
uploadResult when available. When creating the
RemoteResourceUploadedChange<TMetadata> object, you need to extract and include
the metadata from uploadResult (in addition to the localResource.Id and
uploadResult.RemoteId that are already being passed). Check the
RemoteResourceUploadedChange<TMetadata> constructor to understand what metadata
parameter it expects, then update the method to pass the appropriate metadata
from uploadResult to ensure server-provided metadata is properly persisted in
the change log.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RemoteResource<TMetadata>with syncedMetadataso clients can list media without downloading blobsResourceService<TMetadata>,HarmonyResource<TMetadata>, andAddCrdtRemoteResources<TMetadata>()DI registrationSetRemoteResourceMetadataChange<TMetadata>,DeleteRemoteResourceChange<TMetadata>(stabledelete:RemoteResourcetype name)IRemoteResourceService<TMetadata>generic;UploadResult<TMetadata>can return server-authored metadata that overrides client-passed metadata on uploadNoMetadatamarker type for apps that do not need synced metadata fieldsMediaMetadatarecord in SIL.Harmony.SampleBreaking changes
RemoteResource,ResourceService,HarmonyResource, andIRemoteResourceServiceare now generic-onlyCrdtConfig.AddRemoteResourceEntity()(non-generic) removed — useAddCrdtRemoteResources<TMetadata>()DeleteChange<RemoteResource>replaced byDeleteRemoteResourceChange<TMetadata>SyncWithResourceUploadnow requires<TMetadata>type argumentSummary by CodeRabbit
Release Notes
New Features
Tests