Skip to content

Conversation

juanjosegarcan
Copy link
Contributor

Adapts asset service to router_read_many approach standardizing and getting by its pagination system

@juanjosegarcan
Copy link
Contributor Author

@GianlucaFicarelli Take a look please. The only problem now is test_get_entity_assets is failing cause now when not entity or assets found is just giving back and empty page instead of 404 error. Should I change the test or give back 404 when empty page?

@GianlucaFicarelli
Copy link
Collaborator

GianlucaFicarelli commented Sep 8, 2025

@GianlucaFicarelli Take a look please. The only problem now is test_get_entity_assets is failing cause now when not entity or assets found is just giving back and empty page instead of 404 error. Should I change the test or give back 404 when empty page?

I was expecting an error when calling /{entity_route}/{entity_id}/assets and entity_id doesn't exist. If we'll need to return 404 we could execute an additional query on the entity when there aren't results.
However, the endpoint /{entity_route}/{entity_id}/derived-from has the same behaviour of returning an empty list, so I think it's ok to return the empty list for the moment since it's simpler, and the client should already know if the entity exists and it's accessible after hitting the /{entity_route}/{entity_id} endpoint.

@juanjosegarcan
Copy link
Contributor Author

@GianlucaFicarelli Take a look please. The only problem now is test_get_entity_assets is failing cause now when not entity or assets found is just giving back and empty page instead of 404 error. Should I change the test or give back 404 when empty page?

I was expecting an error when calling /{entity_route}/{entity_id}/assets and entity_id doesn't exist. If we'll need to return 404 we could execute an additional query on the entity when there aren't results. However, the endpoint /{entity_route}/{entity_id}/derived-from has the same behaviour of returning an empty list, so I think it's ok to return the empty list for the moment since it's simpler, and the client should already know if the entity exists and it's accessible after hitting the /{entity_route}/{entity_id} endpoint.

I changed the failing test to accept an empty list as entity not found

@juanjosegarcan juanjosegarcan self-assigned this Sep 9, 2025
@juanjosegarcan juanjosegarcan marked this pull request as ready for review September 9, 2025 08:00
Copy link
Collaborator

@GianlucaFicarelli GianlucaFicarelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@juanjosegarcan
Copy link
Contributor Author

I'll wait if @eleftherioszisis and @mgeplf have any comment!

Copy link
Collaborator

@mgeplf mgeplf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks reasonable to me

@juanjosegarcan
Copy link
Contributor Author

@eleftherioszisis @GianlucaFicarelli @mgeplf I set back the authorization check for get_assets and added tests to verify authorization

@@ -103,7 +103,7 @@ def asset_directory(db, root_circuit, person_id) -> Asset:
return asset


def test_upload_entity_asset(client, entity):
def test_upload_entity_asset(client, client_user_2, entity):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call on testing non-authorized.
I wonder if the non-authorization tests can be moved to separate tests, and parameterized also on user_context_no_project

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Collaborator

@mgeplf mgeplf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@juanjosegarcan juanjosegarcan merged commit 939fbe4 into main Sep 12, 2025
1 check passed
@juanjosegarcan juanjosegarcan deleted the assets_pagination branch September 12, 2025 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants