fix(path): correctly encode delimiters and relative paths #108

luqven · 2021-04-06T13:04:35Z

Do not merge until rebased on #103 and #104

Description

This PR adds a sanitize_path method to path.rb, related tests, and build improvements.

Tests

Using gem 'minitest-reporters', miniTest output logs now behave similarly to rSpec's, with color formatting and better diffing.

Test suite logging example:

SrcsetTest::SrcsetMinMaxWidths
  test_srcset_generates_width_pairs                               PASS (0.00s)
  test_only_max                                                   PASS (0.00s)
  test_srcset_pair_values                                         PASS (0.00s)
  test_negative_max_emits_error                                   PASS (0.00s)
...

Finished in 0.03846s
91 tests, 620 assertions, 0 failures, 0 errors, 0 skips

Diffing example:

test_resetting_defaults                                         FAIL (0.00s)
        --- expected
        +++ actual
        @@ -1 +1 @@
        -"https://demo.imgix.net/images/demo.png?w=200&s=da421114ca238d1f4a927b889f67c34e"
        +"https://demo.imgix.netfalse?w=200&s=59a2133b87f1b07792f54601d0988c77"
        /Users/Luis/Documents/imgix/libraries/imgix-rb/test/units/path_test.rb:23:in `test_resetting_defaults'

Tests can also be run in VSCode as the default test task, SHIFT + COMMAND + T in my system for example.

Up for discussion

One of the ways this code base could be improved would be to re-name a lot of the methods. This library exposes a to_url method instead of a build_url method like some of the others for example. Moreover, the logic for path encoding is separate from the rest of the library, but that's not how the projects are organized elsewhere.

commit-lint · 2021-04-06T13:04:38Z

Chore

vscode: add test task (034f461)
minitest: add reporters gem (a9dfa33)
vscode: make test task default (2954cab)
utf8: remove unused helper fn (0c0c6ff)
sanitizePath: make method private and call it in to_url (bb4674e)
encodeURI: refactor and add comments for clarity (efc732f)

Tests

minitest: use reporters gem to create human readbale output (d0dd15e)
path: add delimiter & relative path tests (7b7bce8)
unicode: ensure unicode chars are encoded (6d416df)
change reserved delimiters to resolvable url (40ec089)
remove comment and duplicated import (3fd79ce)

Bug Fixes

path: correctly encode reserved chars (701020f)

Styles

remove comment (6e9f72b)

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

sherwinski

LGTM! And I'm glad to see some much needed improvements to the test runner.
To try and address some of your other points and questions:

This library exposes a to_url method instead of a build_url method like some of the others for example.

Unfortunately, we can't do much about this inconsistency since it is a part of the library's public API. But for anything else (like helper and private methods), feel free to refactor them to make the naming consistent across the kit.

Moreover, the logic for path encoding is separate from the rest of the library, but that's not how the projects are organized elsewhere.

Fun fact, imgix-rb was the first of the imgix SDK libraries. As a result, it's a lot unlike the other projects in most cases (Client -> path -> to_url). Like I just mentioned, we're open to reorganizing things so long as it's unnoticeable to end users.

sherwinski · 2021-04-08T23:15:50Z

lib/imgix/path.rb

    def to_url(opts = {})
+      @path = sanitize_path(@path)


Will this cause multiple encoding issues if to_url is called multiple times on the same Path? Perhaps a different ivar name and ||= sanitize_path(@path) could be used instead.

Great suggestion 💯 , I'll try this out locally and push the change if all looks good.

sherwinski · 2021-04-08T23:37:02Z

Oh also another note, #105 #103 introduced some performance benchmarks which I think we should extend and take advantage of. For example, you can try running all benchmarks in main and compare it to same benchmarks in this library to see if there are any noticeable performance regressions/improvements.
Eventually I'd like have this built into default tests but for now we can stick to running them manually.

stevehodgkiss · 2021-04-09T03:53:28Z

lib/imgix/path.rb

+        # otherwise, encode only specific characters
+        return encode_URI(path)
+      end
+    end


In the Variant PR, which shifts a lot of this escaping/encoding work to application boot, this would ideally be done in the Variant initializer: https://github.com/imgix/imgix-rb/pull/104/files#diff-f7f33d40dff4d8a40100ac8a68f7fab2e082a98a683b678c8ee04c1bc930102aR8

Haven't had a chance to dig into #104 too much, but loved what I saw so far. Will definitely keep this in mind.

Thanks for the suggestion @stevehodgkiss. I could not find any encoding issues when to_url is called multiple times on the same Path, but I’m sure there will be edge cases that I haven’t thought of. Also, I think the ivar for sanitized_path creates a more predictable experience for the user.

I’ve implemented a solution based on your idea:

imgix-rb/lib/imgix/path.rb

Lines 17 to 27 in bd5bdec

def to_url(opts = {})

sanitized_path ||= sanitize_path(@path)

prev_options = @options.dup

@options.merge!(opts)

current_path_and_params = path_and_params(sanitized_path)

url = @prefix + current_path_and_params

if @secure_url_token

url += (has_query? ? "&" : "?") + "s=#{signature(current_path_and_params)}"

end

Before these changes, @path was being modified in a way that was not explicit. There may be a case in the future where we want an @sanitized_path var or a santize_path! func, but for the purposes of this PR that is out of scope.

There are a lot of improvements needed in this file and the library throughout, but I felt these changes have to least potential for producing breaking changes.

Looks good! I take back my previous comment about "this would ideally be done in the Variant initializer". This can't be done there since it's about the Path itself, not the Variant.

luqven · 2021-04-13T12:52:57Z

My apols, blew up the commit history locally without realizing. Never push before your first cup of coffee ☕ 😴

ericdeansanchez

At minimum, I think we need to have the tests for those other two paths I mentioned, other than that, looks good 👍

lib/imgix/path.rb

test/units/path_test.rb

test/test_helper.rb

stevehodgkiss · 2021-04-17T14:20:36Z

lib/imgix/path.rb

    end

    def to_url(opts = {})
+      sanitized_path ||= sanitize_path(@path)
      prev_options = @options.dup
      @options.merge!(opts)


Not related to this PR, but it would be great if this state mutation could be removed by passing state in method calls, like you're doing with sanitized_path.

lib/imgix/path.rb

Co-authored-by: Steve Hodgkiss <steve@hodgkiss.me>

ericdeansanchez

LGTM 👍

sherwinski

👍

luqven self-assigned this Apr 6, 2021

luqven force-pushed the luis/pathEncoding branch from 900975e to e2f6c11 Compare April 6, 2021 19:33

luqven marked this pull request as ready for review April 8, 2021 18:09

luqven requested a review from a team as a code owner April 8, 2021 18:09

sherwinski mentioned this pull request Apr 8, 2021

Optimise Imgix::Path#to_url by not generating the escaped query string twice #103

Closed

sherwinski reviewed Apr 8, 2021

View reviewed changes

stevehodgkiss reviewed Apr 9, 2021

View reviewed changes

luqven added 12 commits April 9, 2021 09:12

chore(vscode): add test task

034f461

chore(minitest): add reporters gem

a9dfa33

test(minitest): use reporters gem to create human readbale output

d0dd15e

chore(vscode): make test task default

2954cab

test(path): add delimiter & relative path tests

7b7bce8

fix(path): correctly encode reserved chars

701020f

chore(utf8): remove unused helper fn

0c0c6ff

test(unicode): ensure unicode chars are encoded

6d416df

chore(sanitizePath): make method private and call it in to_url

bb4674e

style: remove comment

6e9f72b

test(to_url): consecutive calls have same result

b912bfe

chore(to_url): add path ivar & params to signature method

bd5bdec

luqven force-pushed the luis/pathEncoding branch from 7315506 to bd5bdec Compare April 13, 2021 12:51

luqven requested a review from ericdeansanchez April 13, 2021 18:26

ericdeansanchez requested changes Apr 15, 2021

View reviewed changes

lib/imgix/path.rb Outdated Show resolved Hide resolved

test/units/path_test.rb Outdated Show resolved Hide resolved

test/units/path_test.rb Show resolved Hide resolved

test/test_helper.rb Outdated Show resolved Hide resolved

luqven added 3 commits April 16, 2021 12:56

chore(encodeURI): refactor and add comments for clarity

efc732f

test: change reserved delimiters to resolvable url

40ec089

test: remove comment and duplicated import

3fd79ce

stevehodgkiss reviewed Apr 17, 2021

View reviewed changes

lib/imgix/path.rb Outdated Show resolved Hide resolved

luqven and others added 2 commits April 19, 2021 09:48

test(to_url): add unicode and delimiter tests

6fa3f2c

chore(to_url): remove double pipe

da3711c

Co-authored-by: Steve Hodgkiss <steve@hodgkiss.me>

luqven requested a review from ericdeansanchez April 19, 2021 14:17

ericdeansanchez approved these changes Apr 20, 2021

View reviewed changes

ericdeansanchez requested a review from sherwinski April 20, 2021 22:26

sherwinski approved these changes Apr 21, 2021

View reviewed changes

luqven merged commit 363ccc2 into main Apr 22, 2021

luqven deleted the luis/pathEncoding branch April 22, 2021 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(path): correctly encode delimiters and relative paths #108

fix(path): correctly encode delimiters and relative paths #108

luqven commented Apr 6, 2021 •

edited

Loading

commit-lint bot commented Apr 6, 2021 •

edited

Loading

sherwinski left a comment

sherwinski Apr 8, 2021

stevehodgkiss Apr 9, 2021

luqven Apr 9, 2021

sherwinski commented Apr 8, 2021

stevehodgkiss Apr 9, 2021

luqven Apr 9, 2021

luqven Apr 13, 2021

stevehodgkiss Apr 17, 2021

luqven commented Apr 13, 2021

ericdeansanchez left a comment

stevehodgkiss Apr 17, 2021

ericdeansanchez left a comment

sherwinski left a comment

	def to_url(opts = {})
	sanitized_path \|\|= sanitize_path(@path)
	prev_options = @options.dup
	@options.merge!(opts)

	current_path_and_params = path_and_params(sanitized_path)
	url = @prefix + current_path_and_params

	if @secure_url_token
	url += (has_query? ? "&" : "?") + "s=#{signature(current_path_and_params)}"
	end

fix(path): correctly encode delimiters and relative paths #108

fix(path): correctly encode delimiters and relative paths #108

Conversation

luqven commented Apr 6, 2021 • edited Loading

Description

Tests

Up for discussion

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

Chore

Tests

Bug Fixes

Styles

Contributors

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

sherwinski commented Apr 8, 2021

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 13, 2021

ericdeansanchez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericdeansanchez left a comment

Choose a reason for hiding this comment

sherwinski left a comment

Choose a reason for hiding this comment

luqven commented Apr 6, 2021 •

edited

Loading

commit-lint bot commented Apr 6, 2021 •

edited

Loading