From ed5e50b2d28a847c56ca9748e4f709e26d5178cf Mon Sep 17 00:00:00 2001 From: lleeoo Date: Thu, 5 Oct 2023 17:26:48 +0200 Subject: [PATCH 1/3] wip. add version to response header --- src/ralph/api/__init__.py | 3 ++- src/ralph/api/routers/statements.py | 24 ++++++++++++++++++++---- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/src/ralph/api/__init__.py b/src/ralph/api/__init__.py index 23c3e16f4..30eb21ba7 100644 --- a/src/ralph/api/__init__.py +++ b/src/ralph/api/__init__.py @@ -3,7 +3,7 @@ from urllib.parse import urlparse import sentry_sdk -from fastapi import Depends, FastAPI +from fastapi import Depends, Header, FastAPI from ralph.conf import settings @@ -39,6 +39,7 @@ def filter_transactions(event, hint): # pylint: disable=unused-argument before_send_transaction=filter_transactions, ) + app = FastAPI() app.include_router(statements.router) app.include_router(health.router) diff --git a/src/ralph/api/routers/statements.py b/src/ralph/api/routers/statements.py index 9c1d31ad9..c69228bac 100644 --- a/src/ralph/api/routers/statements.py +++ b/src/ralph/api/routers/statements.py @@ -132,6 +132,7 @@ def strict_query_params(request: Request): @router.get("/") # pylint: disable=too-many-arguments, too-many-locals async def get( + response: Response, request: Request, current_user: Annotated[AuthenticatedUser, Depends(get_authenticated_user)], ### @@ -279,6 +280,9 @@ async def get( LRS Specification: https://github.com/adlnet/xAPI-Spec/blob/1.0.3/xAPI-Communication.md#213-get-statements """ + # The LRS MUST include the "X-Experience-API-Version" header in every response + response.headers["X-Experience-API-Version"] = "1.0.3" # TODO: change this ? + # Make sure the limit does not go above max from settings limit = min(limit, settings.RUNSERVER_MAX_SEARCH_HITS_COUNT) @@ -357,7 +361,7 @@ async def get( # NB: There is an unhandled edge case where the total number of results is # exactly a multiple of the "limit", in which case we'll offer an extra page # with 0 results. - response = {} + more_query_parameters = {} if len(query_result.statements) == limit: # Search after relies on sorting info located in the last hit path = request.url.path @@ -370,7 +374,7 @@ async def get( } ) - response.update( + more_query_parameters.update( { "more": ParseResult( scheme="", @@ -383,13 +387,17 @@ async def get( } ) - return {**response, "statements": query_result.statements} + # for statement in query_result.statements: + # statement["version"] = statement.get("version", "1.0.0") + + return {**more_query_parameters, "statements": query_result.statements} @router.put("/", responses=POST_PUT_RESPONSES, status_code=status.HTTP_204_NO_CONTENT) @router.put("", responses=POST_PUT_RESPONSES, status_code=status.HTTP_204_NO_CONTENT) # pylint: disable=unused-argument async def put( + response: Response, current_user: Annotated[AuthenticatedUser, Depends(get_authenticated_user)], statement: LaxStatement, background_tasks: BackgroundTasks, @@ -401,6 +409,10 @@ async def put( LRS Specification: https://github.com/adlnet/xAPI-Spec/blob/1.0.3/xAPI-Communication.md#211-put-statements """ + + # The LRS MUST include the "X-Experience-API-Version" header in every response + response.headers["X-Experience-API-Version"] = "1.0.3" # TODO: change this ? + statement_as_dict = statement.dict(exclude_unset=True) statement_id = str(statement_id) @@ -459,10 +471,10 @@ async def put( @router.post("/", responses=POST_PUT_RESPONSES) @router.post("", responses=POST_PUT_RESPONSES) async def post( + response: Response, current_user: Annotated[AuthenticatedUser, Depends(get_authenticated_user)], statements: Union[LaxStatement, List[LaxStatement]], background_tasks: BackgroundTasks, - response: Response, _=Depends(strict_query_params), ): """Store a set of statements (or a single statement as a single member of a set). @@ -471,6 +483,10 @@ async def post( LRS Specification: https://github.com/adlnet/xAPI-Spec/blob/1.0.3/xAPI-Communication.md#212-post-statements """ + + # The LRS MUST include the "X-Experience-API-Version" header in every response + response.headers["X-Experience-API-Version"] = "1.0.3" # TODO: change this ? + # As we accept both a single statement as a dict, and multiple statements as a list, # we need to normalize the data into a list in all cases before we can process it. if not isinstance(statements, list): From 7fd84d1d3490745f396d894944b5284496bd98f8 Mon Sep 17 00:00:00 2001 From: lleeoo Date: Tue, 10 Oct 2023 11:48:59 +0200 Subject: [PATCH 2/3] add version to statements retrieved by GET + test --- src/ralph/api/routers/statements.py | 10 +++- src/ralph/backends/database/clickhouse.py | 2 +- src/ralph/conf.py | 1 + tests/api/test_statements_get.py | 62 +++++++++++++++++++++++ tests/api/test_statements_post.py | 3 ++ tests/api/test_statements_put.py | 3 ++ 6 files changed, 78 insertions(+), 3 deletions(-) diff --git a/src/ralph/api/routers/statements.py b/src/ralph/api/routers/statements.py index c69228bac..e6e9ea0ba 100644 --- a/src/ralph/api/routers/statements.py +++ b/src/ralph/api/routers/statements.py @@ -387,8 +387,14 @@ async def get( } ) - # for statement in query_result.statements: - # statement["version"] = statement.get("version", "1.0.0") + # Statements returned by an LRS MUST retain the version they are accepted with. + # If they lack a version, the version MUST be set to 1.0.0. + for statement in query_result.statements: + # Delete `version` if it is an empty string. Necessary for clickhouse. + if "version" in statement and not statement["version"]: + statement.pop("version") + statement["version"] = statement.get("version", settings.LRS_DEFAULT_STATEMENT_VERSION) + return {**more_query_parameters, "statements": query_result.statements} diff --git a/src/ralph/backends/database/clickhouse.py b/src/ralph/backends/database/clickhouse.py index a0d85a8dd..bba37ac31 100755 --- a/src/ralph/backends/database/clickhouse.py +++ b/src/ralph/backends/database/clickhouse.py @@ -398,7 +398,7 @@ def query_statements(self, params: RalphStatementsQuery) -> StatementQueryResult # same timestamp, and also avoid sending the same event twice. new_search_after = response[-1]["emission_time"].isoformat() new_pit_id = str(response[-1]["event_id"]) - + return StatementQueryResult( statements=[document["event"] for document in response], search_after=new_search_after, diff --git a/src/ralph/conf.py b/src/ralph/conf.py index ee6eb94fc..c5aa04c0f 100644 --- a/src/ralph/conf.py +++ b/src/ralph/conf.py @@ -381,6 +381,7 @@ class AuthBackends(Enum): RUNSERVER_MAX_SEARCH_HITS_COUNT: int = 100 RUNSERVER_POINT_IN_TIME_KEEP_ALIVE: str = "1m" RUNSERVER_PORT: int = 8100 + LRS_DEFAULT_STATEMENT_VERSION: str = "1.0.0" LRS_RESTRICT_BY_AUTHORITY: bool = False LRS_RESTRICT_BY_SCOPES: bool = False SENTRY_CLI_TRACES_SAMPLE_RATE: float = 1.0 diff --git a/tests/api/test_statements_get.py b/tests/api/test_statements_get.py index 163b4abde..fddd45952 100644 --- a/tests/api/test_statements_get.py +++ b/tests/api/test_statements_get.py @@ -148,12 +148,14 @@ def test_api_statements_get_statements_mine( "timestamp": (datetime.now() - timedelta(hours=1)).isoformat(), "actor": agent_1, "authority": agent_1, + "version": "1.0.0" }, { "id": "72c81e98-1763-4730-8cfc-f5ab34f1bad2", "timestamp": datetime.now().isoformat(), "actor": agent_1, "authority": agent_2, + "version": "1.0.3" }, ] insert_statements_and_monkeypatch_backend(statements) @@ -164,6 +166,7 @@ def test_api_statements_get_statements_mine( headers={"Authorization": f"Basic {credentials_1_bis}"}, ) assert response.status_code == 200 + assert len(response.json()["statements"]) == 2 assert response.json() == {"statements": [statements[1], statements[0]]} # No restriction on "mine" (explicit) : Return all statements @@ -229,10 +232,12 @@ def test_api_statements_get_statements( { "id": "be67b160-d958-4f51-b8b8-1892002dbac6", "timestamp": (datetime.now() - timedelta(hours=1)).isoformat(), + "version": "1.0.0" }, { "id": "72c81e98-1763-4730-8cfc-f5ab34f1bad2", "timestamp": datetime.now().isoformat(), + "version": "1.0.0" }, ] insert_statements_and_monkeypatch_backend(statements) @@ -246,6 +251,39 @@ def test_api_statements_get_statements( assert response.status_code == 200 assert response.json() == {"statements": [statements[1], statements[0]]} + # Test that version is in response header + assert response.headers["X-Experience-API-Version"] == "1.0.3" + +def test_api_statements_get_statements_version( + insert_statements_and_monkeypatch_backend, + auth_credentials +): + """Test that statements are returned with the proper version according to + xAPI specification (existing version or 1.0.0).""" + + statements = [ + { + "id": "be67b160-d958-4f51-b8b8-1892002dbac6", + "timestamp": (datetime.now() + timedelta(hours=1)).isoformat(), + "version": "1.0.3" + }, + { + "id": "72c81e98-1763-4730-8cfc-f5ab34f1bad2", + "timestamp": datetime.now().isoformat(), + }, + ] + insert_statements_and_monkeypatch_backend(statements) + + response = client.get( + "xAPI/statements", headers={"Authorization": f"Basic {auth_credentials}"} + ) + assert response.status_code == 200 + + # Test that statement with existing `version` is unchanged + assert response.json()["statements"][0]["version"] == "1.0.3" + + # Test that statement with no `version` is assigned 1.0.0 + assert response.json()["statements"][1]["version"] == "1.0.0" def test_api_statements_get_statements_ascending( insert_statements_and_monkeypatch_backend, auth_credentials @@ -259,10 +297,12 @@ def test_api_statements_get_statements_ascending( { "id": "be67b160-d958-4f51-b8b8-1892002dbac6", "timestamp": (datetime.now() - timedelta(hours=1)).isoformat(), + "version": "1.0.0" }, { "id": "72c81e98-1763-4730-8cfc-f5ab34f1bad2", "timestamp": datetime.now().isoformat(), + "version": "1.0.0" }, ] insert_statements_and_monkeypatch_backend(statements) @@ -288,10 +328,12 @@ def test_api_statements_get_statements_by_statement_id( { "id": "be67b160-d958-4f51-b8b8-1892002dbac6", "timestamp": (datetime.now() - timedelta(hours=1)).isoformat(), + "version": "1.0.0" }, { "id": "72c81e98-1763-4730-8cfc-f5ab34f1bad2", "timestamp": datetime.now().isoformat(), + "version": "1.0.0" }, ] insert_statements_and_monkeypatch_backend(statements) @@ -340,12 +382,14 @@ def test_api_statements_get_statements_by_agent( "timestamp": datetime.now().isoformat(), "actor": agent_1, "authority": agent_1, + "version": "1.0.0" }, { "id": "72c81e98-1763-4730-8cfc-f5ab34f1bad2", "timestamp": datetime.now().isoformat(), "actor": agent_2, "authority": agent_1, + "version": "1.0.0" }, ] insert_statements_and_monkeypatch_backend(statements) @@ -372,11 +416,13 @@ def test_api_statements_get_statements_by_verb( "id": "be67b160-d958-4f51-b8b8-1892002dbac6", "timestamp": datetime.now().isoformat(), "verb": {"id": "http://adlnet.gov/expapi/verbs/experienced"}, + "version": "1.0.0" }, { "id": "72c81e98-1763-4730-8cfc-f5ab34f1bad2", "timestamp": datetime.now().isoformat(), "verb": {"id": "http://adlnet.gov/expapi/verbs/played"}, + "version": "1.0.0" }, ] insert_statements_and_monkeypatch_backend(statements) @@ -406,11 +452,13 @@ def test_api_statements_get_statements_by_activity( "id": "be67b160-d958-4f51-b8b8-1892002dbac6", "timestamp": datetime.now().isoformat(), "object": activity_0, + "version": "1.0.0" }, { "id": "72c81e98-1763-4730-8cfc-f5ab34f1bad2", "timestamp": datetime.now().isoformat(), "object": activity_1, + "version": "1.0.0" }, ] insert_statements_and_monkeypatch_backend(statements) @@ -445,10 +493,12 @@ def test_api_statements_get_statements_since_timestamp( { "id": "be67b160-d958-4f51-b8b8-1892002dbac6", "timestamp": (datetime.now() - timedelta(hours=1)).isoformat(), + "version": "1.0.0" }, { "id": "72c81e98-1763-4730-8cfc-f5ab34f1bad2", "timestamp": datetime.now().isoformat(), + "version": "1.0.0" }, ] insert_statements_and_monkeypatch_backend(statements) @@ -475,10 +525,12 @@ def test_api_statements_get_statements_until_timestamp( { "id": "be67b160-d958-4f51-b8b8-1892002dbac6", "timestamp": (datetime.now() - timedelta(hours=1)).isoformat(), + "version": "1.0.0" }, { "id": "72c81e98-1763-4730-8cfc-f5ab34f1bad2", "timestamp": datetime.now().isoformat(), + "version": "1.0.0" }, ] insert_statements_and_monkeypatch_backend(statements) @@ -510,22 +562,27 @@ def test_api_statements_get_statements_with_pagination( { "id": "5d345b99-517c-4b54-848e-45010904b177", "timestamp": (datetime.now() - timedelta(hours=4)).isoformat(), + "version": "1.0.0" }, { "id": "be67b160-d958-4f51-b8b8-1892002dbac6", "timestamp": (datetime.now() - timedelta(hours=3)).isoformat(), + "version": "1.0.0" }, { "id": "be67b160-d958-4f51-b8b8-1892002dbac5", "timestamp": (datetime.now() - timedelta(hours=2)).isoformat(), + "version": "1.0.0" }, { "id": "be67b160-d958-4f51-b8b8-1892002dbac4", "timestamp": (datetime.now() - timedelta(hours=1)).isoformat(), + "version": "1.0.0" }, { "id": "72c81e98-1763-4730-8cfc-f5ab34f1bad2", "timestamp": datetime.now().isoformat(), + "version": "1.0.0" }, ] insert_statements_and_monkeypatch_backend(statements) @@ -585,6 +642,7 @@ def test_api_statements_get_statements_with_pagination_and_query( "display": {"en-US": "played"}, }, "timestamp": (datetime.now() - timedelta(hours=2)).isoformat(), + "version": "1.0.0" }, { "id": "be67b160-d958-4f51-b8b8-1892002dbac1", @@ -593,6 +651,7 @@ def test_api_statements_get_statements_with_pagination_and_query( "display": {"en-US": "played"}, }, "timestamp": (datetime.now() - timedelta(hours=1)).isoformat(), + "version": "1.0.0" }, { "id": "72c81e98-1763-4730-8cfc-f5ab34f1bad2", @@ -601,6 +660,7 @@ def test_api_statements_get_statements_with_pagination_and_query( "display": {"en-US": "played"}, }, "timestamp": datetime.now().isoformat(), + "version": "1.0.0" }, ] insert_statements_and_monkeypatch_backend(statements) @@ -640,10 +700,12 @@ def test_api_statements_get_statements_with_no_matching_statement( { "id": "be67b160-d958-4f51-b8b8-1892002dbac6", "timestamp": (datetime.now() - timedelta(hours=1)).isoformat(), + "version": "1.0.0" }, { "id": "72c81e98-1763-4730-8cfc-f5ab34f1bad2", "timestamp": datetime.now().isoformat(), + "version": "1.0.0" }, ] insert_statements_and_monkeypatch_backend(statements) diff --git a/tests/api/test_statements_post.py b/tests/api/test_statements_post.py index 5c743ec37..d186ae649 100644 --- a/tests/api/test_statements_post.py +++ b/tests/api/test_statements_post.py @@ -304,6 +304,9 @@ def test_api_statements_post_statements_list_of_one( response.json(), {"statements": [statement]} ) + # Test that version is in response header + assert response.headers["X-Experience-API-Version"] == "1.0.3" + @pytest.mark.parametrize( "backend", diff --git a/tests/api/test_statements_put.py b/tests/api/test_statements_put.py index 4700c38ab..ecd3ae41f 100644 --- a/tests/api/test_statements_put.py +++ b/tests/api/test_statements_put.py @@ -91,6 +91,9 @@ def test_api_statements_put_single_statement_directly( ) assert response.status_code == 204 + + # Test that version is in response header + assert response.headers["X-Experience-API-Version"] == "1.0.3" es.indices.refresh() From 248f8dcfb419a8e7c4d69157707fdd7c9ffa2769 Mon Sep 17 00:00:00 2001 From: lleeoo Date: Wed, 11 Oct 2023 11:57:44 +0200 Subject: [PATCH 3/3] parametrize xAPI versions --- src/ralph/api/routers/statements.py | 8 ++++---- src/ralph/conf.py | 3 ++- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/ralph/api/routers/statements.py b/src/ralph/api/routers/statements.py index e6e9ea0ba..0c2820e9e 100644 --- a/src/ralph/api/routers/statements.py +++ b/src/ralph/api/routers/statements.py @@ -281,7 +281,7 @@ async def get( https://github.com/adlnet/xAPI-Spec/blob/1.0.3/xAPI-Communication.md#213-get-statements """ # The LRS MUST include the "X-Experience-API-Version" header in every response - response.headers["X-Experience-API-Version"] = "1.0.3" # TODO: change this ? + response.headers["X-Experience-API-Version"] = settings.LRS_HEADER_XAPI_VERSION # Make sure the limit does not go above max from settings limit = min(limit, settings.RUNSERVER_MAX_SEARCH_HITS_COUNT) @@ -393,7 +393,7 @@ async def get( # Delete `version` if it is an empty string. Necessary for clickhouse. if "version" in statement and not statement["version"]: statement.pop("version") - statement["version"] = statement.get("version", settings.LRS_DEFAULT_STATEMENT_VERSION) + statement["version"] = statement.get("version", settings.LRS_GET_STATEMENT_DEFAULT_XAPI_VERSION) return {**more_query_parameters, "statements": query_result.statements} @@ -417,7 +417,7 @@ async def put( """ # The LRS MUST include the "X-Experience-API-Version" header in every response - response.headers["X-Experience-API-Version"] = "1.0.3" # TODO: change this ? + response.headers["X-Experience-API-Version"] = settings.LRS_HEADER_XAPI_VERSION statement_as_dict = statement.dict(exclude_unset=True) statement_id = str(statement_id) @@ -491,7 +491,7 @@ async def post( """ # The LRS MUST include the "X-Experience-API-Version" header in every response - response.headers["X-Experience-API-Version"] = "1.0.3" # TODO: change this ? + response.headers["X-Experience-API-Version"] = settings.LRS_HEADER_XAPI_VERSION # As we accept both a single statement as a dict, and multiple statements as a list, # we need to normalize the data into a list in all cases before we can process it. diff --git a/src/ralph/conf.py b/src/ralph/conf.py index c5aa04c0f..21b846132 100644 --- a/src/ralph/conf.py +++ b/src/ralph/conf.py @@ -381,7 +381,8 @@ class AuthBackends(Enum): RUNSERVER_MAX_SEARCH_HITS_COUNT: int = 100 RUNSERVER_POINT_IN_TIME_KEEP_ALIVE: str = "1m" RUNSERVER_PORT: int = 8100 - LRS_DEFAULT_STATEMENT_VERSION: str = "1.0.0" + LRS_HEADER_XAPI_VERSION: str = "1.0.3" + LRS_GET_STATEMENT_DEFAULT_XAPI_VERSION: str = "1.0.0" LRS_RESTRICT_BY_AUTHORITY: bool = False LRS_RESTRICT_BY_SCOPES: bool = False SENTRY_CLI_TRACES_SAMPLE_RATE: float = 1.0