fix(BuildUrl): address path encoding lapses #40

luqven · 2021-04-02T14:39:35Z

Description

This PR includes the following changes:

fixed ms-dotnettools lint issues
adds NUnitTestAdapter
adds vscode dotnet-test test settings
adds more Base64 encoding tests
creates the SanitizePath, UrlEncode, and Base64Encode methods to more closely resemble ur-building logic in imgix-js and address some path encoding issues.

commit-lint · 2021-04-02T14:39:38Z

Styles

UrlBuilder: fix ms-dotnettools lint issues (707dc70)

Tests

NUnit: add NUnitTestAdapter (228e2fb)
UrlBuilder: add more Base64 encoding tests (83826e4)
UrlBuilder: change UrlBuilder test name (b83f672)

Chore

VSCODE: add dotnet-test test settings (8528fc6)
remove string const (7675772)
CheckProxy: fix typo (f3d2fab)
remvoe vscode ext settings file (d35e965)

Bug Fixes

BuildURl: address path encoding lapses (8068ee0)

Code Refactoring

BuildParams: add SignParams and comments (ff4bc98)

Contributors

luqven

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

@Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
@Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
@Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
@Commit-Lint merge disable will desactivate merge dependabot PR
@Commit-Lint review will approve dependabot PR
@Commit-Lint stop review will stop approve dependabot PR

luqven · 2021-04-05T13:39:51Z

src/Imgix/UrlBuilder.cs

+            String encodedHTTP = "http%3A%2F%2F";
+            String encodedHTTPS = "https%3A%2F%2F";

-            if (path.StartsWith("http"))
+            String encodedHTTPLower = "http%3a%2f%2f";
+            String encodedHTTPSLower = "https%3a%ff%2f";


We have to leave pre-encoded path support in since the library has, in the past, supported it. As evidenced by UrlBuilderProperlySignsFullyQualifiedUrls UrlBuilderProperlySignsFullyQualifiedUrlsWithParameters tests.

sherwinski

Overall looks really good, most of my concerns were around style and readability which we can discuss 👍

sherwinski · 2021-04-07T16:10:34Z

src/Imgix/UrlBuilder.cs

+            String scheme = UseHttps ? "https" : "http";
+            path = SanitizePath(path);
+
+            var qs = BuildParams(parameters);


style nit: I think this would be more clear fully written out as queryString

sherwinski · 2021-04-07T18:28:17Z

src/Imgix/UrlBuilder.cs

            {
-                path = WebUtility.UrlEncode(path);
+                var hashString = String.Format("{0}/{1}{2}", _signKey, path, localParams.Any() ? "?" + qs : String.Empty);


style: can we break out the localParams.Any() ? "?" + qs : String.Empty into its own line?

Suggested change

var hashString = String.Format("{0}/{1}{2}", _signKey, path, localParams.Any() ? "?" + qs : String.Empty);

var queryString = localParams.Any() ? "?" + qs : String.Empty;

var hashString = String.Format("{0}/{1}{2}", _signKey, path, queryString);

Agree with this. We can do away with a lot of this ternary logic by making BuildParams better and using a SignParams helper, as I mention above.

sherwinski · 2021-04-07T19:39:27Z

src/Imgix/UrlBuilder.cs

            }
+            return String.Format("{0}://{1}/{2}{3}", scheme, Domain, path, localParams.Any() ? "?" + BuildParams(localParams) : String.Empty);


Same comment as above.

Suggested change

return String.Format("{0}://{1}/{2}{3}", scheme, Domain, path, localParams.Any() ? "?" + BuildParams(localParams) : String.Empty);

var queryString = localParams.Any() ? "?" + BuildParams(localParams) : String.Empty;

return String.Format("{0}://{1}/{2}{3}", scheme, Domain, path, queryString);

sherwinski · 2021-04-07T19:43:51Z

src/Imgix/UrlBuilder.cs

+        * Encodes the `path` and `paramater` arguments to the UTF8 
+        * scheme and returns a formatted string that conforms to the 
+        * "{scheme}://{domain}/{path}{params}" format.
+        */
        public String BuildUrl(String path, Dictionary<String, String> parameters)


I had a bit of difficulty parsing out the steps of this method despite knowing what it's supposed to do, specifically why qs was used over localParams at times (and vice versa). I think stylistically we can make things clearer for other contributors by either 1) reworking the variable names to be a bit more descriptive (I'll mull this one over) and/or 2) adding a comment describing what the purpose of the if block on L82 is for.

This makes sense. I think the best thing to do here is to break this up similarly to js-core, where sanitizePath, buildParams, and signParams are all extracted out into methods. Makes reading this a lot easier and also allows us remove references to qs or queryString and just say finalParams.

sherwinski · 2021-04-07T19:50:56Z

src/Imgix/UrlBuilder.cs

+        {
+            String path = p;
+            Dictionary<String, Boolean> status = new Dictionary<String, Boolean>();
+            path.Replace("^/", "");


out of curiosity, you already trim the leading and trailing slashes in SanitizePath so do we still need to this line?

💯 good catch, not needed here.

sherwinski · 2021-04-07T23:14:35Z

src/Imgix/UrlBuilder.cs

+            if (!String.IsNullOrEmpty(_signKey))
+            {
+                var hashString = String.Format("{0}/{1}{2}", _signKey, path, localParams.Any() ? "?" + qs : String.Empty);
+                localParams.Add("s", HashString(hashString));


I know it's out of scope for this PR, but I also think we should rename the HashString method to more closely match js-core

💯 , for now going to call it inside a SignParams helper to make it clearer what this is doing.

luqven · 2021-04-08T15:40:11Z

Overall looks really good, most of my concerns were around style and readability which we can discuss 👍

Addressed all of this concerns with latest 2 commits.

Improved syntax in BuildUrl by refactoring some of the logic into BuildParams and a new SignParams helper.
Removed a lot of ternary operators thanks to the above
Added comments for hover-help and generally improved readability.
Fixed a mismatched test name

sherwinski

Aside from one question this LGTM! Everything looks a lot cleaner now 👌

.vscode/settings.json

luqven added 5 commits April 1, 2021 13:21

style(UrlBuilder): fix ms-dotnettools lint issues

707dc70

test(NUnit): add NUnitTestAdapter

228e2fb

chore(VSCODE): add dotnet-test test settings

8528fc6

test(UrlBuilder): add more Base64 encoding tests

83826e4

fix(BuildURl): address path encoding lapses

8068ee0

luqven self-assigned this Apr 2, 2021

chore: remove string const

7675772

luqven commented Apr 5, 2021

View reviewed changes

luqven marked this pull request as ready for review April 5, 2021 13:57

luqven requested a review from a team as a code owner April 5, 2021 13:57

sherwinski suggested changes Apr 7, 2021

View reviewed changes

sherwinski reviewed Apr 7, 2021

View reviewed changes

luqven added 2 commits April 8, 2021 11:34

refactor(BuildParams): add SignParams and comments

ff4bc98

test(UrlBuilder): change UrlBuilder test name

b83f672

luqven requested a review from sherwinski April 9, 2021 02:53

chore(CheckProxy): fix typo

f3d2fab

sherwinski approved these changes Apr 9, 2021

View reviewed changes

.vscode/settings.json Outdated Show resolved Hide resolved

chore: remvoe vscode ext settings file

d35e965

luqven merged commit e12c54b into main Apr 12, 2021

luqven deleted the luis/pathEncoding branch April 12, 2021 20:43

luqven mentioned this pull request Apr 13, 2021

chore(release): 3.2.2 #41

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(BuildUrl): address path encoding lapses #40

fix(BuildUrl): address path encoding lapses #40

luqven commented Apr 2, 2021 •

edited

Loading

commit-lint bot commented Apr 2, 2021 •

edited

Loading

luqven Apr 5, 2021

sherwinski left a comment

sherwinski Apr 7, 2021

sherwinski Apr 7, 2021

luqven Apr 8, 2021

sherwinski Apr 7, 2021

sherwinski Apr 7, 2021

luqven Apr 8, 2021

sherwinski Apr 7, 2021

luqven Apr 8, 2021

sherwinski Apr 7, 2021

luqven Apr 8, 2021

luqven commented Apr 8, 2021

sherwinski left a comment

	var hashString = String.Format("{0}/{1}{2}", _signKey, path, localParams.Any() ? "?" + qs : String.Empty);
	var queryString = localParams.Any() ? "?" + qs : String.Empty;
	var hashString = String.Format("{0}/{1}{2}", _signKey, path, queryString);

		}
		return String.Format("{0}://{1}/{2}{3}", scheme, Domain, path, localParams.Any() ? "?" + BuildParams(localParams) : String.Empty);

fix(BuildUrl): address path encoding lapses #40

fix(BuildUrl): address path encoding lapses #40

Conversation

luqven commented Apr 2, 2021 • edited Loading

Description

commit-lint bot commented Apr 2, 2021 • edited Loading

Styles

Tests

Chore

Bug Fixes

Code Refactoring

Contributors

Choose a reason for hiding this comment

sherwinski left a 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

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

luqven commented Apr 8, 2021

sherwinski left a comment

Choose a reason for hiding this comment

luqven commented Apr 2, 2021 •

edited

Loading

commit-lint bot commented Apr 2, 2021 •

edited

Loading