docs(data): 添加数据与存档系统文档并实现数据仓库功能#187
Merged
Merged
Conversation
- 新增数据与存档系统详细文档，涵盖核心概念和使用方法 - 实现 DataRepository 类提供统一数据持久化接口 - 添加 DataRepositoryOptions 配置选项支持备份和事件功能 - 实现完整的数据仓库测试用例验证持久化行为 - 支持多槽位存档管理和版本化数据迁移功能 - 提供批量数据操作和事件通知机制 - 实现自动备份功能防止数据丢失 - 支持聚合设置仓库统一管理多个配置项
Contributor
📝 WalkthroughWalkthroughRefactors repository persistence to centralize backup/write/event logic, tighten semantics (backup only on overwrite, batch-only events for SaveAll), adjust event payload typing, and add extensive tests for persistence and settings system behaviors. Changes
Sequence DiagramssequenceDiagram
participant Client as Client
participant DR as DataRepository
participant Store as Storage
participant EB as EventBus
Note over Client,EB: SaveAsync(key, value) with AutoBackup=true (overwrite)
Client->>DR: SaveAsync(key, value)
activate DR
DR->>Store: ExistsAsync(key)
alt key exists
DR->>Store: ReadAsync(key) %% read previous value
DR->>Store: WriteAsync(key + ".backup", oldValue)
end
DR->>Store: WriteAsync(key, value) %% overwrite main key
DR->>EB: Publish(DataSavedEvent<T>)
deactivate DR
EB-->>Client: Event delivered
sequenceDiagram
participant Client as Client
participant USR as UnifiedSettingsRepository
participant Lock as Lock
participant Clone as InMemoryClone
participant Store as Storage
participant EB as EventBus
Note over Client,EB: SaveAllAsync (unified repository, EnableEvents=true)
Client->>USR: SaveAllAsync(items)
activate USR
USR->>Lock: acquire
USR->>Clone: CloneFile(current)
Clone-->>USR: nextFile (apply mutations)
USR->>Store: ExistsAsync(unifiedKey)
alt exists and AutoBackup=true
USR->>Store: ReadAsync(unifiedKey)
USR->>Store: WriteAsync(unifiedKey + ".backup", oldFile)
end
USR->>Store: WriteAsync(unifiedKey, nextFile)
USR->>USR: swap committed snapshot (_file = nextFile)
USR->>Lock: release
USR->>EB: Publish(DataBatchSavedEvent)
deactivate USR
EB-->>Client: Batch event delivered
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| C# |  |
Apr 6, 2026 4:52a.m. | Review ↗ |
| Secrets | |
Apr 6, 2026 4:52a.m. | Review ↗ |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/zh-CN/game/data.md`:
- Around line 432-452: Update the "聚合设置仓库" section to include a minimal,
copy-pastable integration example that shows (1) registering concrete setting
types via RegisterDataType(...) with the same serializer/storage used by
UnifiedSettingsDataRepository, (2) calling LoadAllAsync() and handling
deserialization of mixed sections, and (3) subscribing to the new
DataLoadedEvent<T> signature (showing the event handler shape). Also add a short
migration note explaining that LoadAllAsync now requires registered data types
for polymorphic/mixed-section deserialization and that listeners must be updated
from the old load event to the generic DataLoadedEvent<T> handler.
In `@GFramework.Game.Tests/Data/PersistenceTests.cs`:
- Around line 264-297: The test only exercises homogeneous TestSimpleData and
misses the runtime-type batch path through SaveCoreUntypedAsync; update
DataRepository_SaveAllAsync_Should_Emit_Only_Batch_Event to include a
heterogeneous batch (e.g., one TestSimpleData and one other concrete IData type
like TestComplexData) and add an overwrite scenario (save an item once, then
SaveAllAsync with the same location but a different concrete type) to force the
runtime-type path; after SaveAllAsync assert the DataBatchSavedEvent count is 1,
verify each item round-trips (read back and assert concrete type and values) or
that backups were created as expected, and keep the existing event registrations
(context.RegisterEvent<DataSavedEvent<TestSimpleData>> and
context.RegisterEvent<DataBatchSavedEvent>) so the test fails if
SaveCoreUntypedAsync behavior regresses.
In `@GFramework.Game.Tests/Setting/SettingsSystemTests.cs`:
- Around line 79-111: Add a second distinct applicator (e.g., a
SecondaryTestSettings instance) to the FakeSettingsModel used in these tests and
update assertions to verify that ResetAll() causes both applicators' ApplyCount
to become 1 while Reset<PrimaryTestSettings>() causes only the
PrimaryTestSettings applicator ApplyCount to be 1 and the secondary applicator
ApplyCount to remain 0; locate and modify the tests that call
CreateContext(model) / CreateSystem(...) and reference PrimaryTestSettings,
ResetAll(), Reset<T>(), FakeSettingsModel, and the applicator.ApplyCount
properties when adding the new applicator and the new assertions.
In `@GFramework.Game/Data/UnifiedSettingsDataRepository.cs`:
- Around line 118-128: DeleteAsync and MutateAndPersistAsync mutate the
in-memory File/_file before persistence, causing state divergence if
WriteUnifiedFileCoreAsync or backup/read fails; change both methods to take a
snapshot (deep clone) of File (or copy the relevant Sections) before mutating,
perform persistence (WriteUnifiedFileCoreAsync/backup) against the snapshot, and
only on successful write swap in the snapshot into the _file/File field (or
otherwise restore the original _file on exception). Specifically update
DeleteAsync, MutateAndPersistAsync and any code paths that call
WriteUnifiedFileCoreAsync so they: create a snapshot of File.Sections or the
whole File, apply removals/mutations to the snapshot, call the existing
WriteUnifiedFileCoreAsync/backup logic, and on success assign the snapshot back
to _file; on failure leave _file unchanged and rethrow/log.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 17af3017-5a39-4a43-ad12-d49c178db6f7
📒 Files selected for processing (6)
GFramework.Game.Abstractions/Data/DataRepositoryOptions.csGFramework.Game.Tests/Data/PersistenceTests.csGFramework.Game.Tests/Setting/SettingsSystemTests.csGFramework.Game/Data/DataRepository.csGFramework.Game/Data/UnifiedSettingsDataRepository.csdocs/zh-CN/game/data.md
- 新增数据与存档系统完整文档，包含核心概念、基本用法和高级功能 - 实现文件存储、槽位存档仓库和统一设置仓库的持久化行为测试 - 覆盖存档迁移、备份恢复、批量操作等关键功能的测试用例 - 添加数据版本控制和自动备份机制的详细说明 - 提供完整的 API 接口文档和使用示例代码
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
GFramework.Game.Tests/Data/PersistenceTests.cs (2)
398-407: Consider using deserialization instead of string matching for backup verification.The assertions using
Does.Contain("\\\"Value\\\":1")are brittle as they depend on specific JSON formatting (double-encoding, whitespace, property ordering). If the serializer changes formatting, these tests will fail despite correct backup functionality.Consider deserializing the backup file and asserting on the typed object:
♻️ Suggested improvement
- var backupJson = await File.ReadAllTextAsync(Path.Combine(root, "settings.json.backup.json")); + var backupFile = await verifyStorage.ReadAsync<UnifiedSettingsFile>("settings.json.backup"); Assert.Multiple(() => { Assert.That(current.Value, Is.EqualTo(2)); Assert.That(savedEventCount, Is.Zero); Assert.That(batchEventCount, Is.EqualTo(1)); - Assert.That(backupJson, Does.Contain("settings/graphics")); - Assert.That(backupJson, Does.Contain("\\\"Value\\\":1")); + Assert.That(backupFile.Sections.ContainsKey("settings/graphics"), Is.True); + // Deserialize and verify the old value was preserved });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@GFramework.Game.Tests/Data/PersistenceTests.cs` around lines 398 - 407, The test currently verifies the backup by string matching on backupJson (read via File.ReadAllTextAsync) which is brittle; instead deserialize the backup JSON into the settings DTO/typed model used by the code (the same type that contains the "Value" and "settings/graphics" properties) and assert on the object fields (e.g., check that the deserialized object's settings.graphics.Value == 1 and that the expected path/key exists) while keeping the other assertions (current.Value, savedEventCount, batchEventCount) unchanged; locate the assertions block around the backupJson variable and replace the Does.Contain("\\\"Value\\\":1") string check with a deserialization step and typed assertions against that object.
724-826: Consider implementingIDisposablefor proper resource cleanup.
ToggleWriteFailureStoragewraps anIStorage(innerStorage) but doesn't implementIDisposable. IfFileStorage(which implementsIDisposablebased on theusingstatements in tests) is passed in, the inner storage won't be disposed through this wrapper.While this is less critical for test code since the outer
usingstatement handles theFileStoragedirectly in most tests, it could be an issue if someone uses this wrapper in ausingstatement expecting it to clean up.♻️ Suggested improvement
- private sealed class ToggleWriteFailureStorage(IStorage innerStorage, string failingKey) : IStorage + private sealed class ToggleWriteFailureStorage(IStorage innerStorage, string failingKey) : IStorage, IDisposable { + /// <inheritdoc /> + public void Dispose() + { + (innerStorage as IDisposable)?.Dispose(); + } + // ... rest of implementation }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@GFramework.Game.Tests/Data/PersistenceTests.cs` around lines 724 - 826, ToggleWriteFailureStorage currently wraps an IStorage (constructor ToggleWriteFailureStorage(IStorage innerStorage, string failingKey)) but does not implement IDisposable, so wrapped disposables like FileStorage won’t be cleaned up when this wrapper is disposed; implement IDisposable on ToggleWriteFailureStorage, add a Dispose method (and standard Dispose(bool) pattern if desired) that calls Dispose on innerStorage if innerStorage is IDisposable, and ensure Dispose is idempotent (guarded by a disposed flag) and suppress finalization if using the full pattern; keep existing methods (Exists, Read, Write, ThrowIfNeeded, etc.) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@GFramework.Game.Tests/Data/PersistenceTests.cs`:
- Around line 398-407: The test currently verifies the backup by string matching
on backupJson (read via File.ReadAllTextAsync) which is brittle; instead
deserialize the backup JSON into the settings DTO/typed model used by the code
(the same type that contains the "Value" and "settings/graphics" properties) and
assert on the object fields (e.g., check that the deserialized object's
settings.graphics.Value == 1 and that the expected path/key exists) while
keeping the other assertions (current.Value, savedEventCount, batchEventCount)
unchanged; locate the assertions block around the backupJson variable and
replace the Does.Contain("\\\"Value\\\":1") string check with a deserialization
step and typed assertions against that object.
- Around line 724-826: ToggleWriteFailureStorage currently wraps an IStorage
(constructor ToggleWriteFailureStorage(IStorage innerStorage, string
failingKey)) but does not implement IDisposable, so wrapped disposables like
FileStorage won’t be cleaned up when this wrapper is disposed; implement
IDisposable on ToggleWriteFailureStorage, add a Dispose method (and standard
Dispose(bool) pattern if desired) that calls Dispose on innerStorage if
innerStorage is IDisposable, and ensure Dispose is idempotent (guarded by a
disposed flag) and suppress finalization if using the full pattern; keep
existing methods (Exists, Read, Write, ThrowIfNeeded, etc.) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: da71445b-60b2-4e8d-b8aa-412f4bc2231a
📒 Files selected for processing (5)
GFramework.Game.Tests/Data/PersistenceTestUtilities.csGFramework.Game.Tests/Data/PersistenceTests.csGFramework.Game.Tests/Setting/SettingsSystemTests.csGFramework.Game/Data/UnifiedSettingsDataRepository.csdocs/zh-CN/game/data.md
✅ Files skipped from review due to trivial changes (1)
- GFramework.Game.Tests/Data/PersistenceTestUtilities.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- GFramework.Game/Data/UnifiedSettingsDataRepository.cs
- GFramework.Game.Tests/Setting/SettingsSystemTests.cs
This was referenced Apr 19, 2026
This was referenced May 10, 2026
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 by CodeRabbit
Bug Fixes
Refactor
Tests
Documentation