nickoala / telepot

Python framework for Telegram Bot API
MIT License
2.43k stars 474 forks source link

Display appropriate error message on invalid token #40

Closed zumoshi closed 8 years ago

zumoshi commented 8 years ago

Hello, me again :)

so this took me a while to figure out , if you supply an invalid token , say asd , telegram redirects you to https://core.telegram.org/bots . to try click on this link : http://api.telegram.org/botasd/getMe

this results in a BadHTTPResponse with a 27KB unreadable html in the console... i know that its my job to validate the tokens before passing it to telepot , but on this last project on which the tokens where provided by users, it took me a while to figure out what was going on , perhaps you could raise a TelegramError with message invalid token instead of 27KB of html?

example code :

import telepot

bot = telepot.Bot('asd')
bot.getMe()

results in : image

html actually fills the cmd buffer , so the actual error is not even visible , i had to dump the output to a file to find out what's what.

thanks in advance.

nickoala commented 8 years ago

The logic is this: a request to Bot API expects a JSON object in return. If the response is not a JSON object, it raises a BadHTTPResponse.

The question is this: when the response is not a JSON object, is it necessarily because the token is invalid? Could there be other causes?

At this moment, I cannot think of any other causes. The only instance where I have seen (and can imagine) a BadHTTPResponse is when the token is invalid. However, this is not guaranteed.

As of now, when Telegram servers are down, Bot API would return a JSON object indicating a bad gateway. Let's say, sometime in the future, Telegram may decide that when the servers are down, all Bot API requests be re-directed to a web page. In this case, a BadHTTPResponse would happen but it is not due to an invalid token. How could I distinguish between a BadHTTPResponse that is caused by an invalid token and one that is not? I cannot.

My take is this:

I could try to suggest the possible cause in the BadHTTPResponse, but it would only end up with the same dilemma. What if, one day, a BadHTTPResponse is caused by something other than an invalid token? After all, it is Telegram who decides when to re-direct to a web page. Bot API's response to an invalid token is unspecified.

In the end, I choose to opt for correctness, and keep the BadHTTPResponse as is.

zumoshi commented 8 years ago

a redirect and a non-json response are not exactly the same thing , maybe you could associate a 302 code , or more specifically a 302 redirect to core.telegram.org which is a completely different subdomain than the api to an invalid token. also i have never come across an actual use of redirect in any of api calls , so maybe you could stop following the redirects ?

image

as you can see the main html before redirect is relatively short , one of my problems was that i didn't even see the BadHTTPResponse error because it was buried under 27kb of html in the console.


another possibility would be some sort of regex based validation , either as part of telepot.Bot constructor or as a separate helper method , there should be some logic behind what telegram considers a valid token and an invalid one , a valid but wrong token will respond with a valid json response with code 401 Unauthorized

image


i should add that i also agree that we shouldn't just replace BadHTTPResponse with an invalid token exception , but we can narrow it down by specifying a 302 code (not a failure, which is the most likely cause for a badhttpresponse) that redirects to the specific address (not on the official api subdomain which bot should not operate outside of it).

nickoala commented 8 years ago

I will not provide a regex validation of token, because I am just not sure what the format is, and what it might become in the future. Yes, it is easy to discern from existing tokens, but there is no evidence that it will stay the same. My main reluctance stems from the fact that there is simply no official token format declared by Telegram. In fact, they don't need one.

Your suggestion of not following redirects is reasonable, but as the author of telepot, I have to weigh it against the risk of: what if, suddenly, Telegram uses redirects to handle Bot API requests? Telepot will fail "catastrophically", all for the occasional benefit of not "burying an exception under 27 kB of HTML". :smile: I know it is far-fetched, but it is imaginable.

Yet, I understand the frustration of not knowing what is going on because the clue is buried under 27 kB of HTML, so I have decided to override the method __str__() and __unicode__() to give you a better view of the situation. Expect something like this in the upcoming version:

>>> import telepot
>>> bot = telepot.Bot('abc')
>>> bot.getMe()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "telepot/__init__.py", line 173, in getMe
    return self._parse(r)
  File "telepot/__init__.py", line 164, in _parse
    raise BadHTTPResponse(response.status_code, response.text)
telepot.exception.BadHTTPResponse: Status 200 - First 500 characters are shown below:
<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8">
    <title>Bots: An introduction for developers</title>
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <meta property="og:title" content="Bots: An introduction for developers">
    <meta property="og:image" content="https://core.telegram.org/file/811140663/1/uHVzwsRJz3Y/a499733c59840694ca">
    <meta property="og:description" content="Bots are third-party applications that run inside Telegram. Users can in
>>>
zumoshi commented 8 years ago

yes that would be nice to limit to first 500 characters and format it (whether its json or html). however i still think a separate exception would be beneficial , because i have implanted in my code to assume BadHTTPResponse is invalid token , without any way to differentiate a redirect out of api to a non-json answer in the api. in the event of change of behavior in case telegram servers are down for example my code would mistakenly assume token is invalid.

i have another proposal , maybe you could subclass BadHTTPResponse and add a RedirectOutOfApiSubdomain exception , due being a subclass of BadHTTPResponse existing code won't break , and behavior remains mostly unchanged . also its more generic so in case of a change of api telepot won't break , it only occurs when a redirect happened to a domain other than api.telegram.org AND the response is not json.


also while we're at it might as well add some other exceptions too. i find myself writing code like this:

try:
    # some code that sends something to user
except telepot.TelegramError as ex:
    if ex.description == '[Error]: Bot was blocked by the user':
        # remove user from database or something similar
    else:
        raise ex

it would be nice if you could subclass TelegramError to have different exception for different errors , yet current behavior will remain unchanged since they are sub classes of the main exception. some exceptions to subclass:

nickoala commented 8 years ago

I will not add a RedirectOutOfApiSubdomain (or something like that) because, in my opinion, requesters to a web service should be blind to redirects and only interpret answers as they are sent back. Although it is reasonable to expect Bot API to not redirect, one can never say it should not redirect.

An easy way to differentiate between a bad token and server down is this:

I could add a redirect history to BadHTTPResponse to help you differentiate. But, because situations giving rise to BadHTTPResponse are very limited and you can already distinguish them as described above, I would hold that off for now.

Your suggestion of adding subclasses to TelegramError is a good one. But I need you to give me a list of possible errors and their error code. I am sure you encounter those more than I ever do. Would you do that? I appreciate your help.

zumoshi commented 8 years ago

still not convinced about the invalid token exception, but its your library so i won't ask again for it .

as for the error codes i have listed the one's I've encountered most , but i will start making a more detailed list (with full description and error codes) as i encounter them from now on.

nickoala commented 8 years ago

@zumoshi , thank you, both for relenting and the list.

zumoshi commented 8 years ago
telepot.exception.TelegramError:
("[Error]: Bad Request: Can't parse message text: Can't find end of the entity starting at byte offset 140", 400)
('[Error]: Unauthorized', 401)
('[Error]: Bad Request: group chat is migrated to supergroup chat', 400)
('[Error]: Forbidden: bot was kicked from the group chat', 403)
('[Error]: Bot was blocked by the user', 403)
('[Error]: Bad Request: QUERY_ID_INVALID', 400)
('[Error]: Bad Request: RESULT_ID_DUPLICATE', 400)
('[Error]: Bad Request: RESULT_ID_INVALID', 400)
('[Error]: Bad request: Can\'t find field "id"', 400)
('[Error]: Bad Request: file is too big[size:22288926]', 400)
('[Error]: Bad Request: chat not found', 400)
('[Error]: PEER_ID_INVALID', 400)
('[Error]: Forbidden: bot was kicked from the channel chat', 403)
('[Error]: Bad Request: Type of file to send mismatch', 400)
('[Error]: Too many requests: retry later', 429)
('[Error]: Conflict: another webhook is active', 409)

these are the ones i found in the logs now , i will keep adding to the list as i encounter more errors.

nickoala commented 8 years ago

Let me add this:

('[Error]: Bad request: Message text is empty', 400)

I am starting to make changes now. Expect a new version of telepot within the next 3 days (if everything goes well).

May I ask:

zumoshi commented 8 years ago

when parsemode is set to markdown but text is not valid markdown , for example the case i encountered was an entity i think a bracket that was opened but not closed, therefor why the error says cant find end of entity. i catch the error than escape all markdown related characters than resend the message (cant 't send without parsmode because of a signture link added by bot to user supplied message)

nickoala commented 8 years ago

Telepot now updated, version 6.7

I have added 3 subclasses to TelegramError:

I did not include all of your list because of this consideration:

I may have disappointed you, but here is a very nice thing which I am sure you will love. For example, if you want to isolate these two:

("[Error]: Bad Request: Can't parse message text: Can't find end of the entity starting at byte offset 140", 400)
('[Error]: Bad Request: Type of file to send mismatch', 400)

You can do that easily by subclassing TelegramError with a list of signature regex:

class ParseMessageTextError(telepot.exception.TelegramError):
    DESCRIPTION_PATTERNS = ['parse message text']

class FileTypeMismatchError(telepot.exception.TelegramError):
    DESCRIPTION_PATTERNS = ['file.*type.*mismatch', 'type.*file.*mismatch']

The matching is done case-insensitively, so you don't have to worry about the case.

This is the same principle I use to define my own UnauthorizedError, BotWasKickedError, and BotWasBlockedError. It is very flexible. You can easily define your own error classes and catch them.

I hope this issue can now be closed. :smile:

zumoshi commented 8 years ago

being able to subclass will solve most of my problems , i like how you are going towards making telepot extensible. just this morning i was unable to achieve what i needed with delegate bot , so i made a custom seed_calculating_function and it worked perfectly (almost ... i also had to create a custom ChatHandler replacement).

if it was me , i would've made a CantSendMessage exception which covers the following cases:

which is mostly useful when bots have a subscribe function , and user does one of the above before unsubscribing , i can use the following exception to remove the user from database.

and another one for 429 error (too many messages) which would help while sending message to subscribers for throttling .

but the rest of exceptions are mostly useful case by case and being able to define custom ones with regex really removes the need for them to be in the framework. good job on the update :+1:

the only thing i can think of (which you probably have already considered) is to provide a complete list of errors you have encountered so far with description of when they happen in the documentation near where you explain how to subclass TelegramError. it could help not only to sub classing but also to predicting errors that may happen when creating the bot. specially for new developers that are not very familiar with the API.

edit: ooh and now that we get to subclass could you let me create a subclassed extecption for redirect to core.telegram.org from api.telegram.org and name it InvalidToken ? or more general question , how would sub classing of BadHTTPResponse be ? is it possible?

nickoala commented 8 years ago
  1. Nice to hear that you create your own seed-calculating-function. I never expect anyone to understand that part of the docs. :smile:
  2. I wouldn't have one class to cover those 3 cases. I would have 3 classes to cover those 3 cases. The latter two already exist (telepot.exception.BotWasKickedError and BotWasBlockedError ). The first one you can subclass on your own. I may consider adding it later.
  3. Good suggestion to have 429 too-many-messages as a separate exception (subclass of TelegramError). Can you show me the description and error code of that?
  4. Adding that list of errors to docs is a good idea. I will do it in the foreseeable future.
  5. The way TelegramError and its subclasses works is this: On seeing an erroneous response, telepot matches the error description against all subclasses' DESCRIPTION_PATTERNS. If a match is found, an exception of that subclass is raised. If no match, a generic TelegramError is raised.

    I don't see how we can do the same thing with BadHTTPResponse because there is not a general way to map response to subclass. What subclasses do you want to have, by the way? InvalidToken? What else?

    I don't like relying on a redirect to mean an InvalidToken because it is simply not safe to make that assumption on the framework-level. It is perfectly fine on the application-level. But I don't like to put it in the framework. It is not robust.

    Finally, I don't know about your project, but it seems to me, if a user supplies a token that is so clearly wrong, it is him or her who is being unreasonable. I think we have spent enough time and energy on that.

    If you want to solve the problem cleanly, you should really ask Telegram to give a more well-defined response. It is not telepot's problem.

nickoala commented 8 years ago

Your new profile photo intrigues me, by the way. It is better than the dead :fish:

zumoshi commented 8 years ago

ok i think we made good progress here , i'm going to let you close this issue 😁 just one last question i didn't find how to create the custom exception in the docs , do i have to register it somewhere or does it automatically work as soon as the subclassed exception is imported by any file?

nickoala commented 8 years ago

I haven't put the exception stuff into docs. Will do it sometime.

You don't have to register the subclasses. Python classes have a special method __subclasses__() to find out all the subclasses. So, you just define the subclasses. That's it!

zumoshi commented 8 years ago

you are probably not going to like this , but i have another idea. 😱 embedding more data in the exceptions .

for example if user is kicked from the group you could do this:

try:
    bot.sendMessage(chat_id, text)
except BotWasKickedError as ex:
    print(ex.chat_id)

this may seem useless (since chat_id is available in the except block of example code) but in my case i am using delegates and use sender to reply, from a method other than on_message , which didn't receive chat_id as a parameter. of course i could refactor my code but it could be simpler.

my suggestion is that we receive parameters sent to telepot methods as a dictionary to __init__ of our custom exception along with name of method , so we can create our own custom properties.

class BotWasKickedError(TelegramError):
    def __init__(self, description, error_code, params):
        super(BotWasKickedError).__init__(self, description, error_code, params)
        self.chat_id= params['chat_id']
    DESCRIPTION_PATTERNS = ['bot.*blocked']

not to mention ability to include these nice information in the logs.

this could make custom exceptions a lot more useful. but may be a little hard to create (i think it would need refactoring all calls to _parse function). so i won't push it if you don't consider it useful enough to be worth the trouble.


on another note (i should have probably made another issue for this but it is so small that is not worth a separate issue) my ide is having a hard time auto-completing self.sender.X methods in delegates. it is a minor issue but it would be nice if you could rewrite Sender class to be more IDE friendly.

nickoala commented 8 years ago
  1. Actually, your suggestion of including more info in exception may be a good one. It's in the same vein as Pull Request #43. I will give it some thoughts before deciding what to do.
  2. The way I implement Sender, it dynamically creates methods for itself (using setattr), which are actually partial functions over the bot's underlying methods with chat_id fixed. You can ask me to be more IDE friendly, but you can also ask the IDE maker to make the IDE smarter. I can imagine a super-smart IDE to understand what I am doing and give you the right auto-completes. I encourage you to suggest this to the IDE maker. :smile:
zumoshi commented 8 years ago

i looked at the pull request it was mostly about parameters sent by telegram servers as opposed to parameters sent to telepot methods when calling them but seeing his example that would come in handy also. thanks for considering it looking forward to the next update.

zumoshi commented 8 years ago
Traceback (most recent call last):
  File "C:\Python34\lib\site-packages\telepot\__init__.py", line 338, in handle
    callback(update['message'])
  File "kerm.py", line 13, in handle_message
    bot.sendMessage(msg['chat']['id'], '\U0001f610', reply_to_message_id=msg['message_id'])
  File "C:\Python34\lib\site-packages\telepot\__init__.py", line 190, in sendMessage
    return self._parse(r)
  File "C:\Python34\lib\site-packages\telepot\__init__.py", line 179, in _parse
    raise TelegramError(description, error_code)
telepot.exception.TelegramError: ('[Error]: Too many requests: retry later', 429)

as per your request to provide you will an example of 429 error this is the full trace.

nickoala commented 8 years ago

@zumoshi, thanks for the constant update. I have added a TooManyRequestsError. Telepot is now at version 6.8

Sorry to disappoint you again. I have decided not to include the original calling parameters in TelegramError. I'd like to keep them "clean" and not include things that are "bigger" than their immediate contexts. I have also referenced the requests library and aiohttp library and found that they tend to make the same choice, e.g. a HttpProcessingError does not include the URL.

That said, I appreciate your efforts of helping me make telepot better. Feel free to make any more suggestions.

vladimirmyshkovski commented 7 years ago

I get the following error continuously:

Traceback (most recent call last):
  File "/home/narnikgamarnik/PycharmProjects/FlaskProjects/luminate/venv/lib/python3.5/site-packages/telepot/__init__.py", line 787, in get_from_telegram_server
    result = self.getUpdates(offset=offset, timeout=timeout)
  File "/home/narnikgamarnik/PycharmProjects/FlaskProjects/luminate/venv/lib/python3.5/site-packages/telepot/__init__.py", line 630, in getUpdates
    return self._api_request('getUpdates', _rectify(p))
  File "/home/narnikgamarnik/PycharmProjects/FlaskProjects/luminate/venv/lib/python3.5/site-packages/telepot/__init__.py", line 398, in _api_request
    return api.request((self._token, method, params, files), **kwargs)
  File "/home/narnikgamarnik/PycharmProjects/FlaskProjects/luminate/venv/lib/python3.5/site-packages/telepot/api.py", line 131, in request
    return _parse(r)
  File "/home/narnikgamarnik/PycharmProjects/FlaskProjects/luminate/venv/lib/python3.5/site-packages/telepot/api.py", line 126, in _parse
    raise exception.TelegramError(description, error_code, data)
telepot.exception.TelegramError: ('Conflict: terminated by other long poll or webhook', 409, {'description': 'Conflict: terminated by other long poll or webhook', 'ok': False, 'error_code': 409})

I am trying to run the bat as a blueprint in the flask.

All this is due to the fact that I do not register the bot as it should (bot.init_app (app)) How to register it correctly - I do not know.

Conflict, as I understand it, the app itself, and bot.message loop (send)

For me it is important that the boat was part of the Flask application.

nickoala commented 7 years ago

That error you got was because you had webhook set up for the same bot. Telegram does not allow you to use webhook and getUpdates at the same time.

To cancel webhook:

>>> import telepot
>>> bot = telepot.Bot('TOKEN')
>>> bot.setWebhook()

Try that, and see if you still get the error.

vladimirmyshkovski commented 7 years ago

bot.setWebhook() no results, but if I write bot.message_loop(send, source=update_queue) and nothing is passing in the queue, everything works as it should

That is, the problem is solved, but I do not understand why it works

this is example of my code:

from ..utils.quere import update_queue

bp = Blueprint('bot', __name__)

bot = telepot.Bot(TOKEN)
#bot.setWebhook()

@bp.route('/send', methods=['POST'])
def send():
    if request.method == "POST":     
        params = request.form
        email = params['email']
        phone = params['phone']
        id = params['id']
        bot.sendMessage(169641544, phone + "\n" + email)
    return str(request.form)

bot.message_loop(send, source=update_queue)