feat: Query parameter support for entity store query API

[albinsuresh]

Proposed changes

• Add query parameter support for the query API
• Supported parameters:
  • root
  • parent
  • type
• Documentation

Decided not to add the depth parameter as agreed here.
Multiple values for the same parameter is also not supported.

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
• Documentation Update (if none of the other choices apply)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

#3339

Checklist

• I have read the CONTRIBUTING doc
• I have signed the CLA (in all commits with git commit -s)
• I ran cargo fmt as mentioned in CODING_GUIDELINES
• I used cargo clippy as mentioned in CODING_GUIDELINES
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)

Further comments

[codecov]

Codecov Report

Attention: Patch coverage is 96.21451% with 12 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...s/core/tedge_agent/src/http_server/entity_store.rs	96.27%	0 Missing and 7 partials ⚠️
...ates/core/tedge_agent/src/entity_manager/server.rs	0.00%	3 Missing ⚠️
crates/core/tedge_api/src/entity_store.rs	98.41%	2 Missing ⚠️

Additional details and impacted files

📢 Thoughts on this report? Let us know!

[github-actions]

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
582	0	3	582	100	1h36m44.268970999s

[didier-wenzek]

I don't see a depth parameter as useful, and I would not spent time implementing support for that.

In practice the entity tree will be mostly never deep.

[didier-wenzek]

I will be happy to approve, once docs added.

[reubenmiller]

I don't think the putting all of the query parameter mappings in the params is a good idea here (it just makes it a lot harder to use).

Suggested change

	self, params: Dict[str, str] = None, device_name: str = None
	self, entity_type: Optional[str, str] = None, parent: Optional[str, str] = None, root: Optional[str, str] = None, device_name: str = None

Note I'm using entity_type instead of type as type might be a reserved word in python (but check this yourself).

And if people still want to use a dictionary, then we could just add kwargs support to the function signature, and then people can pass this stuff as a dictionary (as that is more python like)

[albinsuresh]

Resolved by 1de9671

[didier-wenzek]

Approved

[reubenmiller]

I'm not to sure about returning 404 when the query does not match any entities.

Generally having no results in a query is not an error. Trying to access a resource that does not exist, would warrant a 404 response.

If you look at the REST API from Cumulocity, the query endpoints (e.g. GET /inventory/managedObjects don't return a 404 NOT FOUND HTTP status code when there are no matching results, it just returns a 200 HTTP status and an empty array in the body (assuming we're talking about json bodies here). Technically speaking, I believe the "resource" in GET /v1/entities is /v1/entities itself...which does exist.

[albinsuresh]

Yeah, this was a confusing case for me as well and my original implementation was retuning empty array for non-existent entity. I changed it to 404 because empty array is returned for a leaf entity that doesn't have any children as well. So, when we get an empty array, we won't be able to differentiate if it's because there are no entities matching the query or if it's a non-existent entity. That's why I changed it to 404 for the latter case so that there is a clear distinction.

But, if the convention is empty array, then I'll change it back.

[reubenmiller]

Yes I would change it back to returning an empty array. This is much easier to deal with for people consuming the API as generally error handling generally wants to split "there are no results" from "the api request failed".

[reubenmiller]

Plus one could use the root query parameter as that returns child entities and the root entity.

[reubenmiller]

And if you want to check the existence of a device, then you should be using GET http://localhost:8000/tedge/entity-store/v1/entities/{entity}

[albinsuresh]

Reverted that change in 1de9671

[reubenmiller]

Item 1: Response body Content-Type on errors.

Currently errors responses are returned to the user with the Content-Type: text/plain. This makes it difficult for simple clients to parse the responses (especially when writing scripts).

Given that the responses of successful queries are json, having the error messages in the same format makes it easier to parse by clients, for example:

Using an invalid value for the type query parameter yields an error, however that error is not visible by the user if they're piping it to jq:

curl -s 'http://localhost:8000/tedge/entity-store/v1/entities?type=asdfasdf' | jq
parse error: Invalid numeric literal at line 1, column 8

So for example if the error message is in json format, it could look something like this:

curl -s 'http://localhost:8000/tedge/entity-store/v1/entities?type=asdfasdf' | jq

{
    "error": "Invalid entity type: asdfasdf"
}

The user is still free to control whether they want to pipe results at all from a failing curl command using curl -f, but still in this case the error message is still lost.

[reubenmiller]

Item 2: Empty query parameter values should be treated as missing query parameter values

Treating empty values as missing values makes it very easy for users to write a simple wrapper around the calls without having to complicated string matching.

For example, the following currently returns an error.

$ curl -s 'http://localhost:8000/tedge/entity-store/v1/entities?type=' -v
Invalid entity type: 

But if we were to treat an empty query parameter value the same as an absent value, then it makes it much easier to build the query parameters.

For example, below shows two implementations that do exactly the same thing (wrap the type filter so the user can provide a given value), and it shows how simple it could be versus if we treat empty values as an error (due to being an invalid type).

# Option 1 (proposal): treat empty query param values as non-existing values
FILTER_BY_TYPE="${1:-}"
curl -s 'http://localhost:8000/tedge/entity-store/v1/entities?type=${FILTER_BY_TYPE}' -v

# Option 2 (current): empty query param values are treated like any explicit values
# vs. having to add a lot of empty string checking when building the query parameters
FILTER_BY_TYPE="${1:-}"
FILTER_ARGS=
if [ -n "$FILTER_BY_TYPE" ]; then
    if [ -n "$FILTER_ARGS" ]; then
        FILTER_ARGS="$FILTER_ARGS&type=${FILTER_BY_TYPE}"
    else
        FILTER_ARGS="type=${FILTER_BY_TYPE}"
    fi
fi
curl -s 'http://localhost:8000/tedge/entity-store/v1/entities?${FILTER_ARGS}' -v

[reubenmiller]

Might be worthwhile mentioned that using the root query parameter includes the root element.

"The topic id of the root node where to start the search from (including the root entity)"

[albinsuresh]

Resolved by 1de9671

[reubenmiller]

Item 3: Deleting a non-existent entity should return 404 status code

Update @albinsuresh pointed out that returning 204 instead of 404 is under some REST API guidelines docs...so I'm ok with that. But the response should return no content (at the moment it is return an empty json array).

To be covered by a follow up PR. #3387

Deleting a resource that does not exist should return a HTTP 404 status code, currently it returns 200, and an empty json array.

curl -XDELETE http://localhost:8000/tedge/entity-store/v1/entities/device/child2// | jq

[reubenmiller]

Item 4: Return the full entity representation

Update: Created a ticket, #3385, to cover the described behaviour in a follow up PR.

It is more consistent if the list of deleted entites objects is returned in its entirety instead of just the @topic-id field.

It is also easier to interpret the returned data, because the fields are clearly labeld.

Current

curl -XDELETE http://localhost:8000/tedge/entity-store/v1/entities/device/child2// | jq
[
  "device/child21/service/service210",
  "device/child210/service/service2100",
  "device/child2100//",
  "device/child210//",
  "device/child21//",
  "device/child22//",
  "device/child2/service/service20",
  "device/child2/service/service21",
  "device/child20//",
  "device/child2//"
]

Expected

curl -XDELETE http://localhost:8000/tedge/entity-store/v1/entities/device/child2// | jq
[
    {
        "@topic-id": "device/child20//",
        "@parent": "device/child2//",
        "@type": "child-device"
    },
  // ..
]

[reubenmiller]

Ok, overall looks nice, just a few minor things to change.

But I'm sorry I just realised that I've been reviewing the entire entity api not just the query api (probably because I was looking at the "register.md" file without a diff view in my editor). So all of the non-query related changes can be ignored and done in a follow up PR (we shouldn't corrupt the scope).

[albinsuresh]

Ok, overall looks nice, just a few minor things to change.

Addressed Items 1 and 2 in 416d499 and 1de9671 respectively. Items 3 and 4 related to the DELETE API would be addressed in a separate PR.

[reubenmiller]

Ok, overall looks nice, just a few minor things to change.

Addressed Items 1 and 2 in 416d499 and 1de9671 respectively. Items 3 and 4 related to the DELETE API would be addressed in a separate PR.

Item 1 is ok, though item 2 is still returning an error when using empty query parameters.

$ curl -s 'http://localhost:8000/tedge/entity-store/v1/entities?type=' -v

*   Trying 127.0.0.1:8000...
* Connected to localhost (127.0.0.1) port 8000 (#0)
> GET /tedge/entity-store/v1/entities?type= HTTP/1.1
> Host: localhost:8000
> User-Agent: curl/7.88.1
> Accept: */*
> 
< HTTP/1.1 400 Bad Request
< content-type: application/json
< content-length: 33
< date: Mon, 10 Feb 2025 14:45:46 GMT
< 
* Connection #0 to host localhost left intact
{"error":"Invalid entity type: "}

Though using empty parent= or root= is fine...so it seems to be caused by the additional type validation that is put on the type query parameter.

[albinsuresh]

Though using empty parent= or root= is fine...so it seems to be caused by the additional type validation that is put on the type query parameter.

Addressed by 2302609

[reubenmiller]

Approved