Add package version to query string for v2 CDN endpoint (#10082) #10083

clairernovotny · 2024-07-26T20:59:17Z

PR Classification

New feature: Added support for handling a version parameter in cloud blob storage services

Implements #10082

PR Summary

Introduced a version parameter to enhance file storage service methods and updated related tests.

CloudBlobFileStorageService.cs: Modified constructor and methods to handle versionParameter.
IFileStorageService: Updated interface to include versionParameter.
FileSystemFileStorageService.cs: Updated methods to handle versionParameter.
PackageFileService: Passed versionParameter in method calls.
Updated tests in CloudBlobFileStorageServiceFacts, FileSystemFileStorageServiceFacts, and PackageFileServiceFacts to include versionParameter.

Add the packageVersion to the query string of cdn redirect url's Added a new constant `PackageVersionParameterName` to `CoreConstants.cs`. Reorganized and added necessary `using` directives in multiple files. Modified `CloudBlobFileStorageService` to include and handle the new `versionParameter` in its constructor and methods. Refactored `GetRedirectUri` to include the `versionParameter` in the query string. Added a helper method `ParseQueryString` in `CloudBlobFileStorageService`. Updated `IFileStorageService` interface to include the `versionParameter`. Updated `FileSystemFileStorageService` and `SymbolPackageFileService` to handle the `versionParameter`. Updated `PackageFileService` to pass the `versionParameter`. Updated and added tests in corresponding test files to verify the handling of the `versionParameter`.

joelverhagen · 2024-07-29T12:40:29Z

src/NuGetGallery.Services/Storage/CloudBlobFileStorageService.cs

            : base(client, diagnosticsService, cloudBlobFolderInformationProvider)
        {
            _configuration = configuration;
            _redirectPolicy = redirectPolicy;
        }

-        public async Task<ActionResult> CreateDownloadFileActionResultAsync(Uri requestUrl, string folderName, string fileName)
+        public async Task<ActionResult> CreateDownloadFileActionResultAsync(Uri requestUrl, string folderName, string fileName, string versionParameter)


This is a low level method not specific to a package or version. What do you think about a simple queryString parameter instead?

This would allow flexibility at the call site.

I looked a bit closer and the easiest approach hardcodes the parameter name lower down--this just passes the version. Making it more generic would also put the burden on a properly urlencoded query string on the caller.

Seems okay to me. A Dictionary<string, string> would be more flexible. but it's not a big deal to me.

joelverhagen · 2024-07-29T12:42:34Z

src/NuGetGallery/Services/PackageFileService.cs

@@ -28,13 +29,18 @@ public PackageFileService(IFileStorageService fileStorageService)
        public Task<ActionResult> CreateDownloadPackageActionResultAsync(Uri requestUrl, Package package)
        {
            var fileName = FileNameHelper.BuildFileName(package, CoreConstants.PackageFileSavePathTemplate, CoreConstants.NuGetPackageFileExtension);
-            return _fileStorageService.CreateDownloadFileActionResultAsync(requestUrl, CoreConstants.Folders.PackagesFolderName, fileName);
+
+            var packageVersion = NuGetVersionFormatter.GetNormalizedPackageVersion(package).ToLowerInvariant(); // will not return null bc BuildFileName will throw if values are null


Why lower invariant? Generally we use the original casing when we have it and allow the user of the value to do the comparison in a case insensitive manner if necessary. This means we can retain that original author intent as much as possible.

src/NuGetGallery/Services/PackageFileService.cs

tests/NuGetGallery.Facts/Services/CloudBlobFileStorageServiceFacts.cs

src/NuGetGallery/Services/SymbolPackageFileService.cs

joelverhagen · 2024-07-29T13:14:23Z

tests/NuGetGallery.Facts/Services/FileSystemFileStorageServiceFacts.cs

@@ -1,6 +1,11 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

+using Moq;
+
+using NuGetGallery.Configuration;


No need to organize usings. IDE will mess it up as we go.

Unless it's possible to add it to encode the rule in editorconfig

If you set dotnet_sort_system_directives_first = true in editorconfig and once it's in the correct order, the IDE will insert new usings correctly.

It used to get weird with using aliases (e.g. using Foo = System.Foo), but I don't know if it still does or not.

I've added an editorconfig to the repo to resolve this.

joelverhagen

Minor comments

src/NuGetGallery.Services/Storage/CloudBlobFileStorageService.cs

src/NuGetGallery/Services/PackageFileService.cs

erdembayar · 2024-07-29T18:25:42Z

src/NuGetGallery.Services/Storage/CloudBlobFileStorageService.cs

+
+            string query = uri.Query.Substring(queryIndex);
+
+            var nvcol = HttpUtility.ParseQueryString(query);


Explicit type please

erdembayar · 2024-07-29T18:28:21Z

Please add tracking issue link in description.

Enhanced null and whitespace checks in `CreateDownloadPackageActionResultAsync` method of `PackageFileService.cs`. Modified `ParseQueryString` in `CloudBlobFileStorageService.cs` to return `HttpUtility.ParseQueryString(string.Empty)` when query index is `-1`. Updated and removed tests in `CloudBlobFileStorageServiceFacts.cs`. Added new tests in `PackageFileServiceFacts.cs` for null or empty `version` and `id`.

jimmylewis · 2024-07-29T20:10:10Z

.editorconfig

+csharp_prefer_braces = true:silent
+csharp_prefer_simple_using_statement = true:suggestion
+csharp_space_around_binary_operators = before_and_after
+csharp_indent_labels = no_change


nit: ironically this file doesn't have a trailing newline. 😜

jimmylewis · 2024-07-29T20:10:46Z

.editorconfig

+; Helpful errors
+dotnet_diagnostic.CA2017.severity = error
+dotnet_diagnostic.CS0105.severity = error
+dotnet_diagnostic.IDE0005.severity = error


nit: would be nice to keep the diagnostics alphabetically sorted by ID, as we will likely add more rules over time, and duplicates are tedious to find when the list is long.

jimmylewis

LGTM with a few nits, but I'll defer to Joel for his design feedback comments.

jimmylewis · 2024-07-31T19:04:14Z

src/NuGetGallery/Services/FileSystemFileStorageService.cs

@@ -1,14 +1,15 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

+using NuGetGallery.Configuration;


nit: this file and a few others modified in this PR don't adhere to the new style rules put in place by this PR (.editorconfig).

.editorconfig

clairernovotny requested a review from a team as a code owner July 26, 2024 20:59

joelverhagen reviewed Jul 29, 2024

View reviewed changes

src/NuGetGallery/Services/PackageFileService.cs Show resolved Hide resolved

joelverhagen reviewed Jul 29, 2024

View reviewed changes

tests/NuGetGallery.Facts/Services/CloudBlobFileStorageServiceFacts.cs Outdated Show resolved Hide resolved

joelverhagen reviewed Jul 29, 2024

View reviewed changes

src/NuGetGallery/Services/SymbolPackageFileService.cs Outdated Show resolved Hide resolved

joelverhagen reviewed Jul 29, 2024

View reviewed changes

joelverhagen requested changes Jul 29, 2024

View reviewed changes

jimmylewis requested changes Jul 29, 2024

View reviewed changes

src/NuGetGallery.Services/Storage/CloudBlobFileStorageService.cs Outdated Show resolved Hide resolved

src/NuGetGallery.Services/Storage/CloudBlobFileStorageService.cs Outdated Show resolved Hide resolved

src/NuGetGallery/Services/PackageFileService.cs Outdated Show resolved Hide resolved

erdembayar reviewed Jul 29, 2024

View reviewed changes

clairernovotny added 3 commits July 29, 2024 14:28

Add editorconfig to ensure code isn't formatted incorrectly on save

030504e

Fixes from feedback

e5c703f

jimmylewis reviewed Jul 29, 2024

View reviewed changes

Use explicit types per feedback

b3ccb52

clairernovotny requested review from jimmylewis and joelverhagen July 31, 2024 18:33

jimmylewis previously approved these changes Jul 31, 2024

View reviewed changes

joelverhagen previously approved these changes Aug 1, 2024

View reviewed changes

Make errors into suggestions

7a6e130

clairernovotny dismissed stale reviews from joelverhagen and jimmylewis via 7a6e130 August 1, 2024 15:31

joelverhagen reviewed Aug 1, 2024

View reviewed changes

.editorconfig Show resolved Hide resolved

joelverhagen approved these changes Aug 1, 2024

View reviewed changes

clairernovotny merged commit 4036cf1 into dev Aug 1, 2024
2 checks passed

clairernovotny deleted the cnov-package-version-param branch August 1, 2024 16:14

joelverhagen mentioned this pull request Aug 16, 2024

Merge branch dev into main (NuGet.Jobs and ServerCommon merge) #10135

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add package version to query string for v2 CDN endpoint (#10082) #10083

Add package version to query string for v2 CDN endpoint (#10082) #10083

clairernovotny commented Jul 26, 2024 •

edited

Loading

joelverhagen Jul 29, 2024

joelverhagen Jul 29, 2024

clairernovotny Jul 29, 2024

joelverhagen Aug 1, 2024

joelverhagen Jul 29, 2024

joelverhagen Jul 29, 2024

joelverhagen Jul 29, 2024 •

edited

Loading

jimmylewis Jul 29, 2024

clairernovotny Jul 29, 2024

joelverhagen left a comment

erdembayar Jul 29, 2024

erdembayar commented Jul 29, 2024

jimmylewis Jul 29, 2024

jimmylewis Jul 29, 2024

jimmylewis left a comment

jimmylewis Jul 31, 2024


		string query = uri.Query.Substring(queryIndex);

		var nvcol = HttpUtility.ParseQueryString(query);

Add package version to query string for v2 CDN endpoint (#10082) #10083

Add package version to query string for v2 CDN endpoint (#10082) #10083

Conversation

clairernovotny commented Jul 26, 2024 • edited Loading

PR Classification

PR Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelverhagen Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelverhagen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdembayar commented Jul 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmylewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clairernovotny commented Jul 26, 2024 •

edited

Loading

joelverhagen Jul 29, 2024 •

edited

Loading