RegisterWebhookAsync does not encode the url parameter

[mitchcapper]

RegisterWebhookAsync does not url encode the url parameter which breaks registration if you have a ? or & in it for example. I can create a PR to properly encode it but it will double encode if someone is already encoding it because the current version does not.

I recommend Uri.EscapeDataString

At a minimum if you don't want to encode it might be good to add some notes on that, also that port 443 is the only allowed port for https servers.

This causes an error on twitters side that is not clear if you don't manually encode the url.

[JoshKeegan]

Sounds good to me. I can't think of any reason to not encode it in Tweetinvi, but it should probably take an optional bool encodeUri = true in case someone has a use-case that means it's already encoded :)

[mitchcapper]

No there is no reason not to encode it EXCEPT for someone already using this library and running into this problem. If they then upgrade and the method signature is the same their code will now break as it will be double encoded. We could make an ecodeUri param that just breaks for everyone and forces them to set true or false. Or you could say that likely there are just not a lot of users who probably are currently encoding the url so if we change the current behavior no big deal.

[JoshKeegan]

I’d personally change the current behaviour. As long as it’s released in a major version update I think that’s fine & it can just be documented in the release notes. @linvi would you be happy with that?

[linvi]

I agree, the url should be and will be encoded.

Thanks for the feedback

[linvi]

@JoshKeegan @mitchcapper I have been looking at this tonight, and I actually wondered why Tweetinvi should encode this url.

If the developer does not encore the url why should it be Tweetinvi responsibility to do so?

In the end Tweetinvi is a library that executes what the developer sends, regardless of what he asks for.

In the case Tweetinvi had to handle this encoding we would end up supporting 2 cases :

• Developers sends a not encoded url and Tweetinvi should encode it
• Developer sends an already encoded url, Tweetinvi should not encode it twice.

By moving this responsibility up to the developer and documenting it this will not be a problem and it also provides more freedom to the developer if tomorrow the way Twitter expects the URL changes (no need of library release).

Let me know what you think but as is, I think I will close this ticket.

[mitchcapper]

Personally I disagree, I think it is odd behavior to encode form data at a higher level. If you call a post or get function one doesn't normally manually encode the form data ahead of time. In theory a library should make a developers life easier. I assume if there is a url parameter I pass it a url and not have previously uri encoded that url. The fact it must be encoded has to do with how the call is done to twitter so I would put in the realm of the library. If a service updates it would be great if all I do is update the library I use to interact with it and not have to update my code.

With that said clearly I think this way as I filed the bug. Documentation will also work, but in terms of expected it would not seem to me to be the desired default behavior.

[JoshKeegan]

If we think of Tweetinvi as an SDK for Twitter, the consumer of Tweetinvi should just know that by using Tweetinvi, they are using Twitter. The fact that Tweetinvi is using Twitter’s HTTP REST API and this parameter needs URL encoding is irrelevant to the consumer. What happens if Twitter released a gRpc API and Tweetinvi started changing calls to use that? Having leaked implementation details of the HTTP API, it now becomes harder work to port to a different implementation...

So, from a purist point of view Tweetinvi should do the encoding.

However, in reality, if Twitter made any major changes to their API (never mind something as major as switching to gRpc) it’s likely to require changes to the public contract of Tweetinvi.

So either way the developer using Tweetinvi needs to have some awareness of Twitter and it’s APIs. I don’t think there’s a right answer, but would probably lean towards having Tweetinvi do the encoding.

[linvi]

Thank you both for your comments, I will read more on the subject and will let you know.

[linvi]

I could not find any good source on the subject but I think I understand your point of view and will proceed with it.