Validate Conversation ID #7238

aleronupe · 2020-11-10T23:06:38Z

Proposed changes:

Solves issue Check if conversation ID is valid for all server endpoints #7022
Create a decorator to validate that a conversation ID exists when requests are made to the following endpoints:
- GET /conversations/<conversation_id:path>/story
- POST /conversations/<conversation_id:path>/execute
- POST /conversations/<conversation_id:path>/predict

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)

Co-authored-by: Alexandre Miguel <[email protected]>

Co-authored-by: Rômulo Souza <[email protected]>

…te_conversation_id

Co-authored-by: Alexandre Miguel <[email protected]>

sara-tagger · 2020-11-11T07:00:07Z

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

wochinge · 2020-11-11T09:19:46Z

@JustinaPetr I assigned myself as @aleronupe were already taking here about it

@aleronupe Will give it a review later today or tomorrow 👍

wochinge

Thanks for your PR and sorry for the late review! I really appreciate your initiative and contribution 💪

Can we please also add unit tests for the new changes?

Let me know if I can help somehow!

changelog/7022.improvement.md

rasa/server.py

wochinge · 2020-11-23T13:16:18Z

rasa/server.py

@@ -776,6 +797,7 @@ async def trigger_intent(request: Request, conversation_id: Text) -> HTTPRespons
    @app.post("/conversations/<conversation_id:path>/predict")
    @requires_auth(app, auth_token)
    @ensure_loaded_agent(app)
+    @ensure_conversation_exists(app)
    async def predict(request: Request, conversation_id: Text) -> HTTPResponse:
        try:


out of the scope of your PR / optional: We should actually use the lockstore here as processor.predict_next persists the updated tracker.

…te_conversation_id

…S/rasa into validate_conversation_id

Co-authored-by: Rômulo Souza <[email protected]>

Co-authored-by: Alexandre Miguel <[email protected]>

…_conversation_id

Co-authored-by: Rômulo Souza <[email protected]>

…_conversation_id

aleronupe · 2020-12-05T19:16:29Z

Hello @wochinge, we've just made the required changes and added the unit tests for the new functionalities. Would you please take a look at it?

wochinge

Looks great! Added a few more comments, but nothing critical 👍🏻

changelog/7022.improvement.md

rasa/core/tracker_store.py

rasa/server.py

wochinge · 2020-12-11T15:00:56Z

tests/test_server.py

+async def test_predict_without_conversation_id(rasa_app: SanicASGITestClient):
+    _, response = await rasa_app.post("/conversations/non_existent_id/predict")
+
+    assert response.status == 404


Suggested change

assert response.status == 404

assert response.status == HTTPStatus.NOT_FOUND

wochinge · 2020-12-11T15:01:04Z

tests/test_server.py

+        "/conversations/non_existent_id/execute", json=data
+    )
+
+    assert response.status == 404


Suggested change

assert response.status == 404

assert response.status == HTTPStatus.NOT_FOUND

wochinge · 2020-12-11T15:01:14Z

tests/test_server.py

+
+    _, response = await rasa_app.get(url)
+
+    assert response.status == 404


Suggested change

assert response.status == 404

assert response.status == HTTPStatus.NOT_FOUND

…_conversation_id

Co-authored-by: Alexandre Miguel <[email protected]>

RomuloSouza · 2021-01-12T17:48:33Z

Hi @wochinge, we've made the required changes. Can you please take a look?

…_conversation_id

rasa/core/tracker_store.py

tests/test_server.py

wochinge · 2021-02-03T09:06:08Z

@aleronupe Can we wrap this up? Happy to help 🙌🏻

…onversation_id

Co-authored-by: Alexandre Miguel <[email protected]>

wochinge

💯 thanks a lot @aleronupe

tests/test_server.py

wochinge · 2021-02-04T09:38:13Z

Re the failing windows tests: Windows testing is a bit flakey atm. Re-running the tests should hep 🤞🏻

Co-authored-by: Rômulo Souza <[email protected]>

aleronupe · 2021-02-05T03:32:42Z

@wochinge we've fixed the URL error but windows tests keep failing on a different section of the code. Is there any other change we should make to fix this?

wochinge · 2021-02-05T09:16:37Z

Can you please merge the latest main state into your branch? It should be just a matter of re-running / might be fixed on main already.

…onversation_id

aleronupe · 2021-02-11T13:43:11Z

@wochinge we’ve updated The branch and now all the testes are passing

wochinge · 2021-02-11T13:50:57Z

@aleronupe It's out of date again. Can you somehow make it that I can push to your repo as well? Then i can update the branch as well for you.

…onversation_id

aleronupe · 2021-02-11T15:39:07Z

@wochinge Our repo is on an organization from our university, so I think it won’t be possible, but we’ve just updated the branch

…onversation_id

wochinge · 2021-02-15T09:26:29Z

Merged 🎉 Thanks for your patience!

RomuloSouza and others added 4 commits November 5, 2020 01:16

Create decorator to validate if conversation_id exists

f00cf8b

Co-authored-by: Alexandre Miguel <[email protected]>

Add decorator to required functions

90066ee

Co-authored-by: Rômulo Souza <[email protected]>

Merge branch 'master' of https://github.com/FGA-GCES/rasa into valida…

03a7567

…te_conversation_id

Add changelog file

9955405

Co-authored-by: Alexandre Miguel <[email protected]>

sara-tagger requested a review from JustinaPetr November 11, 2020 07:00

wochinge requested review from wochinge and removed request for JustinaPetr November 11, 2020 09:19

wochinge reviewed Nov 23, 2020

View reviewed changes

aleronupe and others added 8 commits November 26, 2020 15:10

Merge branch 'master' of https://github.com/FGA-GCES/rasa into valida…

faf75c2

…te_conversation_id

Make clean code changes

9572e3a

Merge branch 'validate_conversation_id' of https://github.com/FGA-GCE…

91f5980

…S/rasa into validate_conversation_id

Update changelog file and implement function type

dff15ae

Co-authored-by: Rômulo Souza <[email protected]>

Add method exists in TrackerStore interface

00cec5c

Co-authored-by: Alexandre Miguel <[email protected]>

Merge branch 'master' of https://github.com/RasaHQ/rasa into validate…

9dab7b4

…_conversation_id

Create tests for endpoints that use decorator

95920dc

Co-authored-by: Rômulo Souza <[email protected]>

Merge branch 'master' of https://github.com/RasaHQ/rasa into validate…

9f1560a

…_conversation_id

aleronupe requested a review from wochinge December 5, 2020 19:16

wochinge reviewed Dec 11, 2020

View reviewed changes

RomuloSouza and others added 3 commits January 11, 2021 20:17

Merge branch 'master' of https://github.com/RasaHQ/rasa into validate…

a45e2d5

…_conversation_id

Rewrite comments and change values to constants

ddfd8bd

Co-authored-by: Alexandre Miguel <[email protected]>

Extract agent from request and remove app param

4618354

Co-authored-by: Alexandre Miguel <[email protected]>

RomuloSouza force-pushed the validate_conversation_id branch from 529aeb5 to 4618354 Compare January 12, 2021 00:22

Merge branch 'master' of https://github.com/RasaHQ/rasa into validate…

cd27837

…_conversation_id

wochinge reviewed Jan 22, 2021

View reviewed changes

rasa/core/tracker_store.py Outdated Show resolved Hide resolved

rasa/core/tracker_store.py Outdated Show resolved Hide resolved

tests/test_server.py Outdated Show resolved Hide resolved

tests/test_server.py Outdated Show resolved Hide resolved

Base automatically changed from master to main January 22, 2021 11:15

RomuloSouza and others added 2 commits February 3, 2021 18:54

Merge branch 'main' of https://github.com/RasaHQ/rasa into validate_c…

9586cad

…onversation_id

Fix comments and remove unused code in tests

f43873d

Co-authored-by: Alexandre Miguel <[email protected]>

wochinge approved these changes Feb 4, 2021

View reviewed changes

tests/test_server.py Outdated Show resolved Hide resolved

Fix URL tests

9cd843b

Co-authored-by: Rômulo Souza <[email protected]>

Merge branch 'main' of https://github.com/RasaHQ/rasa into validate_c…

cbb6e7e

…onversation_id

RomuloSouza added 2 commits February 11, 2021 10:54

Merge branch 'main' of https://github.com/RasaHQ/rasa into validate_c…

74a50df

…onversation_id

Merge branch 'main' of https://github.com/RasaHQ/rasa into validate_c…

724cc79

…onversation_id

RomuloSouza added 2 commits February 11, 2021 15:09

Merge branch 'main' of https://github.com/RasaHQ/rasa into validate_c…

da43126

…onversation_id

Merge branch 'main' of https://github.com/RasaHQ/rasa into validate_c…

f5249af

…onversation_id

wochinge merged commit 4abc498 into RasaHQ:main Feb 15, 2021

iurisevero deleted the validate_conversation_id branch February 21, 2021 03:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate Conversation ID #7238

Validate Conversation ID #7238

aleronupe commented Nov 10, 2020 •

edited

Loading

sara-tagger commented Nov 11, 2020

wochinge commented Nov 11, 2020

wochinge left a comment

wochinge Nov 23, 2020

aleronupe commented Dec 5, 2020

wochinge left a comment

wochinge Dec 11, 2020

wochinge Dec 11, 2020

wochinge Dec 11, 2020

RomuloSouza commented Jan 12, 2021

wochinge commented Feb 3, 2021

wochinge left a comment

wochinge commented Feb 4, 2021

aleronupe commented Feb 5, 2021

wochinge commented Feb 5, 2021

aleronupe commented Feb 11, 2021

wochinge commented Feb 11, 2021

aleronupe commented Feb 11, 2021

wochinge commented Feb 15, 2021

	assert response.status == 404
	assert response.status == HTTPStatus.NOT_FOUND


		_, response = await rasa_app.get(url)

		assert response.status == 404

Validate Conversation ID #7238

Validate Conversation ID #7238

Conversation

aleronupe commented Nov 10, 2020 • edited Loading

sara-tagger commented Nov 11, 2020

wochinge commented Nov 11, 2020

wochinge left a comment

Choose a reason for hiding this comment

wochinge Nov 23, 2020

Choose a reason for hiding this comment

aleronupe commented Dec 5, 2020

wochinge left a comment

Choose a reason for hiding this comment

wochinge Dec 11, 2020

Choose a reason for hiding this comment

wochinge Dec 11, 2020

Choose a reason for hiding this comment

wochinge Dec 11, 2020

Choose a reason for hiding this comment

RomuloSouza commented Jan 12, 2021

wochinge commented Feb 3, 2021

wochinge left a comment

Choose a reason for hiding this comment

wochinge commented Feb 4, 2021

aleronupe commented Feb 5, 2021

wochinge commented Feb 5, 2021

aleronupe commented Feb 11, 2021

wochinge commented Feb 11, 2021

aleronupe commented Feb 11, 2021

wochinge commented Feb 15, 2021

aleronupe commented Nov 10, 2020 •

edited

Loading