diff --git a/src/agent_toolkit/forestadmin/agent_toolkit/services/serializers/json_api_serializer.py b/src/agent_toolkit/forestadmin/agent_toolkit/services/serializers/json_api_serializer.py index f015a6ad9..08f04536a 100644 --- a/src/agent_toolkit/forestadmin/agent_toolkit/services/serializers/json_api_serializer.py +++ b/src/agent_toolkit/forestadmin/agent_toolkit/services/serializers/json_api_serializer.py @@ -1,6 +1,7 @@ from ast import literal_eval from datetime import date, datetime, time from typing import Any, Callable, Dict, List, Optional, Tuple, Union, cast +from urllib.parse import quote from uuid import uuid4 from forestadmin.agent_toolkit.forest_logger import ForestLogger @@ -84,16 +85,18 @@ def _serialize_one( ) -> DumpedResult: projection = projection if projection is not None else self.projection pk_value = self._get_id(collection, data) + + encoded_pk_value = quote(pk_value, safe="") if isinstance(pk_value, str) else pk_value ret = { "data": { "id": pk_value, "attributes": {}, - "links": {"self": f"/forest/{collection.name}/{pk_value}"}, + "links": {"self": f"/forest/{collection.name}/{encoded_pk_value}"}, "relationships": {}, "type": collection.name, }, "included": [], - "links": {"self": f"/forest/{collection.name}/{pk_value}"}, + "links": {"self": f"/forest/{collection.name}/{encoded_pk_value}"}, } first_level_projection = [*projection.relations.keys(), *projection.columns] @@ -109,7 +112,7 @@ def _serialize_one( key, data, cast(RelationAlias, collection.schema["fields"][key]), - f"/forest/{collection.name}/{pk_value}", + f"/forest/{collection.name}/{encoded_pk_value}", ) ret["data"]["relationships"][key] = relation if included is not None and not self._is_in_included(ret["included"], included): @@ -207,10 +210,13 @@ def _serialize_to_one_relationships( continue included_attributes[key] = self._serialize_value(value, foreign_collection.schema["fields"][key]) + id_ = self._get_id(foreign_collection, data) + encoded_id = quote(id_, safe="") if isinstance(id_, str) else id_ + included = { - "id": self._get_id(foreign_collection, data), + "id": id_, "links": { - "self": f"/forest/{foreign_collection.name}/{self._get_id(foreign_collection, data)}", + "self": f"/forest/{foreign_collection.name}/{encoded_id}", }, "type": foreign_collection.name, } diff --git a/src/agent_toolkit/tests/resources/collections/test_crud.py b/src/agent_toolkit/tests/resources/collections/test_crud.py index 3b314106c..8412ea01a 100644 --- a/src/agent_toolkit/tests/resources/collections/test_crud.py +++ b/src/agent_toolkit/tests/resources/collections/test_crud.py @@ -270,6 +270,29 @@ def _create_collections(cls): }, ) + # str as pk for url encoding + + cls.collection_str_pk = cls._create_collection( + "StrPK", + { + "pk": { + "column_type": PrimitiveType.STRING, + "is_primary_key": True, + "type": FieldType.COLUMN, + "is_read_only": False, + "default_value": None, + "enum_values": None, + "filter_operators": set([Operator.EQUAL, Operator.IN]), + "is_sortable": True, + "validations": [], + }, + "name": { + "column_type": "String", + "type": "Column", + }, + }, + ) + @classmethod def setUpClass(cls) -> None: cls.loop = asyncio.new_event_loop() @@ -293,6 +316,7 @@ def setUpClass(cls) -> None: "tag": cls.collection_tag, # for uuid "author": cls.collection_author, + "StrPK": cls.collection_str_pk, } cls.datasource_composite.add_datasource(cls.datasource) @@ -1349,6 +1373,27 @@ def test_list_errors_jsonapi_error(self): "status": 500, } + request = RequestCollection( + RequestMethod.GET, + self.collection_order, + query={ + "collection_name": "order", + "timezone": "Europe/Paris", + "fields[order]": "id,cost", + }, + headers={}, + client_ip="127.0.0.1", + ) + crud_resource = CrudResource( + self.datasource_composite, + self.datasource, + self.permission_service, + self.ip_white_list_service, + self.options, + ) + with patch.object(self.collection_order, "list", new_callable=AsyncMock, return_value=mock_orders): + response = self.loop.run_until_complete(crud_resource.list(request)) + # count def test_count(self): request = RequestCollection( diff --git a/src/agent_toolkit/tests/services/serializers/test_jsonapi.py b/src/agent_toolkit/tests/services/serializers/test_jsonapi.py index 3343ea7ed..51a935455 100644 --- a/src/agent_toolkit/tests/services/serializers/test_jsonapi.py +++ b/src/agent_toolkit/tests/services/serializers/test_jsonapi.py @@ -456,6 +456,49 @@ def setUpClass(cls) -> None: } ) + cls.collection_str_pk = Collection("StrPK", cls.datasource) # type:ignore + cls.collection_str_pk.add_fields( + { + "pk": Column( + column_type=PrimitiveType.STRING, + is_primary_key=True, + type=FieldType.COLUMN, + is_read_only=False, + default_value=None, + enum_values=None, + filter_operators=set([Operator.EQUAL, Operator.IN]), + is_sortable=True, + validations=[], + ), + "name": Column(column_type="String", type="Column"), + "relation_pk": Column(column_type="String", type="Column"), + "relation": ManyToOne( + foreign_collection="StrPKRelation", + foreign_key="relation_pk", + foreign_key_target="pk", + type=FieldType.MANY_TO_ONE, + ), + } + ) + + cls.collection_str_pk_relation = Collection("StrPKRelation", cls.datasource) # type:ignore + cls.collection_str_pk_relation.add_fields( + { + "pk": Column( + column_type=PrimitiveType.STRING, + is_primary_key=True, + type=FieldType.COLUMN, + is_read_only=False, + default_value=None, + enum_values=None, + filter_operators=set([Operator.EQUAL, Operator.IN]), + is_sortable=True, + validations=[], + ), + "name": Column(column_type="String", type="Column"), + } + ) + cls.datasource.add_collection(cls.collection_order) cls.datasource.add_collection(cls.collection_order_products) cls.datasource.add_collection(cls.collection_product) @@ -464,6 +507,8 @@ def setUpClass(cls) -> None: cls.datasource.add_collection(cls.collection_picture) cls.datasource.add_collection(cls.collection_comment) cls.datasource.add_collection(cls.collection_all_types) + cls.datasource.add_collection(cls.collection_str_pk) + cls.datasource.add_collection(cls.collection_str_pk_relation) class TestJsonApiDeserializer(TestJsonApi): @@ -1253,3 +1298,58 @@ def test_should_ignore_polymorphic_many_to_one_if_type_is_unknown(self): ], }, ) + + def test_string_primary_keys_should_be_url_encoded(self): + serializer = JsonApiSerializer(self.datasource, Projection("pk", "name")) + record = {"pk": "hello/world", "name": "hello world"} + dumped = serializer.serialize(record, self.collection_str_pk) + self.assertEqual( + dumped, + { + "data": { + "type": "StrPK", + "id": "hello/world", + "attributes": {"pk": "hello/world", "name": "hello world"}, + "links": {"self": "/forest/StrPK/hello%2Fworld"}, + }, + "links": {"self": "/forest/StrPK/hello%2Fworld"}, + }, + ) + + def test_string_foreign_keys_should_be_url_encoded_so_foreign_pk(self): + serializer = JsonApiSerializer( + self.datasource, Projection("pk", "name", "relation_pk", "relation:pk", "relation:name") + ) + record = { + "pk": "hello/world", + "name": "hello world", + "relation_pk": "hello/other/people", + "relation": {"pk": "hello/other/people", "name": "hello other people"}, + } + dumped = serializer.serialize(record, self.collection_str_pk) + self.assertEqual( + dumped, + { + "data": { + "type": "StrPK", + "id": "hello/world", + "attributes": {"pk": "hello/world", "name": "hello world", "relation_pk": "hello/other/people"}, + "links": {"self": "/forest/StrPK/hello%2Fworld"}, + "relationships": { + "relation": { + "data": {"id": "hello/other/people", "type": "StrPKRelation"}, + "links": {"related": {"href": "/forest/StrPK/hello%2Fworld/relationships/relation"}}, + } + }, + }, + "links": {"self": "/forest/StrPK/hello%2Fworld"}, + "included": [ + { + "id": "hello/other/people", + "links": {"self": "/forest/StrPKRelation/hello%2Fother%2Fpeople"}, + "type": "StrPKRelation", + "attributes": {"pk": "hello/other/people", "name": "hello other people"}, + } + ], + }, + ) diff --git a/src/django_agent/forestadmin/django_agent/urls.py b/src/django_agent/forestadmin/django_agent/urls.py index da2765286..bab908fac 100644 --- a/src/django_agent/forestadmin/django_agent/urls.py +++ b/src/django_agent/forestadmin/django_agent/urls.py @@ -1,5 +1,5 @@ from django.conf import settings -from django.urls import path +from django.urls import path, re_path from .views import actions, authentication, capabilities, charts, crud, crud_related, index, native_query, stats @@ -52,24 +52,24 @@ # stats path(f"{prefix}forest/stats/", stats.stats, name="stats"), # crud related - path( - f"{prefix}forest///relationships//count", + re_path( + f"{prefix}forest/(?P[^/]+)/(?P.*)/relationships/(?P[^/]+)/count", crud_related.count, name="crud_related_count", ), - path( - f"{prefix}forest///relationships/.csv", + re_path( + f"{prefix}forest/(?P[^/]+)/(?P.*)/relationships/(?P[^/]+).csv", crud_related.csv, name="crud_related_csv", ), - path( - f"{prefix}forest///relationships/", + re_path( + f"{prefix}forest/(?P[^/]+)/(?P.*)/relationships/(?P[^/]+)", crud_related.list_, name="crud_related_list", ), # crud path(f"{prefix}forest/.csv", crud.csv, name="crud_csv"), path(f"{prefix}forest//count", crud.count, name="crud_count"), - path(f"{prefix}forest//", crud.detail, name="crud_detail"), + re_path(f"{prefix}forest/(?P[^/]+)/(?P.+)", crud.detail, name="crud_detail"), path(f"{prefix}forest/", crud.list_, name="crud_list"), ]