Document SHOUTRRR_PARAMS: Your SHOUTRRR integration for generic webhooks only sends a string in the body when the default is application/json #243

New Issue

@qdm12 commented on GitHub (Jun 12, 2023):

First of all, sorry for the many hours wasted, and this is definitely a nifty horrible bug and missing documentation.
Second, sorry for the massive delay, I was moving continents, and didn't have time to maintain much my projects.

See #411 as far as I have seen, sadly, I don't think you can configure a content type per service if you cannot do it using SHOUTRRR_PARAMS'; I even dug quite a bit in the Go shoutrrr library. So I'm thinking now to move everything to JSON, such that the default generic content type would work, what do you think?

@qdm12 commented on GitHub (Jun 12, 2023): First of all, sorry for the many hours wasted, and this is definitely a nifty horrible bug and missing documentation. Second, sorry for the massive delay, I was moving continents, and didn't have time to maintain much my projects. See #411 as far as I have seen, sadly, I don't think you can configure a content type per service if you cannot do it using `SHOUTRRR_PARAMS`'; I even dug quite a bit in the Go shoutrrr library. So I'm thinking now to move everything to JSON, such that the default generic content type would work, what do you think?

saavagebueno commented

@qdm12 commented on GitHub (Jun 12, 2023):

@DennisGaida @HansBaumgaertner

Quoting from the PR adding documentation for SHOUTRRR_PARAMS which links to this issue:

I'm thinking of changing the content type to json for everything, since it's the default for the generic endpoint.
However, after some tests, it looks like changing it to application/json (without changing the message string send to shoutrrr) still works with Gotify, because its shoutrrr implementation handles the json encoding. Now with the generic endpoint, would your generic endpoint work with application/json as content-type? What should be the message sent to it?? Or does it simply refuses application/json? 🤔

Or should we just leave everything as is, let Shoutrrr handles its thing and keep the link from the readme SHOUTRRR_PARAMS variable documentation to this issue?

@qdm12 commented on GitHub (Jun 12, 2023): @DennisGaida @HansBaumgaertner Quoting from the PR adding documentation for `SHOUTRRR_PARAMS` which links to this issue: > I'm thinking of changing the content type to json for everything, since it's the default for the generic endpoint. However, after some tests, it looks like changing it to application/json (without changing the message string send to shoutrrr) still works with Gotify, because its shoutrrr implementation handles the json encoding. Now with the generic endpoint, would your generic endpoint work with `application/json` as content-type? What should be the message sent to it?? Or does it simply refuses application/json? 🤔 Or should we just leave everything as is, let Shoutrrr handles its thing and keep the link from the readme `SHOUTRRR_PARAMS` variable documentation to this issue?

saavagebueno commented

@DennisGaida commented on GitHub (Jun 12, 2023):

My application (Node-Red https://nodered.org/) would definitely work with application/json - this is what it expects. What doesn't work is sending raw test and saying "this is json" when it isn't - hence my issue.

I believe it would be totally fine if we'd document this cleanly - we are not here to fix / improve Shoutrrr. Sending JSON for everything sounds nice to me, but maybe that doesn't work with all services? Json should be the default anyways IMHO compared to plain text, but that may just be my opinion.

@DennisGaida commented on GitHub (Jun 12, 2023): My application (Node-Red https://nodered.org/) would definitely work with `application/json` - this is what it expects. What doesn't work is sending raw test and saying "this is json" when it isn't - hence my issue. I believe it would be totally fine if we'd document this cleanly - we are not here to fix / improve Shoutrrr. Sending JSON for everything sounds nice to me, but maybe that doesn't work with all services? Json should be the default anyways IMHO compared to plain text, but that may just be my opinion.

saavagebueno commented

@qdm12 commented on GitHub (Jun 12, 2023):

What should be the keys for the JSON message? Is this standardised, or is it specific to your service?
What plaintext do you get on your application? Just the message? Or also the title DDNS updater?

@qdm12 commented on GitHub (Jun 12, 2023): 1. What should be the keys for the JSON message? Is this standardised, or is it specific to your service? 2. What plaintext do you get on your application? Just the message? Or also the title `DDNS updater`?

saavagebueno commented

@DennisGaida commented on GitHub (Jun 12, 2023):

What should be the keys for the JSON message? Is this standardised, or is it specific to your service?

Check the shoutrrr documentation - it unfortunately is different per service.

Pushbullet uses "title"
Pushover uses "title, devices, priority"
... etc.

Node-Red really doesn't care what JSON data you throw at it. Add a title. Add a description. Add anything - you will have to deal with parsing everything anyways. All it wants is valid JSON if you say you send JSON 😉

What plaintext do you get on your application? Just the message? Or also the title DDNS updater?

Again: the raw message. So if I send "this a test" as content type text/plain this is what I receive. When I send { "title":"DDNS updater", "message":"this is a message"} as application/json I would receive exactly that raw JSON.

@DennisGaida commented on GitHub (Jun 12, 2023): > 1. What should be the keys for the JSON message? Is this standardised, or is it specific to your service? Check the shoutrrr documentation - it unfortunately is different per service. - Pushbullet uses "title" - Pushover uses "title, devices, priority" - ... etc. Node-Red really doesn't care what JSON data you throw at it. Add a title. Add a description. Add anything - you will have to deal with parsing everything anyways. All it wants is valid JSON if you say you send JSON 😉 > 2. What plaintext do you get on your application? Just the message? Or also the title `DDNS updater`? Again: the raw message. So if I send "this a test" as content type `text/plain` this is what I receive. When I send `{ "title":"DDNS updater", "message":"this is a message"}` as `application/json` I would receive exactly that raw JSON.

saavagebueno commented

@qdm12 commented on GitHub (Jun 12, 2023):

I created https://github.com/containrrr/shoutrrr/issues/363 to get some clarifications. I still tend to think the default on shoutrrr should be text/plain, and, especially important if not, we need a way to specify the content type per service.

@qdm12 commented on GitHub (Jun 12, 2023): I created https://github.com/containrrr/shoutrrr/issues/363 to get some clarifications. I still tend to think the default on shoutrrr should be `text/plain`, and, especially important if not, we need a way to specify the content type per service.

saavagebueno commented

@piksel commented on GitHub (Jun 13, 2023):

Sorry if I misunderstand something, but couldn't you just use:

SHOUTRRR_ADDRESSES="generic://<shoutrrr-webhook-endpoint-url>?template=json"

(for a json-encoded object, with title and message) or

SHOUTRRR_ADDRESSES="generic://<shoutrrr-webhook-endpoint-url>?contenttype=text/plain"

(for just the message as a string)

The PARAMS are used to set the title because we didn't add the title as a standardized field until later in shoutrrrrs lifetime, so it's a bit of a hack unfortunately. Everything you can put in PARAMS, you could just as well put in the URL to begin with.
Everything in the params will be sent to all notification services, so it is probably not a good idea to expose it to the end users, since title is the only thing you can safely pass to all services without them complaining about unknown keys.

@piksel commented on GitHub (Jun 13, 2023): Sorry if I misunderstand something, but couldn't you just use: ``` SHOUTRRR_ADDRESSES="generic://<shoutrrr-webhook-endpoint-url>?template=json" ``` (for a json-encoded object, with `title` and `message`) or ``` SHOUTRRR_ADDRESSES="generic://<shoutrrr-webhook-endpoint-url>?contenttype=text/plain" ``` (for just the message as a string) The `PARAMS` are used to set the title because we didn't add the `title` as a standardized field until later in shoutrrrrs lifetime, so it's a bit of a hack unfortunately. Everything you can put in `PARAMS`, you could just as well put in the URL to begin with. Everything in the params will be sent to all notification services, so it is probably not a good idea to expose it to the end users, since `title` is the only thing you can safely pass to all services without them complaining about unknown keys.

saavagebueno commented

@qdm12 commented on GitHub (Jun 13, 2023):

@piksel thanks so much for your input over here 🎖️
Given your comment, I removed the recently added documentation about SHOUTRRR_PARAMS in 5455207852 (ideally I'll remove it when doing a breaking change release).

@HansBaumgaertner @DennisGaida how come you were not aware of such address options? Maybe an interesting documentation update to be done for Shoutrrr 🤔

@qdm12 commented on GitHub (Jun 13, 2023): @piksel thanks so much for your input over here 🎖️ Given your comment, I removed the recently added documentation about `SHOUTRRR_PARAMS` in 5455207852530d1736cc839f039e0e5b93474f47 (ideally I'll remove it when doing a breaking change release). @HansBaumgaertner @DennisGaida how come you were not aware of such address options? Maybe an interesting documentation update to be done for Shoutrrr 🤔

saavagebueno commented

@piksel commented on GitHub (Jun 13, 2023):

Yeah, I am really confused by this as well. There are no examples of putting those settings in the params, only for putting them in the configuration URL itself.
The params are mentioned here, but I still don't understand why that would be the first thing to try: https://containrrr.dev/shoutrrr/0.7/services/generic/#queryparam_props

To clarify, I am not trying to shame you, I am just trying to understand what went wrong to improve the documentation and/or guide future users in the same situation.

@piksel commented on GitHub (Jun 13, 2023): Yeah, I am really confused by this as well. There are no examples of putting those settings in the params, only for putting them in the configuration URL itself. The params are *mentioned* here, but I still don't understand why that would be the first thing to try: https://containrrr.dev/shoutrrr/0.7/services/generic/#queryparam_props To clarify, I am not trying to shame you, I am just trying to understand what went wrong to improve the documentation and/or guide future users in the same situation.

saavagebueno commented

@DennisGaida commented on GitHub (Jun 15, 2023):

@qdm12 "how come you were not aware of such address options". sounds a bit offensive. We're all doing this for fun aren't we?

Just as the OP in this issue I found the SHOUTRRR_PARAMS in the source code of this repo and I thought this was the way of doing params for Shoutrrr in DDNS Updater. Trying to use what is there/existing. I checked what params shoutrrr allows and lo and behold there was a ContentType param. I just plugged that into SHOUTRRR_PARAMS and it works.

I totally didn't check if I could use another URL or URL params to modify the request, i.e. set the content-type this way. And honestly, I still think that it looks very clean using the additional environment variable, like so (makes it easier to see the base URL and the parameters):

    --env SHOUTRRR_ADDRESSES="generic://<shoutrrr-webhook-endpoint-url>" \
    --env SHOUTRRR_PARAMS="contenttype=text/plain" \

Knowing now (thanks @piksel!), that I can set the content-type via the URL/URL params I don't have to use SHOUTRRR_PARAMS. Looking at the documentation of shoutrrr I believe we are good. The default for the generic endpoint is application/json - why not. Personally I didn't consider being able to change the behavior via the URL since I found the "hidden" SHOUTRRR_PARAMS environment variable and thought this is how things should done (i.e. all URL parameters belong into the params env-var and it was simply not documented).

As for documentation goes: I still consider it best practice to document the available environment variables and since the default title of the shoutrrr message is "DDNS Updater" it should be known to users that you could change this default behavior by overriding the title via the URL - or can they? I didn't try it now, but does the SHOUTRRR_ADDRESSES take precedence over the environment parameter SHOUTRRR_PARAMS or would the title always be "DDNS Updater" (maybe a user would want this changed to something else?).

In short: Finding SHOUTRRR_PARAMS in the source code I thought I found the holy grail of fixing my Node-Red endpoint that expected JSON (since that's the shoutrrr default), but received plain text from DDNS updater. And I did find a solution. Maybe not the solution 😉

@DennisGaida commented on GitHub (Jun 15, 2023): @qdm12 _"how come you were not aware of such address options"_. sounds a bit offensive. We're all doing this for fun aren't we? Just as the OP in this issue I found the `SHOUTRRR_PARAMS` in the source code of this repo and I thought this was the way of doing **params** for Shoutrrr in DDNS Updater. Trying to use what is there/existing. I checked what params shoutrrr allows and lo and behold there was a ContentType param. I just plugged that into `SHOUTRRR_PARAMS` and it works. I totally didn't check if I could use another *URL* or URL params to modify the request, i.e. set the content-type this way. And honestly, I still think that it looks very clean using the additional environment variable, like so (makes it easier to see the base URL and the parameters): ``` --env SHOUTRRR_ADDRESSES="generic://<shoutrrr-webhook-endpoint-url>" \ --env SHOUTRRR_PARAMS="contenttype=text/plain" \ ``` Knowing now (thanks @piksel!), that I can set the content-type via the URL/URL params I don't have to use `SHOUTRRR_PARAMS`. Looking at the documentation of shoutrrr I believe we are good. The default for the generic endpoint is `application/json` - why not. Personally I didn't consider being able to change the behavior via the URL since I found the "hidden" `SHOUTRRR_PARAMS` environment variable and thought this is how things should done (i.e. all URL parameters belong into the params env-var and it was simply not documented). As for documentation goes: I still consider it best practice to document the available environment variables and since the default `title` of the shoutrrr message is "DDNS Updater" it should be known to users that you could change this default behavior by overriding the `title` via the URL - or can they? I didn't try it now, but does the `SHOUTRRR_ADDRESSES` take precedence over the environment parameter `SHOUTRRR_PARAMS` or would the `title` always be "DDNS Updater" (maybe a user would want this changed to something else?). In short: Finding `SHOUTRRR_PARAMS` in the source code I thought I found the holy grail of fixing my Node-Red endpoint that expected JSON (since that's the shoutrrr default), but received plain text from DDNS updater. And I did find *a* solution. Maybe not *the* solution 😉

saavagebueno commented

@qdm12 commented on GitHub (Jun 15, 2023):

"how come you were not aware of such address options". sounds a bit offensive.

Not intended to be offensive at all 😉 I guess the how come you were could had been a Why were you maybe, but I still don't see it as offensive re-reading it 😸

but does the SHOUTRRR_ADDRESSES take precedence over the environment parameter SHOUTRRR_PARAMS or would the title always be "DDNS Updater"

Interesting point, if it does take precedence (I hope it does), then I would rather remove SHOUTRRR_PARAMS and let the user add it to the address for each service, to avoid confusion which is, as we've seen, a lot more costly than copy pasting a parameter for each service.

If it doesn't, perhaps a better idea would be to change SHOUTRRR_PARAMS to SHOUTRRR_TITLE to override the default title and avoid using global params 🤔

@qdm12 commented on GitHub (Jun 15, 2023): > "how come you were not aware of such address options". sounds a bit offensive. Not intended to be offensive at all 😉 I guess the `how come you were` could had been a `Why were you` maybe, but I still don't see it as offensive re-reading it 😸 > but does the SHOUTRRR_ADDRESSES take precedence over the environment parameter SHOUTRRR_PARAMS or would the title always be "DDNS Updater" Interesting point, if it does take precedence (I hope it does), then I would rather remove SHOUTRRR_PARAMS and let the user add it to the address for each service, to avoid confusion which is, as we've seen, a lot more costly than copy pasting a parameter for each service. If it doesn't, perhaps a better idea would be to change SHOUTRRR_PARAMS to SHOUTRRR_TITLE to override the default title and avoid using global params 🤔

saavagebueno commented

@piksel commented on GitHub (Jun 15, 2023):

Params override the corresponding values from the address, yes.

I still think that it looks very clean using the additional environment variable,

For generic, yes, but for all of the "native" services it would probably not make much sense 🤷‍♀️.
The main problem with exposing is that it can be very confusing that all the fields needs to be available on all of the notification services.

change SHOUTRRR_PARAMS to SHOUTRRR_TITLE to override the default title

That seems the most useful solution to me at least. Email (smtp service) is the only one that "needs" a title. E-mails without a subject are generally not great.

@piksel commented on GitHub (Jun 15, 2023): Params override the corresponding values from the address, yes. > I still think that it looks very clean using the additional environment variable, For `generic`, yes, but for all of the "native" services it would probably not make much sense 🤷‍♀️. The main problem with exposing is that it can be very confusing that all the fields needs to be available on all of the notification services. > change SHOUTRRR_PARAMS to SHOUTRRR_TITLE to override the default title That seems the most useful solution to me at least. Email (`smtp` service) is the only one that "needs" a title. E-mails without a subject are generally not great.

saavagebueno commented

@qdm12 commented on GitHub (Jun 16, 2023):

What I coded locally:

SHOUTRRR_PARAMS is disabled and a warning redirects you to use SHOUTRRR_DEFAULT_TITLE and SHOUTRRR_ADDRESSES instead, if you still use it
The global default Shoutrrr title value defaults to DDNS updater if SHOUTRRR_DEFAULT_TITLE is left unset
After settings reading and validation, each shoutrrr address which does not have the title key in its url parameters has the keyvalue title={default title} added. I do this instead of using the global shoutrrr parameters to avoid overriding a title specified for a particular address. Note this also leaves the title untouched if an address has an empty title set like ?title=&otherkey=othervalue. Finally, since this modification is done after settings parsing etc., it won't show up in each shoutrrr address logged in the settings tree at program start time.

Is everyone's happy with this change?

@qdm12 commented on GitHub (Jun 16, 2023): What I coded locally: 1. `SHOUTRRR_PARAMS` is disabled and a warning redirects you to use `SHOUTRRR_DEFAULT_TITLE` and `SHOUTRRR_ADDRESSES` instead, if you still use it 2. The global default Shoutrrr title value defaults to `DDNS updater` if `SHOUTRRR_DEFAULT_TITLE` is left unset 3. After settings reading and validation, each shoutrrr address which does not have the `title` key in its url parameters has the keyvalue `title={default title}` added. I do this instead of using the global shoutrrr parameters to avoid overriding a title specified for a particular address. Note this also leaves the title untouched if an address has an empty title set like `?title=&otherkey=othervalue`. Finally, since this modification is done after settings parsing etc., it won't show up in each shoutrrr address logged in the settings tree at program start time. Is everyone's happy with this change?

saavagebueno commented

@DennisGaida commented on GitHub (Jun 16, 2023):

Sounds awesome!

@DennisGaida commented on GitHub (Jun 16, 2023): Sounds awesome!

saavagebueno commented