Feature: modification and creation date in the aggregate messages #473

1yam · 2023-09-11T11:56:06Z

Problem :
To avoid fetching all the messages to find the creation/modification time of a specific aggregate entry it will be useful to have this info directly in the aggregate message

Solution:
Update '/api/v0/aggregates' Endpoint

param :

with_info: bool default : False

From :

{
  "address": "0xa1B3bb7d2332383D96b7796B908fB7f7F3c2Be10",
  "data": {
    "key1": ...,
    "key2": ...,
  }
}

To:

{
  "address": "0xa1B3bb7d2332383D96b7796B908fB7f7F3c2Be10",
  "data": {
    "key1": ...,
    "key2": ...,
  }
  "info": {
    "key1": {
      "created": ...,
      "last_updated": ...,
      "original_item_hash": ...,
      "last_update_item_hash: ...,
    },
    "key2": ...
  }
}

Fixes #470.

odesenfans

Use one query instead of two.

odesenfans · 2023-09-11T12:23:57Z

src/aleph/db/accessors/aggregates.py

 from sqlalchemy import (
+    join,


Unused import. Note that PyCharm can remove unused imports for you (Code > Optimize Imports)

odesenfans · 2023-09-11T12:35:26Z

src/aleph/db/accessors/aggregates.py

@@ -38,13 +37,32 @@ def get_aggregates_by_owner(
    return session.execute(select_stmt).all()  # type: ignore


+def get_aggregates_info_by_owner(


If I understand correctly you perform 2 DB accesses, one for the aggregate content and the other one for the info. Performing several DB accesses instead of one should usually be avoided for the following reasons:

Performance: more round trips to the DB -> more time spent in establishing connections, looking for the correct rows, etc.

Atomicity: aggregates may be added/deleted between your two queries, so you may end up with less/more entries in info than you'd expect. If you perform one query, you won't have this problem as the query will happen as a single DB transaction.

It would be better to just add a with_info parameter to get_aggregates_by_owner and use different queries for each case, but always selecting the content field.

Typing-wise, you can add a typing.overload to tell mypy what to expect:

AggregateContentWithInfo = Tuple[str, Dict[str, Any], dt.datetime, dt.datetime, str, str] AggregateContent = Tuple[str, Dict[str, Any]] @overload def get_aggregates_by_owner(..., with_info: Literal[True]) -> Iterable[AggregateContentWithInfo]): ... @overload def get_aggregates_by_owner(..., with_info: Literal[False]) -> Iterable[AggregateContent]): ...

odesenfans · 2023-09-11T12:36:15Z

src/aleph/web/controllers/aggregates.py

+
+            info[aggregate_key] = {
+                "created": created.isoformat(),  # Convert datetime to ISO format
+                "last_updated": last_updated.isoformat(),  # Use actual 'last_updated' value


Why "Use actual 'last_updated' value"? I don't get this comment.

odesenfans · 2023-09-11T12:37:20Z

src/aleph/web/controllers/aggregates.py

@@ -34,6 +39,8 @@ async def address_aggregate(request: web.Request) -> web.Response:
    """

    address = request.match_info["address"]
+    with_info_param = request.query.get("with_info", "false")
+    with_info = with_info_param.lower() == "true"


Add with_info to AggregatesQueryParams just above. We already have a Pydantic model, no need to perform custom input validation.

odesenfans · 2023-09-12T09:50:45Z

src/aleph/web/controllers/aggregates.py

+        if not aggregates:
+            return web.HTTPNotFound(text="No aggregate found for this address")


odesenfans · 2023-09-12T09:52:57Z

src/aleph/web/controllers/aggregates.py

+    info = {}
+    if query_params.with_info:
+        for result in aggregates:
+            (
+                aggregate_key,
+                content,
+                created,
+                last_updated,
+                original_item_hash,
+                last_update_item_hash,
+            ) = result
+
+            if isinstance(created, dt.datetime):
+                created = created.isoformat()
+            if isinstance(last_updated, dt.datetime):
+                last_updated = last_updated.isoformat()
+
+            info[aggregate_key] = {
+                "created": created,
+                "last_updated": last_updated,
+                "original_item_hash": original_item_hash,
+                "last_update_item_hash": last_update_item_hash,
+            }
+        output["info"] = info


The way you're doing this, you're iterating twice over the aggregates list. The output dict can be generated with only one iteration.

odesenfans · 2023-09-12T09:53:37Z

tests/api/test_aggregates.py

@@ -26,12 +26,17 @@ def make_uri(address: str) -> str:
    return AGGREGATES_URI.format(address=address)


-async def get_aggregates(api_client, address: str, **params) -> aiohttp.ClientResponse:
+async def get_aggregates(
+    api_client, address: str, with_info: str, **params


Why str? Use bool. aiohttptakes care of casting iirc.

odesenfans · 2023-09-12T09:54:33Z

tests/api/test_aggregates.py


    assert address == aggregates["address"]
    assert aggregates["data"]["test_key"] == {"a": 1, "b": 2}
    assert aggregates["data"]["test_target"] == {"a": 1, "b": 2}
    assert aggregates["data"]["test_reference"] == {"a": 1, "b": 2, "c": 3, "d": 4}
+    assert aggregates["info"]["test_reference"] is not None
+    print(aggregates)


Remove the print statement.

odesenfans · 2023-09-12T09:55:04Z

tests/api/test_aggregates.py


    assert address == aggregates["address"]
    assert aggregates["data"]["test_key"] == {"a": 1, "b": 2}
    assert aggregates["data"]["test_target"] == {"a": 1, "b": 2}
    assert aggregates["data"]["test_reference"] == {"a": 1, "b": 2, "c": 3, "d": 4}
+    assert aggregates["info"]["test_reference"] is not None


Could you do more than just checking if the key is present? Like, compare to the content of the aggregate message.

* Fix aggregate filter by keys

* Fix regression introduced by (#473) * Fix aggregate filter by keys * Feature: * Add param to only return the value of an aggregate by its key * add Tests

odesenfans requested changes Sep 11, 2023

View reviewed changes

odesenfans requested changes Sep 12, 2023

View reviewed changes

1yam requested a review from odesenfans September 12, 2023 10:22

1yam added 6 commits October 6, 2023 15:55

Feature: Get info on aggregates endpoint

d2c8a3e

Fix: error on sqlalchemy request

af8eac1

Fix: some mypy error

ea03bcb

Reformat + fix mypy

658b723

Fix: mypy error test

aaa6858

Fix: Last mypy error + refactor

4a45fd8

odesenfans approved these changes Oct 6, 2023

View reviewed changes

odesenfans merged commit 5e7c10a into aleph-im:master Oct 6, 2023
2 checks passed

odesenfans deleted the 1yam-better-aggregates branch October 6, 2023 14:20

aliel added a commit that referenced this pull request Feb 19, 2024

Fix regression introduced by (#473)

ef08b9d

* Fix aggregate filter by keys

aliel mentioned this pull request Feb 19, 2024

Fix aggregate filter by keys #556

Merged

aliel added a commit that referenced this pull request Feb 19, 2024

Fix regression introduced by (#473)

ff3efb7

* Fix aggregate filter by keys

hoh pushed a commit that referenced this pull request Feb 20, 2024

Fix regression introduced by (#473)

3a0165a

* Fix aggregate filter by keys

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: modification and creation date in the aggregate messages #473

Feature: modification and creation date in the aggregate messages #473

1yam commented Sep 11, 2023 •

edited by odesenfans

Loading

odesenfans left a comment

odesenfans Sep 11, 2023

odesenfans Sep 11, 2023

odesenfans Sep 11, 2023

odesenfans Sep 11, 2023

odesenfans Sep 12, 2023

odesenfans Sep 12, 2023

odesenfans Sep 12, 2023

odesenfans Sep 12, 2023

odesenfans Sep 12, 2023

		@@ -38,13 +37,32 @@ def get_aggregates_by_owner(
		return session.execute(select_stmt).all() # type: ignore


		def get_aggregates_info_by_owner(

		if not aggregates:
		return web.HTTPNotFound(text="No aggregate found for this address")

Feature: modification and creation date in the aggregate messages #473

Feature: modification and creation date in the aggregate messages #473

Conversation

1yam commented Sep 11, 2023 • edited by odesenfans Loading

odesenfans 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

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

Choose a reason for hiding this comment

1yam commented Sep 11, 2023 •

edited by odesenfans

Loading