search

fog / fog-aliyun

Fog provider for aliyun
MIT License
37 stars 24 forks source link

Use `URI.encode_www_form_component` instead of `URI.encode` #157

Open johha opened 2 years ago

johha commented 2 years ago

Replace URI.encode with URI.encode_www_form_component to make gem compatible to Ruby version to 3.x.

The integration tests passed successfully however there might be some specials case as previously only certain characters ('/[^!*\'()\;?:@#&%=+$,{}[]<>" '`) where encoded. Therefore a thorough review is needed before merging this PR. Related to #156.

johha commented 2 years ago

The PR does not seem to fix the problem, there are still errors.

philippthun commented 2 years ago

I've tested different "encoders" and compared them with URI.encode(some_string, '/[^!*\'()\;?:@#&%=+$,{}[]<>`" '). For testing purposes I used a string containing all ascii characters plus space: "!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnopqrstuvwxyz{|}~".

I've encoded this test string with the currently used implementation, with URI.encode_www_form_component, with CGI.escape and with Addressable::URI.encode_component.

The exact same result can be achieved by replacing URI.encode(some_string, '/[^!*\'()\;?:@#&%=+$,{}[]<>`" ') with Addressable::URI.encode_component(some_string, Addressable::URI::CharacterClasses::UNRESERVED + '|').

Question: Is there a reason why the pipe symbol (|) is handled differently compared to other symbols? Or was it just forgotten in the list of characters passed to URI.encode?

jiangytcn commented 2 years ago

The PR does not seem to fix the problem, there are still errors.

@johha @philippthun still having errors now ?

philippthun commented 2 years ago

@jiangytcn We've opened a new PR (#158) with which we could successfully run our deploy pipeline on AliCloud (still only a development environment with a limited set of components).

We can close this PR and move on to the new one.

It would still be good to get an answer for my question raised above about the pipe symbol. I guess we could replace...

Addressable::URI.encode_component(some_string, Addressable::URI::CharacterClasses::UNRESERVED + '|')

... with...

Addressable::URI.encode_component(some_string, Addressable::URI::CharacterClasses::UNRESERVED)

..., or is there a reason for the special handling of |?

jiangytcn commented 2 years ago

@jiangytcn We've opened a new PR (#158) with which we could successfully run our deploy pipeline on AliCloud (still only a development environment with a limited set of components).

We can close this PR and move on to the new one.

It would still be good to get an answer for my question raised above about the pipe symbol. I guess we could replace...

Addressable::URI.encode_component(some_string, Addressable::URI::CharacterClasses::UNRESERVED + '|')

... with...

Addressable::URI.encode_component(some_string, Addressable::URI::CharacterClasses::UNRESERVED)

..., or is there a reason for the special handling of |?

I would ask @xiaozhu36 to answer that, do you know the reasons of the | char ?

stephanme commented 2 years ago

I suggest to close this PR. The change is included in #158.