imgix / imgix-java

A Java client library for generating URLs with imgix
https://www.imgix.com
BSD 2-Clause "Simplified" License
19 stars 8 forks source link

fix(sanitizePath): ensure unicode chars are encoded as expected #70

Closed luqven closed 3 years ago

luqven commented 3 years ago

Description

This PR adds test and updates the sanitizePath method to ensure Unicode characters get encoded when needed and as expected. It builds off of the imgix-go implementation for encoded URLs and creates a new isASCIIEncoded method that validates ASCII encoding and a few more custom imgix encoding rules.

commit-lint[bot] commented 3 years ago

Tests

Bug Fixes

Chore

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 commented 3 years ago

Latest build fails because of test testHelperBuildSignedURLWithWebProxyWithEncoding() https://github.com/imgix/imgix-java/blob/c8e9655cdbcc7f0f937fce0f7fa62e740f3ba70a/src/test/java/com/imgix/test/TestAll.java#L173-L183

Maybe we need to allow encoded proxy paths through given this was established behavior? Might need to undo https://github.com/imgix/imgix-java/pull/70/commits/0c4f0d05584029326d06e163432503dd133c0eb7

sherwinski commented 3 years ago

Latest build fails because of test testHelperBuildSignedURLWithWebProxyWithEncoding()

https://github.com/imgix/imgix-java/blob/c8e9655cdbcc7f0f937fce0f7fa62e740f3ba70a/src/test/java/com/imgix/test/TestAll.java#L173-L183

Maybe we need to allow encoded proxy paths through given this was established behavior? Might need to undo 0c4f0d0

Yeah I think you're right, we should keep this in order to prevent a breaking change.

luqven commented 3 years ago

@sherwinski @ericdeansanchez double checking we're good to merge this one in

sherwinski commented 3 years ago

@sherwinski @ericdeansanchez double checking we're good to merge this one in

All good from my end 👍

luqven commented 3 years ago

Had to force push since I forgot to rebase on luis/pathEncodingFix before it got merged in to main. Apols.