Fix/602: root folder deletion#607
Conversation
T4g1
force-pushed
the
fix/602-root-folder-deletion
branch
from
4173e88 to
d6683ce
Compare
therobbiedavis
left a comment
therobbiedavis
left a comment
There was a problem hiding this comment.
Thanks for the updates here, the original comments are resolved but sorry for finding more stuff.
|
|
||
| // Audiobooks are considered orphaned if base path is empty or no root folder can be linked to it | ||
| var orphanedAudiobooks = audiobooks | ||
| .Where(a => string.IsNullOrEmpty(a.BasePath) || !rootFolders.Any(r => a.BasePath!.StartsWith(r.Path))); |
There was a problem hiding this comment.
The trailing separator fix helps, but I’d still avoid raw StartsWith for path ownership. This comparison is case-sensitive, so Windows-style paths with different casing can be treated as unrelated and mark a real child audiobook as orphaned.
| } | ||
| set | ||
| { | ||
| var normalized = FileUtils.NormalizeStoredPath(value); |
There was a problem hiding this comment.
I think we need to guard null/empty before normalizing here. Right now NormalizeStoredPath(null) / NormalizeStoredPath("") can turn “no BasePath” into the current working directory, which breaks callers checking for an empty BasePath.
| } | ||
|
|
||
| _db.Audiobooks.Update(audiobook); | ||
| await _db.SaveChangesAsync(); |
There was a problem hiding this comment.
I think this lost the actual update. SaveChangesAsync only persists if the audiobook is already tracked, so a detached entity can return true here while saving nothing.
There was a problem hiding this comment.
The update on tracked entities throws errors (you can check the pipelines on the MR) so i either remove the tracking or go full tracking, i went for the default tracking behavior here
So it is assumed the entities are tracked here (which is the case in the repository used currently iirc)
There was a problem hiding this comment.
Ah ok, I think my primary concern is the method contract more than this specific call path. It now only works for already-tracked instances, but still looks like a general update method and returns true for detached entities too. Maybe instead we only call Update when the entity is detached? WDYT?
There was a problem hiding this comment.
I believe its a matter of convention, but we have an issue: we have inconsistent behavior right now (some method do track others dont, and by default, the framework does implicit tracking)
Like I said, I went for the implicit tracking pattern but we could also:
- Disable tracking: Which requires us to explicitely update objects
- Explicit the tracking mode in the method name
I considered the approach to check if it's tracked or not too but it feels a bit like code smell for me as if we have to do that, it's maybe a lack of comprehension about the inner working of the EF framework from the dev that needs it, i can't really think of a use case where we would want to update an untracked object out of thin air)
There was a problem hiding this comment.
I don’t see DisableHostedServices being read, so WebApplicationFactory tests will start the background workers when the test host boots.
2 participants
Summary
When removing root folders that are contained within other root folders, audiobooks can be detected as using both of them.
I fixed that issue by making it count the amount of root folders associated with each audiobooks and allowing the deletion only if none of them is orphaned from all root folders
Testing
Notes