Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support @include/include directive for spaces included path. Fix #2485 #2780

Merged
merged 3 commits into from
Jan 20, 2020

Conversation

cosmo0920
Copy link
Contributor

Based on @repeatedly's patch:
#2485 (comment)

Signed-off-by: Hiroshi Hatake hatake@clear-code.com

Which issue(s) this PR fixes:
Fixes #2485

What this PR does / why we need it:
Support @include/include directive for spaces included path.

Docs Changes:
Needed?
Release Note:
Same as title.

@cosmo0920 cosmo0920 self-assigned this Jan 17, 2020
@cosmo0920
Copy link
Contributor Author

Umm..., tests which are affected this changes are failed...

Based on @repeatedly's patch:
#2485 (comment)

Signed-off-by: Hiroshi Hatake <hatake@clear-code.com>
@cosmo0920 cosmo0920 force-pushed the v1_config-include-directive-with-spaces-path branch 4 times, most recently from 0c5f6ba to 74cb033 Compare January 18, 2020 05:36
Because raw URI.encode_www_form_components methods overescape for passed string in this case.
We just need to handle URI with spaces as valid URI.
TO acheive this, we just replace spaces with '+' before executing URI.parse.

Signed-off-by: Hiroshi Hatake <hatake@clear-code.com>
@cosmo0920 cosmo0920 force-pushed the v1_config-include-directive-with-spaces-path branch from 74cb033 to a5fa046 Compare January 18, 2020 05:39
@cosmo0920
Copy link
Contributor Author

I used uri.gsub(/ /, '+') instead of URI.encode_www_form_component(uri)/URI.encode(uri).
Because we need to handle spaces in URI.parse(uri).

@ganmacs
Copy link
Member

ganmacs commented Jan 20, 2020

Because we need to handle spaces in URI.parse(uri).

Both methods(URI.encode_www_form_component(uri)/URI.encode(uri)) can replace space with +. Are there any reasons not to use them? I think using them express intentional clearly (it takes me a little time to get why + is used as the replaced char).

irb(main):003:0> URI.encode('/test with ')
=> "/test%20with%20"
irb(main):004:0> URI.encode_www_form_component('/test with ')
=> "%2Ftest+with+"

@cosmo0920
Copy link
Contributor Author

cosmo0920 commented Jan 20, 2020

Both methods(URI.encode_www_form_component(uri)/URI.encode(uri)) can replace space with +. Are there any reasons not to use them? I think using them express intentional clearly (it takes me a little time to get why + is used as the replaced char).

URI.encode_www_form_components will escape drive schema like as C:.

irb>  URI.encode_www_form_component('C:/test with ')
=> "C%3A%2Ftest+with+"

URI.parse does not handle "C%3A%2F" as a schema.

This causes unintentional behavior on this clause:

https://github.com/fluent/fluentd/blob/master/lib/fluent/config/v1_parser.rb#L151

URI.encode has right behavior for this PR but this method is marked as obsoleted:
https://docs.ruby-lang.org/ja/latest/method/URI/s/encode.html
Should we use this method as in the past?

Copy link
Member

@ganmacs ganmacs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

URI.parse does not handle "C%3A%2F" as a schema.

URI.encode has right behavior for this PR but this method is marked as obsoleted:
https://docs.ruby-lang.org/ja/latest/method/URI/s/encode.html

I see...

OK, then 👍

lib/fluent/config/v1_parser.rb Outdated Show resolved Hide resolved
Signed-off-by: Hiroshi Hatake <hatake@clear-code.com>
@cosmo0920 cosmo0920 force-pushed the v1_config-include-directive-with-spaces-path branch from bba1a63 to cd67858 Compare January 20, 2020 02:56
@cosmo0920
Copy link
Contributor Author

@repeatedly ?

@cosmo0920
Copy link
Contributor Author

I'll merge this PR after finishing CI.

@cosmo0920 cosmo0920 merged commit 9632daf into master Jan 20, 2020
@cosmo0920 cosmo0920 deleted the v1_config-include-directive-with-spaces-path branch January 20, 2020 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

include directive does not support Windows directory with spaces
3 participants