Moved the DeletePackageFileAsync from IPackageFileService to ICorePackageFileService #4817
Conversation
| throw new ArgumentNullException(nameof(version)); | ||
| } | ||
|
|
||
| var normalizedVersion = NuGetVersionFormatter.Normalize(version); |
There was a problem hiding this comment.
1 - This is new behavior. Is there a guarantee that the old callers of this method work correctly with this?
2 - Why accept a string if we just convert it to a NuGetVersion and just normalize it? Could this accept a NuGetVersion instead?
There was a problem hiding this comment.
Normalizing a normalized version is fine.
Package.Version is not a version; it's a string. That is the most likely caller.
There was a problem hiding this comment.
Yes, but have we guaranteed that every place that this method is called is a normalized version? Are there other places in the code where we can remove calls to NuGetVersionFormatter.Normalize now that we've moved the call to here?
There was a problem hiding this comment.
See the BuildFileName(Package package, string format, string extension) implementation (which is in turn is used in SavePackageFileAsync that creates files we are trying to delete). It uses normalized version. We have to make sure we also use normalized version when deleting, otherwise we wouldn't be able to find the file.
Normalizing already normalized version does not change it as far as I understand.
As for why the string is passed instead of something else - I have no idea, I just moved the implementation to NuGetGallery.Core, so we can reuse it outside of Gallery. Changing the method signature seems out of scope of this change.
| @@ -348,6 +349,44 @@ public class TheDeletePackageFileMethod : FactsBase | |||
| } | |||
| } | |||
|
|
|||
| public class TheDeletePackageFileMethod : FactsBase | |||
There was a problem hiding this comment.
I would add a unit test to check that the version is normalized when calling to blob storage.
There was a problem hiding this comment.
It is doing that with ValidationFileName in the Verify below.
There was a problem hiding this comment.
It is implicitly checked by expecting the ValidationFileName to be passed to the file storage mock in the WillDeleteTheFileViaTheFileStorageService test.
No description provided.