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

Encoding '&' in influx query url to be handled by influx #43

Merged
merged 2 commits into from
Jul 28, 2018

Conversation

nikhilbelchada
Copy link
Contributor

When having '&' in influx query no result was returned for which encoding need to be encode_www_form

@coveralls
Copy link

coveralls commented Jul 27, 2018

Coverage Status

Coverage increased (+0.004%) to 98.538% when pulling fb18abd on nikhilbelchada:master into d909369 on mneudert:master.

false -> "#{URI.encode(value)}"
end

"#{url}#{glue}#{key}=#{encoded_value}"
Copy link
Owner

Choose a reason for hiding this comment

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

Would it be sufficient to just always use URI.encode_www_form(value)? There shouldn't be any case that relies on that previously broken behaviour, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is one test case which is failing, hence thought to have specific condition

Copy link
Owner

Choose a reason for hiding this comment

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

I think the change might be URI.encode/1 using + to encode a space and URI.encode_www_form/1 using %20. But looking at the InfluxDB documentation always using curl --url-encode and therefore percent encoding I think the test is at fault and not the fix.

Will look at some more official clients (like the official CLI client) how they handle the encoding and then adjust this fix accordingly.

Copy link
Owner

Choose a reason for hiding this comment

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

Can you take a look at ae92a6a to see if that solves your problem?

From what I tested the real solution would be to use URI.encode_query/1 instead of the other variants. That constructs the most likely expected encoding for spaces as %20 is usually only used for path encoding while + is used for parameter value encoding. I modified a test case to ensure both question marks and ampersands are encoded.

If that works for you I would update your PR and merge (and asap release a new version including the fix). I just did not want to mess around on your master branch without any notice :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect!!!
It worked

@mneudert mneudert merged commit 6bd91d2 into mneudert:master Jul 28, 2018
@mneudert
Copy link
Owner

❤️ 💛 💙 💚 💜

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.

3 participants