search

rubenlagus / TelegramBots

Java library to create bots using Telegram Bots API
https://telegram.me/JavaBotsApi
MIT License
4.77k stars 1.22k forks source link

Usage Abilities, webhook and polls causes IllegalStateException #749

Closed kortov closed 4 years ago

kortov commented 4 years ago

Create Webhook ability bot, add ability to send a poll to a user via sendApiMethod (reproducable with quiz and poll)

val sendPoll = SendPoll(AbilityUtils.getChatId(upd), "question", Arrays.asList("a", "b", "c"))
sendApiMethod(sendPoll)

when user clicks on the answer image

the telegram sends to a bot this

{
    "update_id": 689323206,
    "poll": {
        "id": "5269766460913221652",
        "question": "question",
        "options": [
            {
                "text": "a",
                "voter_count": 1
            },
            {
                "text": "b",
                "voter_count": 0
            },
            {
                "text": "c",
                "voter_count": 0
            }
        ],
        "total_voter_count": 1,
        "is_closed": false,
        "is_anonymous": true,
        "type": "regular",
        "allows_multiple_answers": false
    }
}

and then bot throws exception

java.lang.IllegalStateException: Could not retrieve originating user from update
    at org.telegram.abilitybots.api.util.AbilityUtils.getUser(AbilityUtils.java:73) ~[telegrambots-abilities-4.8.jar:na]
    at org.telegram.abilitybots.api.bot.BaseAbilityBot.checkBlacklist(BaseAbilityBot.java:355) ~[telegrambots-abilities-4.8.jar:na]
    at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:174) ~[na:1.8.0_242]
    at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:175) ~[na:1.8.0_242]
    at java.util.stream.Streams$StreamBuilderImpl.forEachRemaining(Streams.java:419) ~[na:1.8.0_242]
    at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:482) ~[na:1.8.0_242]
    at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:472) ~[na:1.8.0_242]
    at java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150) ~[na:1.8.0_242]
    at java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173) ~[na:1.8.0_242]
    at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) ~[na:1.8.0_242]
    at java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:485) ~[na:1.8.0_242]
    at org.telegram.abilitybots.api.bot.BaseAbilityBot.onUpdateReceived(BaseAbilityBot.java:194) ~[telegrambots-abilities-4.8.jar:na]
    at org.telegram.abilitybots.api.bot.AbilityWebhookBot.onWebhookUpdateReceived(AbilityWebhookBot.java:61) ~[telegrambots-abilities-4.8.jar:na]

because method doesn't check for chatId 

boolean checkBlacklist(Update update) {
        Integer id = AbilityUtils.getUser(update).getId();

        return id == creatorId() || !blacklist().contains(id);
    }

and then telegram desperately tries to send you this json

this method has many such methods prone to this problem

public void onUpdateReceived(Update update) {
        log.info(format("[%s] New update [%s] received at %s", botUsername, update.getUpdateId(), now()));
        log.info(update.toString());
        long millisStarted = System.currentTimeMillis();

        Stream.of(update)
                .filter(this::checkGlobalFlags)
                .filter(this::checkBlacklist)
                .map(this::addUser)
                .filter(this::filterReply)
                .map(this::getAbility)
                .filter(this::validateAbility)
                .filter(this::checkPrivacy)
                .filter(this::checkLocality)
                .filter(this::checkInput)
                .filter(this::checkMessageFlags)
                .map(this::getContext)
                .map(this::consumeUpdate)
                .forEach(this::postConsumption);
    }

I have a reproducer you need to fill properties, run springboot BootigramApplication and then send to a bot /uploadquiz

kortov commented 4 years ago

@addo37 thanks, but now I got 

java.lang.NullPointerException: null
    at org.telegram.abilitybots.api.bot.BaseAbilityBot.checkBlacklist(BaseAbilityBot.java:357) ~[telegrambots-abilities-4.8.1.jar:na]
    at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:174) ~[na:1.8.0_242]
    at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:175) ~[na:1.8.0_242]
    at java.util.stream.Streams$StreamBuilderImpl.forEachRemaining(Streams.java:419) ~[na:1.8.0_242]
    at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:482) ~[na:1.8.0_242]
    at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:472) ~[na:1.8.0_242]
    at java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150) ~[na:1.8.0_242]
    at java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173) ~[na:1.8.0_242]
    at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) ~[na:1.8.0_242]
    at java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:485) ~[na:1.8.0_242]
    at org.telegram.abilitybots.api.bot.BaseAbilityBot.onUpdateReceived(BaseAbilityBot.java:191) ~[telegrambots-abilities-4.8.1.jar:na]
    at org.telegram.abilitybots.api.bot.AbilityWebhookBot.onWebhookUpdateReceived(AbilityWebhookBot.java:61) ~[telegrambots-abilities-4.8.1.jar:na]

I guess it's because java tries to evaluate (Integer) null == int a

I think there are should be tests for this issue, I tried to add them, but Idea refused to run tests in project (no tests were found, so I filed a ticked to JB) so I gave up, sorry :)

addo47 commented 4 years ago

Alright @kortov, this should do it.

My apologies, the initial PR was undercooked. This new PR adds two tests and addresses the issue permanently (hopefully).

Thanks!

kortov commented 4 years ago

@addo37 thanks for your work, but I got new errors :) I built your pr to a local maven via snapshot version and got this

java.lang.IllegalStateException: Could not retrieve originating chat ID from update
    at org.telegram.abilitybots.api.util.AbilityUtils.getChatId(AbilityUtils.java:154) ~[telegrambots-abilities-4.8.2-SNAPSHOT.jar:na]
    at org.telegram.abilitybots.api.objects.ReplyFlow$ReplyFlowBuilder.lambda$toStateful$3(ReplyFlow.java:110) ~[telegrambots-abilities-4.8.2-SNAPSHOT.jar:na]
    at org.telegram.abilitybots.api.objects.Reply.lambda$isOkFor$0(Reply.java:48) ~[telegrambots-abilities-4.8.2-SNAPSHOT.jar:na]
    at java.util.stream.ReduceOps$1ReducingSink.accept(ReduceOps.java:80) ~[na:1.8.0_242]
    at java.util.Spliterators$ArraySpliterator.forEachRemaining(Spliterators.java:948) ~[na:1.8.0_242]
    at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:482) ~[na:1.8.0_242]
    at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:472) ~[na:1.8.0_242]
    at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:708) ~[na:1.8.0_242]
    at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) ~[na:1.8.0_242]
    at java.util.stream.ReferencePipeline.reduce(ReferencePipeline.java:551) ~[na:1.8.0_242]
    at org.telegram.abilitybots.api.objects.Reply.isOkFor(Reply.java:49) ~[telegrambots-abilities-4.8.2-SNAPSHOT.jar:na]
    at org.telegram.abilitybots.api.bot.BaseAbilityBot.lambda$filterReply$14(BaseAbilityBot.java:511) ~[telegrambots-abilities-4.8.2-SNAPSHOT.jar:na]
    at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:174) ~[na:1.8.0_242]
    at java.util.Spliterators$ArraySpliterator.forEachRemaining(Spliterators.java:948) ~[na:1.8.0_242]
    at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:482) ~[na:1.8.0_242]
    at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:472) ~[na:1.8.0_242]
    at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:708) ~[na:1.8.0_242]
    at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) ~[na:1.8.0_242]
    at java.util.stream.ReferencePipeline.reduce(ReferencePipeline.java:541) ~[na:1.8.0_242]
    at org.telegram.abilitybots.api.bot.BaseAbilityBot.filterReply(BaseAbilityBot.java:516) ~[telegrambots-abilities-4.8.2-SNAPSHOT.jar:na]
    at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:174) ~[na:1.8.0_242]
    at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:193) ~[na:1.8.0_242]
    at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:175) ~[na:1.8.0_242]
    at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:175) ~[na:1.8.0_242]
    at java.util.stream.Streams$StreamBuilderImpl.forEachRemaining(Streams.java:419) ~[na:1.8.0_242]
    at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:482) ~[na:1.8.0_242]
    at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:472) ~[na:1.8.0_242]
    at java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150) ~[na:1.8.0_242]
    at java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173) ~[na:1.8.0_242]
    at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) ~[na:1.8.0_242]
    at java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:485) ~[na:1.8.0_242]
    at org.telegram.abilitybots.api.bot.BaseAbilityBot.onUpdateReceived(BaseAbilityBot.java:192) ~[telegrambots-abilities-4.8.2-SNAPSHOT.jar:na]
    at org.telegram.abilitybots.api.bot.AbilityWebhookBot.onWebhookUpdateReceived(AbilityWebhookBot.java:61) ~[telegrambots-abilities-4.8.2-SNAPSHOT.jar:na]

I guess if in .filter(this::filterReply) any reply get chatId without null checks or via AbilityUtils then will be NPE or IllegalStateException. I guess there should be test in ReplyFlowTest for this then. (again I've tried to elaborate on this, but Idea refuses run tests or debug this :( )

addo47 commented 4 years ago

Well, that's a different error, so the first one has been resolved.

Technically speaking, this also becomes the developer's responsibility to call getChatId only when the update has a valid chat. This is different here since the developer can define the predicate for a reply. The getUser issue needed to be fixed since AbilityBot was calling it.

Nevertheless, the method is outdated. I've added a few condition checks there, but it will still return an exception if the update has no chat id (or user id, which acts as the chat id).

kortov commented 4 years ago

Thank you, I checked after removing all replies and sending quiz via just ability, and it works (no exceptions). What about replies I have no clue at the moment why I still get IllegalStateException from getChatId though it seems that I added to all my ReplyFlow builders onlyIf Predicate with checks for messages (Flag.MESSAGE). The first issue I guess is resolved, I'll investigate replies later. This issue can be closed I guess

kortov commented 4 years ago

@addo37 I created a PR https://github.com/kortov/TelegramBots/pull/1 to show that current pr fails on ReplyFlow which have to handle only messages when it gets a Poll update. You can see ReplyFlowPollTest in which the bot doesn't send polls and have only one Flow for handling messages and it fails with 

java.lang.IllegalStateException: Could not retrieve originating chat ID from update

    at org.telegram.abilitybots.api.util.AbilityUtils.getChatId(AbilityUtils.java:160)
    at org.telegram.abilitybots.api.objects.ReplyFlow$ReplyFlowBuilder.lambda$toStateful$3(ReplyFlow.java:110)
    at org.telegram.abilitybots.api.objects.Reply.lambda$isOkFor$0(Reply.java:48)
    at java.util.stream.ReduceOps$1ReducingSink.accept(ReduceOps.java:80)
    at java.util.Spliterators$ArraySpliterator.forEachRemaining(Spliterators.java:948)
    at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:482)
    at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:472)
    at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:708)
    at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
    at java.util.stream.ReferencePipeline.reduce(ReferencePipeline.java:551)
    at org.telegram.abilitybots.api.objects.Reply.isOkFor(Reply.java:49)
    at org.telegram.abilitybots.api.bot.BaseAbilityBot.lambda$filterReply$14(BaseAbilityBot.java:511)
    at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:174)
    at java.util.Spliterators$ArraySpliterator.forEachRemaining(Spliterators.java:948)
    at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:482)
    at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:472)
    at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:708)
    at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
    at java.util.stream.ReferencePipeline.reduce(ReferencePipeline.java:541)
    at org.telegram.abilitybots.api.bot.BaseAbilityBot.filterReply(BaseAbilityBot.java:516)
    at org.telegram.abilitybots.api.bot.ReplyFlowPollTest.repliesHandlePollResponse(ReplyFlowPollTest.java:68)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:675)
    at org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
    at org.junit.jupiter.engine.execution.InvocationInterceptorChain$ValidatingInvocation.proceed(InvocationInterceptorChain.java:125)
    at org.junit.jupiter.engine.extension.TimeoutExtension.intercept(TimeoutExtension.java:132)
    at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestableMethod(TimeoutExtension.java:124)
    at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestMethod(TimeoutExtension.java:74)
    at org.junit.jupiter.engine.execution.ExecutableInvoker$ReflectiveInterceptorCall.lambda$ofVoidMethod$0(ExecutableInvoker.java:115)
    at org.junit.jupiter.engine.execution.ExecutableInvoker.lambda$invoke$0(ExecutableInvoker.java:105)
    at org.junit.jupiter.engine.execution.InvocationInterceptorChain$InterceptedInvocation.proceed(InvocationInterceptorChain.java:104)
    at org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:62)
    at org.junit.jupiter.engine.execution.InvocationInterceptorChain.chainAndInvoke(InvocationInterceptorChain.java:43)
    at org.junit.jupiter.engine.execution.InvocationInterceptorChain.invoke(InvocationInterceptorChain.java:35)
    at org.junit.jupiter.engine.execution.ExecutableInvoker.invoke(ExecutableInvoker.java:104)
    at org.junit.jupiter.engine.execution.ExecutableInvoker.invoke(ExecutableInvoker.java:98)
    at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeTestMethod$6(TestMethodTestDescriptor.java:202)
    at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
    at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.invokeTestMethod(TestMethodTestDescriptor.java:198)
    at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:135)
    at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:69)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$5(NodeTestTask.java:135)
    at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$7(NodeTestTask.java:125)
    at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:135)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:123)
    at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:122)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:80)
    at java.util.ArrayList.forEach(ArrayList.java:1257)
    at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:38)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$5(NodeTestTask.java:139)
    at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$7(NodeTestTask.java:125)
    at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:135)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:123)
    at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:122)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:80)
    at java.util.ArrayList.forEach(ArrayList.java:1257)
    at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:38)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$5(NodeTestTask.java:139)
    at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$7(NodeTestTask.java:125)
    at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:135)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:123)
    at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:122)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:80)
    at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.submit(SameThreadHierarchicalTestExecutorService.java:32)
    at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:57)
    at org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine.execute(HierarchicalTestEngine.java:51)
    at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:229)
    at org.junit.platform.launcher.core.DefaultLauncher.lambda$execute$6(DefaultLauncher.java:197)
    at org.junit.platform.launcher.core.DefaultLauncher.withInterceptedStreams(DefaultLauncher.java:211)
    at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:191)
    at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:128)
    at com.intellij.junit5.JUnit5IdeaTestRunner.startRunnerWithArgs(JUnit5IdeaTestRunner.java:69)
    at com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:33)
    at com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:230)
    at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:58)

because in org.telegram.abilitybots.api.bot.BaseAbilityBot.filterReply(BaseAbilityBot.java:516) it somehow finds a reply which have to handle an update and fails on getting chatId. I set on purpose Flag.MESSAGE everywhere because it seems to me the simplest predicate. As I understand when you give the flow a complicated Predicate like Flag.MESSAGE.and(upd -> upd.getMessage().getText().equalsIgnoreCase(msg)) then it executes this lambda in filterReply so let's start with the simplest case.

To run test I had to install a project with new version and then use it in ability project

addo47 commented 4 years ago

Right right! That's because ReplyFlows attempts to tie the state per-user and per-group, so it calls the getChatId method to find that unique id.

POLL flag check was missing in the getChatId method, so I've added it alongside another test. I've added the poll only test to the ReplyFlowTest and I've changed the assertion to True since the default bot shouldn't handle the reply.

So, technically speaking here, a poll update doesn't advance the state of anything useful and people shouldn't rely on it. I can't think of a way to make this useful since there's no userId or chatId attached to a poll.

Nevertheless, thank for the great find. I've pushed the updates. Please test them out and let me know if there's anything else we've missed!

kortov commented 4 years ago

@addo37 Thank you, now my Reply bot works without exceptions :)