Repository navigation
Refuse a second app data type that resolves to another type's file [patch] - #384
Open
matt-edmondson wants to merge 1 commit into
Open
matt-edmondson wants to merge 1 commit into
matt-edmondson wants to merge 1 commit into
Conversation
…atch] The default file name is the simple type name, so two Settings classes in different namespaces, or two closed generics of one class, used one file under different locks, and each type's save wiped the other's data. The first type to load or save a path now owns it on that file system, and a different type throws InvalidOperationException naming both types and the overloads that give it its own file. Fixes #336 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017vfJW44rALLJaHHtXa8D2t
|
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
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.
Fixes #336
What was wrong
The default file name is the simple type name in snake case.
PluginA.SettingsandPluginB.Settingstherefore both usedsettings.json, and so didGen<int>andGen<string>. Each type held its own lock, loaded the other's file without error, and saved over it, so one type's data was silently wiped.Change
This follows the triage comment: the default name stays as it is, since renaming it would move existing users' data, and a collision is detected instead.
AppData.ClaimFile<T>(path)records the first type that loads or saves each path. A different type that resolves to the same path then throwsInvalidOperationException. The message names both types, with namespace and generic arguments, and points to theLoadOrCreate(fileName)andLoadOrCreate(subdirectory)overloads.LoadOrCreatecalls it before reading the file, andSave()calls it before writing, so an instance created directly withnew()is covered too.ConditionalWeakTablekeyed byIFileSystem. In production there is one default file system. In tests each thread has its ownMockFileSystem, where the same path is a different file, so parallel tests that reuse a type name in different classes don't trip it.Behaviour change: an app that already has two colliding types gets an exception on the second one's first load, where before it silently lost data. Loading the same type repeatedly is unaffected.
Tests
New
FileCollisionTests:SameNamedTypesInDifferentNamespacesAreRefused: the issue's repro. The second type throws, both names are in the message, and the first type's data survives.ClosedGenericsOfOneClassAreRefusedSaveIsRefusedForASecondTypeTooTheSameTypeCanLoadAndSaveRepeatedly,SameNamedTypesWithTheirOwnFileNamesCoexist, andTheSamePathOnAnotherFileSystemIsAnotherFile.With
AppData.csreverted, 3 of the 6 new cases fail, and the 3 guards pass either way. Full suite on Linux / .NET 10: 151/151 passed. A Release build of the library succeeds for all six target frameworks.Generated by Claude Code