⚡ Add apiEndpoint to Telegram credentials

[m2scared]

Create an advanced option for setting custom Telegram's apiEndpoint.

Inspired by https://github.com/go-telegram-bot-api/telegram-bot-api/blob/3834565c356e9b2d94bd8080555aeaf795bbb0ea/bot.go#L100

[m2scared]

Hi @ivov,

Should I add manually reviewers?

[Joffcom]

Hey @m2scared,

Just looking at this one, Do you have an example in mind as to when this option would be used?

Looking at the pull request for the Go library they added it a few years ago incase there was a change to the Telegram API URL and it allows some limited testing as you could send the data to your own server or something like pipedream to check it.

Were you thinking about the same thing for n8n?

[m2scared]

Hi @Joffcom

Thanks for taking a look. Currently I am using it as a Telegram API Proxy. My proxy server checks the requests and fake some responses. This my current use case.

Another interesting ones would be testing and auditing.

[m2scared]

The proxy server can deny some requests depending on who made it.

[Joffcom]

Hey @m2scared,

That sounds like a good use case as well, I will give it a review and test this afternoon and we can hopefully get this one merged in and closed off for you.

[m2scared]

Thanks. One thing I notice is that it may cause problems if you have an existing Telegram configuration.

Is there a way to guarantee that upgrades will not break running clients?

[Joffcom]

Hey @m2scared,

That would be a bit of a problem, What I would do is check if the "Advanced" toggle is enabled or not then set the value based on that which removes the risk of breaking the node.

const uri = credentials.advanced ? formatString(`${credentials.apiEndpoint}`, `${credentials.accessToken}`, endpoint) : `https://api.telegram.org/bot${credentials.accessToken}/${endpoint}`;

I did wonder if the {0}/{1} is even needed maybe it is enough to just set the base URL as I suspect it will always be bot{0} for the token and / {1} for the action so that then changes the above a bit more as you wouldn't need the extra formatString() function.

There are also 3 other parts that ignore the advanced option fully which may need to be thought about.

• Credential Test (telegramBotTest)
• Trigger Node - Download Files
• Standard Node - Download Files

Let me know what you think.

[m2scared]

Hey @Joffcom,

Hey @m2scared,

That would be a bit of a problem, What I would do is check if the "Advanced" toggle is enabled or not then set the value based on that which removes the risk of breaking the node.

const uri = credentials.advanced ? formatString(`${credentials.apiEndpoint}`, `${credentials.accessToken}`, endpoint) : `https://api.telegram.org/bot${credentials.accessToken}/${endpoint}`;

I will add that.

I did wonder if the {0}/{1} is even needed maybe it is enough to just set the base URL as I suspect it will always be bot{0} for the token and / {1} for the action so that then changes the above a bit more as you wouldn't need the extra formatString() function.

Since the Golang added it I thought would be good to keep. But I have no good reasons besides that.

There are also 3 other parts that ignore the advanced option fully which may need to be thought about.

• Credential Test (telegramBotTest)
• Trigger Node - Download Files
• Standard Node - Download Files

Let me know what you think.

Let me check them. I think they should check.

[m2scared]

Hey @Joffcom,

I believe I have addressed all the issues. Could you take a look?

[Joffcom]

Hey @m2scared,

I will take a look this afternoon 👍🏻

[Joffcom]

Hey @m2scared,

Looks good to me, I will write up the "Why" behind this change on our internal system and we can see what happens from there.

Thanks for going through the changes 👍🏻

[nivbe06]

Hi m2scared, thanks for your contribution!
After internal discussion, we have decided to not merge this request
With a feature like that, we had to decide whether we develop it for a single node or all nodes, as it is not related only to one service but potentially to every service.
In that case, it should meet higher usage bars, which in this feature's case, it didn't meet.
Appreciate your collaboration on this, it wasn't the most straightforward one from our side, therefore it took some iterations with you to get to this conclusion.
Let me know if you'd like to have a chat on this process and how we're deciding what's get in and what's not
Niv from n8n

[m2scared]

Hi m2scared, thanks for your contribution! After internal discussion, we have decided to not merge this request With a feature like that, we had to decide whether we develop it for a single node or all nodes, as it is not related only to one service but potentially to every service. In that case, it should meet higher usage bars, which in this feature's case, it didn't meet. Appreciate your collaboration on this, it wasn't the most straightforward one from our side, therefore it took some iterations with you to get to this conclusion. Let me know if you'd like to have a chat on this process and how we're deciding what's get in and what's not Niv from n8n

No problem @nivb06.

Thanks for the feedback.

[a5r0n]

@nivb06
plz reconsider merging this PR.
it has some cool uses like supporting self-hosted servers for the telegram bot api (with user-mode support) like this: https://github.com/tdlight-team/tdlight-telegram-bot-api#user-mode

this PR is pretty useful and I hope you'll give it another chance.
thx!