From 9482075cfb9530747a2d03dd2071aacfc50d82dd Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 9 Aug 2023 18:25:35 +0930 Subject: [PATCH] fix: child events must use unique event IDs * pulls event ID generation into get_event_id(), taking care not to modify how parent event IDs are generated. * overrides get_event_id() for child events by using the child_id as part of the UUID namespace_key. * updates tests to check that child events and their parent event use different event IDs, and the affected problem_check multiple-question test data --- ...ck(server,multiple_questions,correct).json | 6 +-- ...(server,multiple_questions,incorrect).json | 6 +-- ...r,multiple_questions,partial_correct).json | 6 +-- .../xapi/tests/test_transformers.py | 5 ++ .../processors/xapi/transformer.py | 46 +++++++++++++++---- 5 files changed, 51 insertions(+), 18 deletions(-) diff --git a/event_routing_backends/processors/xapi/tests/fixtures/expected/problem_check(server,multiple_questions,correct).json b/event_routing_backends/processors/xapi/tests/fixtures/expected/problem_check(server,multiple_questions,correct).json index 0b5e3797..218b0799 100644 --- a/event_routing_backends/processors/xapi/tests/fixtures/expected/problem_check(server,multiple_questions,correct).json +++ b/event_routing_backends/processors/xapi/tests/fixtures/expected/problem_check(server,multiple_questions,correct).json @@ -47,7 +47,7 @@ } }, { - "id": "b2f56b44-07f9-5044-85d1-c1dbc550d35f", + "id": "2cb89608-ab9c-5129-ae6e-4d7d2f9cde68", "result": {"response": "blue", "success": true}, "version": "1.0.3", "actor": { @@ -113,7 +113,7 @@ } }, { - "id": "b2f56b44-07f9-5044-85d1-c1dbc550d35f", + "id": "d0b94156-376d-5012-be91-c6235d3a0770", "result": {"response": "['a piano', 'a guitar']", "success": true}, "version": "1.0.3", "actor": { @@ -179,7 +179,7 @@ } }, { - "id": "b2f56b44-07f9-5044-85d1-c1dbc550d35f", + "id": "a51f4090-8ebf-5644-a669-a18768523eab", "result": {"response": "a chair", "success": true}, "version": "1.0.3", "actor": { diff --git a/event_routing_backends/processors/xapi/tests/fixtures/expected/problem_check(server,multiple_questions,incorrect).json b/event_routing_backends/processors/xapi/tests/fixtures/expected/problem_check(server,multiple_questions,incorrect).json index db914659..bc68cba1 100644 --- a/event_routing_backends/processors/xapi/tests/fixtures/expected/problem_check(server,multiple_questions,incorrect).json +++ b/event_routing_backends/processors/xapi/tests/fixtures/expected/problem_check(server,multiple_questions,incorrect).json @@ -49,7 +49,7 @@ } }, { - "id": "b2f56b44-07f9-5044-85d1-c1dbc550d35f", + "id": "2cb89608-ab9c-5129-ae6e-4d7d2f9cde68", "result": {"response": "yellow", "success": false}, "version": "1.0.3", "actor": { @@ -115,7 +115,7 @@ } }, { - "id": "b2f56b44-07f9-5044-85d1-c1dbc550d35f", + "id": "d0b94156-376d-5012-be91-c6235d3a0770", "result": {"response": "['a window']", "success": false}, "version": "1.0.3", "actor": { @@ -181,7 +181,7 @@ } }, { - "id": "b2f56b44-07f9-5044-85d1-c1dbc550d35f", + "id": "a51f4090-8ebf-5644-a669-a18768523eab", "result": {"response": "a bookshelf", "success": false}, "version": "1.0.3", "actor": { diff --git a/event_routing_backends/processors/xapi/tests/fixtures/expected/problem_check(server,multiple_questions,partial_correct).json b/event_routing_backends/processors/xapi/tests/fixtures/expected/problem_check(server,multiple_questions,partial_correct).json index 8a79bd86..5d5018b8 100644 --- a/event_routing_backends/processors/xapi/tests/fixtures/expected/problem_check(server,multiple_questions,partial_correct).json +++ b/event_routing_backends/processors/xapi/tests/fixtures/expected/problem_check(server,multiple_questions,partial_correct).json @@ -47,7 +47,7 @@ } }, { - "id": "b2f56b44-07f9-5044-85d1-c1dbc550d35f", + "id": "2cb89608-ab9c-5129-ae6e-4d7d2f9cde68", "result": {"response": "blue", "success": true}, "version": "1.0.3", "actor": { @@ -113,7 +113,7 @@ } }, { - "id": "b2f56b44-07f9-5044-85d1-c1dbc550d35f", + "id": "d0b94156-376d-5012-be91-c6235d3a0770", "result": {"response": "['a piano', 'a guitar']", "success": true}, "version": "1.0.3", "actor": { @@ -179,7 +179,7 @@ } }, { - "id": "b2f56b44-07f9-5044-85d1-c1dbc550d35f", + "id": "a51f4090-8ebf-5644-a669-a18768523eab", "result": {"response": "a bookshelf", "success": false}, "version": "1.0.3", "actor": { diff --git a/event_routing_backends/processors/xapi/tests/test_transformers.py b/event_routing_backends/processors/xapi/tests/test_transformers.py index 369d54f5..12a34649 100644 --- a/event_routing_backends/processors/xapi/tests/test_transformers.py +++ b/event_routing_backends/processors/xapi/tests/test_transformers.py @@ -42,9 +42,14 @@ def compare_events(self, transformed_event, expected_event): if isinstance(expected_event, list): assert isinstance(transformed_event, list) assert len(transformed_event) == len(expected_event) + event_ids = set() for idx, e_event in enumerate(expected_event): + event_ids.add(transformed_event[idx].id) self._compare_events(transformed_event[idx], e_event) + # Ensure a unique event ID was applied for the parent + child events + assert len(event_ids) == len(transformed_event) + # Compare single events else: self._compare_events(transformed_event, expected_event) diff --git a/event_routing_backends/processors/xapi/transformer.py b/event_routing_backends/processors/xapi/transformer.py index 3be1843c..d75761ff 100644 --- a/event_routing_backends/processors/xapi/transformer.py +++ b/event_routing_backends/processors/xapi/transformer.py @@ -52,23 +52,31 @@ def base_transform(self, transformed_event): Transform the fields that are common for all events. """ transformed_event = super().base_transform(transformed_event) - actor = self.get_actor() - event_timestamp = self.get_timestamp() transformed_event.update({ - 'actor': actor, + 'id': self.get_event_id(), + 'actor': self.get_actor(), 'context': self.get_context(), - 'timestamp': event_timestamp, + 'timestamp': self.get_timestamp(), }) - transformed_event['actor'] = self.get_actor() - transformed_event['context'] = self.get_context() - transformed_event['timestamp'] = self.get_timestamp() + return transformed_event + + def get_event_id(self): + """ + Generates the UUID for this event. + + Uses the actor, event timestamp, and verb to generate a UUID for this event + which will be the same even if this event is re-processed. + Returns: + UUID + """ # Warning! changing anything in these 2 lines or changing the "base_uuid" can invalidate # billions of rows in the database. Please have a community discussion first before introducing # any change in generation of UUID. + actor = self.get_actor() + event_timestamp = self.get_timestamp() uuid_str = f'{actor.to_json()}-{event_timestamp}' - transformed_event['id'] = get_uuid5(self.verb.to_json(), uuid_str) # pylint: disable=no-member - return transformed_event + return get_uuid5(self.verb.to_json(), uuid_str) # pylint: disable=no-member def get_actor(self): """ @@ -261,6 +269,26 @@ def __init__(self, parent, child_id, *args, **kwargs): self.parent = parent self.child_id = child_id + def get_event_id(self): + """ + Generates the UUID for this event. + + Uses the actor, event timestamp, verb, and child_id to generate a UUID for this child event + which ensures a unique event ID for each child event which also differs from the parent event, + which will be the same even if this event is re-processed. + + Returns: + UUID + """ + # Warning! changing anything in these 2 lines or changing the "base_uuid" can invalidate + # billions of rows in the database. Please have a community discussion first before introducing + # any change in generation of UUID. + actor = self.get_actor() + event_timestamp = self.get_timestamp() + name = f'{actor.to_json()}-{event_timestamp}' + namespace_key = f'{self.verb.to_json()}-{self.child_id}' + return get_uuid5(namespace_key, name) + def get_context(self): """ Returns the context for the xAPI transformed child event.