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

[http] Allow pre-escaped URLs #12350

Merged
merged 1 commit into from
May 29, 2022
Merged

Conversation

jamesmelville
Copy link
Contributor

@jamesmelville jamesmelville commented Feb 22, 2022

Whilst it's helpful that the binding automatically escapes characters in URLs for us, there are some scenarios where it doesn't escape characters which need to be escaped for a specific API call. Because the binding escapes the URL for us, any attempts to include escaped characters ourselves result in the % sign also being included.

An example is an API I wanted to use which expects a query in this format:
https://example.com/api/query?q=deleted%3Dfalse%26creatorId%3Djames&sort=updatedTime+desc&count=1

%3D is =, and %25 is &, these are both valid characters in a URL (see other examples in the URL), so reverting them to their unescaped values results in the API request being rejected.

Using the URL with the HTTP binding results in an error because of the use of java's string.format to include the date in the URL:
Creating request for 'https://example.com/api/query?q=deleted%3Dfalse%26creatorId%3Djames&sort=updatedTime+desc&count=1' failed: Conversion = 'D'

Including a double percentage sign results in the wrong URL being called and the API returning 400, as the remaining % is also encoded:
Requesting 'https://example.com/api/query?q=deleted%253Dfalse%2526creatorId%253Djames&sort=updatedTime+desc&count=1' (method='GET', content='null') failed: 400 null

I'm not the only one who needs a similar capability: https://community.openhab.org/t/oh3-http-binding-in-url/133503

I wanted to propose this PR to allow those who need to to escape the URL themselves. It's a fairly simple change to add an extra parameter on the channel to flag that the URL formed from the baseURL and it's extensions is already escaped.

Signed-off-by: James Melville <jamesmelville@gmail.com>
@jamesmelville jamesmelville requested a review from a team as a code owner February 22, 2022 20:56
@jamesmelville
Copy link
Contributor Author

Zipped Jar file of built code org.openhab.binding.http-3.3.0-SNAPSHOT.zip

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/oh3-http-binding-in-url/133503/2

@wborn wborn added the enhancement An enhancement or new feature for an existing add-on label Mar 8, 2022
Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

Only few remarks about updated documentation.

URLs are properly escaped by the binding itself before the request is sent.
Using escaped strings in URL parameters may lead to problems with the formatting (see below).

In certain scenarios you may need to manually escape your URL, for example if you need to include an escaped `=` (`%3D`) in this scenario include `%%3D` in the URL to preserve the `%` during formatting, and set the parameter `escapedUrl` to true on the channel.
Copy link
Contributor

@lolodomo lolodomo May 8, 2022

Choose a reason for hiding this comment

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

... example if you need to include an escaped = (%3D).
In this scenario, include ...

So . and , added + a return to a new line (one sentence per line recommended).


| parameter | optional | default | description |
|-------------------------|----------|-------------|-------------|
| `stateExtension` | yes | - | Appended to the `baseURL` for requesting states. |
| `commandExtension` | yes | - | Appended to the `baseURL` for sending commands. If empty, same as `stateExtension`. |
| `stateTransformation ` | yes | - | One or more transformation applied to received values before updating channel. |
| `commandTransformation` | yes | - | One or more transformation applied to channel value before sending to a remote. |
| `escapedUrl` | yes | - | This specifies whether the URL is already escaped. |
Copy link
Contributor

@lolodomo lolodomo May 8, 2022

Choose a reason for hiding this comment

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

You defined a default: false

@@ -23,7 +23,7 @@ It can be extended with different channels.
| `commandMethod` | no | GET | Method used for sending commands: `GET`, `PUT`, `POST`. |
| `contentType` | yes | - | MIME content-type of the command requests. Only used for `PUT` and `POST`. |
| `encoding` | yes | - | Encoding to be used if no encoding is found in responses (advanced parameter). |
| `headers` | yes | - | Additional headers that are sent along with the request. Format is "header=value". Multiple values can be stored as `headers="key1=value1", "key2=value2", "key3=value3",`|
| `headers` | yes | - | Additional headers that are sent along with the request. Format is "header=value". Multiple values can be stored as `headers="key1=value1", "key2=value2", "key3=value3",`. When using text based configuration include at minimum 2 headers to avoid parsing errors.|
Copy link
Contributor

@lolodomo lolodomo May 8, 2022

Choose a reason for hiding this comment

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

This is something fully unrelated to your enhancement. I would suggest you exclude from this PR and you either propose a simple update of the doc for this or even better you could open an issue.

@lolodomo
Copy link
Contributor

@jamesmelville : did you see my review comments?

@lolodomo
Copy link
Contributor

As there is no feedback but my comments were relatively minor and only about documentation, I propose to merge.

@lolodomo lolodomo merged commit 93ecb5d into openhab:main May 29, 2022
@lolodomo lolodomo added this to the 3.3 milestone May 29, 2022
leifbladt pushed a commit to leifbladt/openhab-addons that referenced this pull request Oct 15, 2022
Signed-off-by: James Melville <jamesmelville@gmail.com>
andan67 pushed a commit to andan67/openhab-addons that referenced this pull request Nov 6, 2022
Signed-off-by: James Melville <jamesmelville@gmail.com>
andrasU pushed a commit to andrasU/openhab-addons that referenced this pull request Nov 12, 2022
Signed-off-by: James Melville <jamesmelville@gmail.com>
Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>
psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Feb 23, 2023
Signed-off-by: James Melville <jamesmelville@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting feedback enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants