BDMS 55: asset coverage#97
Conversation
the sample table is no longer queried when adding observations, so the simplified adder is now used
cannot have two foreign key constraints to the same (sample) table
Codecov Report❌ Patch coverage is
|
| bucket = get_storage_bucket() | ||
| records = [add_signed_url(ai, bucket) for ai in records] | ||
|
|
||
| records = [add_signed_url(ai, bucket) for ai in records] |
There was a problem hiding this comment.
the thing_id guard was there on purpose. I do not want to generate signed urls if the thing_id is not set.
There was a problem hiding this comment.
If a user sends a GET request and doesn't filter for thing_id, but an asset is associated with a thing, should the response for that record have a signed URL? (and responses for records with no association will not have a signed URL)
There was a problem hiding this comment.
Essentially
def transformer(records: list[Asset]):
records = [add_signed_url(ai, bucket) if ai.things != [] else ai for ai in records]
return recordsThere was a problem hiding this comment.
I implemented this in my latest push. One of my concerns is that this could be slow because calling ai.things does a lazy load, I think. I imagine this could be improved and made faster, but this is a solution that comes to mind initially.
I am also checking for thing associations in /asset/{asset_id}
There was a problem hiding this comment.
no. the signed_url should only be generated when the user specifies a thing_id via the get_assets function. The original code was sufficient
There is no reason to generate signed urls when listing all assets. There is a reason to generate signed urls when listing assets for a given thing_id because this is used by the front end to display a gallery of images all at once. This is the only case in which signer urls should be generated for a list of assets. A signed url is always generated when retrieving assets individually
| records = [add_signed_url(ai, bucket) for ai in records] | ||
|
|
||
| records = [ | ||
| add_signed_url(ai, bucket) if ai.things != [] else ai for ai in records |
There was a problem hiding this comment.
this is not the pythonic way to test for an empty list. typically this would be
add_signed_url(ai, bucket) if ai.things else ai ...
or
add_signed_url(ai, bucket) if ai.things and len(ai.things)>0 else ai ...in this case however the original code should be used
def transformer(records: list[Asset]):
if thing_id is not None:
bucket = get_storage_bucket()
records = [add_signed_url(ai, bucket) for ai in records]
return records
jacob-a-brown
requested review from
jirhiker
and removed request for
chasetmartin,
jirhiker and
ksmuczynski
jirhiker
left a comment
jirhiker
left a comment
There was a problem hiding this comment.
One last small change. Otherwise looking great
| @@ -110,7 +151,6 @@ def transformer(records: list[Asset]): | |||
| if thing_id is not None: | |||
There was a problem hiding this comment.
Lets add a comment explaining this behavior e.g.
Do not generate signed urls when listing ALL assets. There is a reason to generate signed urls when listing assets for a given `thing_id` because this is used by the front end to display a gallery of images all at once. This is the only case in which signed urls should be generated for a list of assets. A signed url is always generated when retrieving assets individually
There was a problem hiding this comment.
This note was added at the beginning of the GET section of the file since it applies to both GET /asset and GET /asset/{asset_id}
3 participants
Why
This PR addresses the following problem / context:
assetrouter should be fully covered with GET, GET by ID, POST, PATCH, and DELETE endpointsHow
Implementation summary - the following was changed / added / removed:
nameandlabel. Is this correct? If they're updated in the database does anything need to change in GCS?Notes
Any special considerations, workarounds, or follow-up work to note?
/asset/{asset_id}/removecorrectly to remove a blob from GCS? I'm using the asset'surito remove.