Check if port is int and raise exception #8575

jppgomes · 2021-04-29T12:19:28Z

Proposed changes:

issue ValueError: invalid literal for int() with base 10 #8309

Status (please check what you already did):

added some tests for the functionality
updated the documentation
updated the changelog (please check changelog for instructions)
reformat files using black (please check Readme for instructions)

sara-tagger · 2021-04-30T06:00:08Z

Thanks for submitting a pull request 🚀 @ArjaanBuijk will take a look at it as soon as possible ✨

ArjaanBuijk · 2021-04-30T15:07:53Z

@ancalita ,
Would you be able to review this update?

ancalita

Thanks for your contribution! 🚀
In addition to adding a unit test to reproduce the Sentry error which checks that the added code catches it and raises RasaException, please also add a changelog entry at rasa/changelog by following the README instructions at that location.

ancalita · 2021-05-04T12:45:12Z

rasa/core/brokers/pika.py

@@ -169,6 +170,10 @@ async def _connect(self) -> aio_pika.RobustConnection:
        last_exception = None
        for _ in range(self._connection_attempts):
            try:
+
+                if not isinstance(self.port, int):
+                    raise RasaException("Port has to be a integer.")


I think the RasaException can be raised a bit earlier - for example in the constructor L75 where you can wrap the integer casting in a try-except block:

try: self.port = int(port) except ValueError as e: raise RasaException("Port could not be converted to integer.") from e

You can try this by adding a new unit test at rasa/tests/core/test_broker.py and reproducing the Sentry error behaviour to see if this try-except block catches it.

ok, thanks for the tips, would it be something like this?

That was fast ⚡
It's looking in the right direction - I would also add an extra assertion after pytest.raises:

with pytest.raises(RasaException) as e: # to do assert "Port could not be converted to integer." in str(e.value)

…ES/rasa into 8309/check_if_port_is_int

CLAassistant · 2021-05-04T15:46:39Z

All committers have signed the CLA.

ancalita · 2021-05-04T16:37:21Z

tests/core/test_broker.py

@@ -310,6 +311,23 @@ async def test_no_pika_logs_if_no_debug_mode(caplog: LogCaptureFixture):
        for record in caplog.records
    )

+    broker = PikaEventBroker(
+        "host", "username", "password", retry_delay_in_seconds=1, connection_attempts=1
+    )


I'm not exactly sure what these new lines are for 🤔 If you added by mistake, could you please remove?

@jppgomes This also needs to be resolved.

ancalita · 2021-05-04T16:37:56Z

tests/core/test_broker.py

+    )
+
+
+def test_raise_exception_port(monkeypatch: MonkeyPatch):


If there is no need for MonkeyPatch, this argument can be removed.

yes.. sorry, fixed!!

ancalita · 2021-05-04T16:42:31Z

tests/core/test_broker.py

+def test_raise_exception_port(monkeypatch: MonkeyPatch):
+
+    with pytest.raises(RasaException):
+        PikaEventBroker(


I'm aware you cannot see the full stacktrace from Sentry, not sure why it got clipped, but I would actually use exactly what gets called further up in the stacktrace: the EventBroker factory from here.
You can create an instance of EndpointConfig which you can pass as argument to the broker factory:
EndpointConfig(username="username", password="password", type="pika", port="PORT")

In addition, note of caution - the EventBroker factory is async so you'll need to use the keyword await when calling it. As well as making the test async too.

ok, i'll do this

@ancalita can you check if it is right now?

@jppgomes Sure, you could also re-request review by clicking on the arrows-in-a-circle icon to the right of the picture avatar under Reviewers section (top right).
Does the test pass?
Please also add this assertion check in the test:

with pytest.raises(RasaException) as e: ... assert "Port could not be converted to integer." in str(e.value)

changelog/8575.bugfix.md

ancalita · 2021-05-05T12:44:08Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ jppgomes
❌ C1300283 Joao Pedro Gomes Cabral Ferreira
C1300283 Joao Pedro Gomes Cabral Ferreira seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@jppgomes There appears to be an issue with the license agreement not signed - did you commit changes from different devices maybe? You need to make sure that you've set an email address on your machine - this is an useful guide.
After this is done, you might need to redo the commits to attach the email (via rebase).

…ES/rasa into 8309/check_if_port_is_int

ancalita · 2021-05-06T08:15:33Z

tests/core/test_broker.py

@@ -22,6 +22,7 @@
 from rasa.core.brokers.kafka import KafkaEventBroker
 from rasa.core.brokers.pika import PikaEventBroker, DEFAULT_QUEUE_NAME
 from rasa.core.brokers.sql import SQLEventBroker
+from rasa.shared.exceptions import RasaException


i just noticed there is actually an existing import from the same module in L27 - could you add the , RasaException to L27 and remove duplicate import in L25?

ancalita

Looks good, thank you 💯

ancalita · 2021-05-14T10:42:49Z

Hey @jppgomes I see you've run into some flakey CI test failures, it's fine to restart the jobs: go to Details of any failed check and you'll see somewhere on the right side Re-run jobs, select Re-run all jobs and that will restart the process. If you have this issue again, let me know and I can have a look.

ancalita · 2021-05-17T12:00:15Z

Hi @jppgomes could you please update branch with the base branch, that's something i can't do - I'll monitor the CI tests and restart if necessary.

jppgomes · 2021-05-17T12:36:36Z

ok @ancalita

jppgomes · 2021-05-17T12:59:03Z

@ancalita it looks like everything is fine now

jppgomes added 4 commits April 29, 2021 09:16

Check if port is int and raise exception

5b63023

import rasa excpetion

de29352

remove unused import

c7126a6

Merge branch 'main' into 8309/check_if_port_is_int

9b69e1d

sara-tagger requested a review from ArjaanBuijk April 30, 2021 06:00

ArjaanBuijk requested a review from ancalita April 30, 2021 15:07

ArjaanBuijk removed their request for review April 30, 2021 15:08

ancalita requested changes May 4, 2021

View reviewed changes

jppgomes and others added 2 commits May 4, 2021 12:45

Adding tests and get exception

eb0aca4

Merge branch '8309/check_if_port_is_int' of https://github.com/FGA-GC…

6c14405

…ES/rasa into 8309/check_if_port_is_int

remove unused vars from test

4f27931

ancalita reviewed May 4, 2021

View reviewed changes

changelog/8575.bugfix.md Outdated Show resolved Hide resolved

jppgomes added 2 commits May 4, 2021 14:43

remove parameter from test

bd0de44

call factory and fix changelog

0234050

jppgomes requested a review from ancalita May 5, 2021 13:38

jppgomes and others added 5 commits May 5, 2021 10:40

assert on test

6e18dd8

Merge branch 'main' into 8309/check_if_port_is_int

e087d6f

remove accidentally added lines

1789025

as e on with raise exception to get msg

939bac2

Merge branch '8309/check_if_port_is_int' of https://github.com/FGA-GC…

fdef0c9

…ES/rasa into 8309/check_if_port_is_int

ancalita reviewed May 6, 2021

View reviewed changes

duplicate import

b21a447

jppgomes requested a review from ancalita May 6, 2021 13:47

ancalita approved these changes May 7, 2021

View reviewed changes

Merge branch 'main' into 8309/check_if_port_is_int

4fa14e7

Merge branch 'main' into 8309/check_if_port_is_int

e05c77e

ancalita merged commit 327851b into RasaHQ:main May 17, 2021

ErickGiffoni deleted the 8309/check_if_port_is_int branch August 5, 2021 19:42

wochinge mentioned this pull request Nov 8, 2021

ValueError: invalid literal for int() with base 10 #8309

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check if port is int and raise exception #8575

Check if port is int and raise exception #8575

jppgomes commented Apr 29, 2021 •

edited

Loading

sara-tagger commented Apr 30, 2021

ArjaanBuijk commented Apr 30, 2021

ancalita left a comment

ancalita May 4, 2021

jppgomes May 4, 2021

ancalita May 4, 2021

CLAassistant commented May 4, 2021 •

edited

Loading

ancalita May 4, 2021

ancalita May 5, 2021

ancalita May 4, 2021

jppgomes May 4, 2021

ancalita May 4, 2021 •

edited

Loading

ancalita May 4, 2021

jppgomes May 4, 2021

jppgomes May 5, 2021

ancalita May 5, 2021

ancalita commented May 5, 2021

ancalita May 6, 2021

jppgomes May 6, 2021

ancalita left a comment

ancalita commented May 14, 2021

ancalita commented May 17, 2021

jppgomes commented May 17, 2021

jppgomes commented May 17, 2021

Check if port is int and raise exception #8575

Check if port is int and raise exception #8575

Conversation

jppgomes commented Apr 29, 2021 • edited Loading

sara-tagger commented Apr 30, 2021

ArjaanBuijk commented Apr 30, 2021

ancalita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented May 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ancalita May 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ancalita commented May 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ancalita left a comment

Choose a reason for hiding this comment

ancalita commented May 14, 2021

ancalita commented May 17, 2021

jppgomes commented May 17, 2021

jppgomes commented May 17, 2021

jppgomes commented Apr 29, 2021 •

edited

Loading

CLAassistant commented May 4, 2021 •

edited

Loading

ancalita May 4, 2021 •

edited

Loading