Skip to content

Store the new instance in FileSystem and Temp RetrieveOrCreateAsync - #59

Merged
matt-edmondson merged 3 commits into
mainfrom
fix/retrieve-or-create-stores
Sep 28, 2026
Merged

matt-edmondson merged 3 commits into
mainfrom
fix/retrieve-or-create-stores

Conversation

@matt-edmondson

Copy link
Copy Markdown
Contributor

Fixes #55

What changed

IPersistenceProvider.RetrieveOrCreateAsync is documented to create and store a new instance when the key is missing. The interface default does this. FileSystemPersistenceProvider and TempPersistenceProvider declared their own public override that returned new T() without storing it. As a result, "first call creates the record" worked on InMemory, ConfigHome and DataHome but never persisted on these two.

Both overrides now store the new instance before returning it, the same way the default does. I kept the methods rather than deleting them, as the triage suggested, because deleting them would remove public members that the package-validation baseline covers.

Tests

  • I added RetrieveOrCreate_Stores_The_New_Instance_On_Every_Provider. It calls through IPersistenceProvider<string> on the default (InMemory), FileSystem and Temp providers and asserts ExistsAsync afterwards.
  • With the old providers restored, the new test fails ("FileSystem did not store the new instance"). With the fix it passes.
  • The full Essentials.Tests suite passes: 917/917. The Temp and FileSystem providers build cleanly in Release.

The triage also mentions #56 (GetAllKeysAsync omitting some key types) on the same providers. That is left for its own PR.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VPis8BNX5ETFM2XytgJCz7


Generated by Claude Code

Both providers declared their own RetrieveOrCreateAsync that returned a
default without storing it, so the call persisted on InMemory, ConfigHome
and DataHome but not on these two. Store it as the interface documents.

Fixes #55

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VPis8BNX5ETFM2XytgJCz7
Comment thread Essentials.Tests/PersistenceProviderTests.cs Fixed
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VPis8BNX5ETFM2XytgJCz7
Both providers declare the member publicly, which hides the interface
default, and had each restated it. Move the body to a linked Shared
helper so the two cannot drift apart again.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VPis8BNX5ETFM2XytgJCz7
@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
4.7% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

Copy link
Copy Markdown
Contributor Author

SonarCloud Code Analysis is still red: 4.7% duplication on new code (limit 3%). This comes from duplication already on main, not from anything new this PR adds.

The first push duplicated the new method body across the two providers (37.9%). 05ac144 moved that body into Shared/PersistenceDefaults.cs, and Sonar reports 0 duplicated lines in that file. What is left is the one-line delegation in each provider, 2 lines per file. Each of those lines sits inside a block that TempPersistenceProvider and FileSystemPersistenceProvider already duplicate on main. Sonar reports 161 of Temp's 271 lines as duplicated, including a single 125-line block starting at about line 42. Any edit inside that block is counted as duplicated new code. Re-running would not help, because the result is deterministic.

I haven't widened this PR to remove that duplication. The fix I'd propose is a follow-up that makes TempPersistenceProvider wrap a FileSystemPersistenceProvider<TKey> rooted at its temp directory, as ConfigHomePersistenceProvider already does. Temp would keep only its own ProviderName, IsPersistent, CleanupDirectory and Dispose. That should shrink the file from about 270 lines to about 90. It would also remove this PR's Temp change entirely, because Temp would inherit FileSystem's RetrieveOrCreateAsync.

Two ways forward:

  1. Merge this PR as-is, accepting the Sonar result for this known pre-existing duplication, and do the Temp refactor separately.
  2. Fold the Temp→FileSystem wrap into this PR. That is a larger change than RetrieveOrCreateAsync doesn't store the new instance on FileSystem/Temp persistence, although the interface (and InMemory/ConfigHome/DataHome) do #55 asked for.

The tests pass on all three OSes, and the new regression test fails without the fix. The only open question is which of the two options to take.


Generated by Claude Code

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.

RetrieveOrCreateAsync doesn't store the new instance on FileSystem/Temp persistence, although the interface (and InMemory/ConfigHome/DataHome) do

2 participants