[media] It is possible to upload multiple images for one media and loose the bucket keys about pyro-api HOT 7 CLOSED

Agreed, I would have a simple suggestion for the key:

• either a concat of device_id and timestamp
• or the better option, the SHA256 hash of the content file

Fully agree that the update should only be made if the upload is successful!

from pyro-api.

We already do the bucket_key upload only if the upload was successfful.

entry = await check_for_media_existence(media_id, current_device.id)
bucket_key = str(hash(datetime.utcnow()))

upload_success = await bucket_service.upload_file(bucket_key=bucket_key,
                                                      file_binary=file.file)
if upload_success is False:
        raise HTTPException(
            status_code=500,
            detail="The upload did not succeed"
        )
entry = dict(**entry)
entry["bucket_key"] = bucket_key
return await crud.update_entry(media, MediaCreation(**entry), media_id)

Agreed for the hashing part.

My point is a bit different, if you read the code above, you will see we don't check there isn't already an uploaded image for a media. If we run the route three times for one unique media, we will have three different images in the CSP.
Don't know exactly what would be the desired behavior. Overwriting?

from pyro-api.

Mmmh, if we have only the content and not the timestamp, I guess we can check if the media content already exists on the bucket?

Actually, this might be a more complex problem: the upload can succeed (ie media content is valid and in the bucket) and we try to overwrite it, but the upload can also fail explicitly (connection issue or something similar) or silently (data corruption during transfer).

So what we could do when we upload a media:

• check if there is a bucket key existing in the table
• if there is, we need to decide whether overwrite is allowed or if we do nothing
• if there isn't, we compute the hashed bucket key
• check if the bucket has a file named the same way
• if there is, we again need to make the same decision
• if there isn't, we upload the file
• once uploaded, we could also try to dl the file, and hash the downloaded version to verify data integrity

As a first step, we could "do nothing" when the file is already in the bucket.

from pyro-api.

Thinking back about that issue, now that data integrity is fully deployed:

• unicity / hashing scheme is currently based solely on the content of the file (no timestamp). This means that if several users try to upload the exact same file (let's say they used the same publicly available image), two things will happen: it will create to abstract media objects pointing to the same bucket key, and only the first uploaded file will remain (it is on purpose to save data transfer and storage).
• if we keep the current hashing scheme, when a media object is deleted, we can't delete the actual bucket file, because we need to check whether another active media uses this content.
• same problem comes when we overwrite the bucket key of a media file: the previous content is kept for now

So my suggestion is simple: add timestamp in the hash.
Inconvenients:

• breaking change: we will have to reset the DB entirely, because current bucket key will not pass the proposed integrity check (because of the hashing scheme modification)
• potential storage & transfer overhead: in production this will most likely never happen, but when uploading images from the web, we might have some overhead

Advantages:

• no more question about whether we can delete a media, so overwriting & deletion of a media bucket key should lead to the actual deletion of the bucket file.

Happy to open a PR if you think we should move forward with this. What do you think?

from pyro-api.

Any opinion on the topic @jeanpasquier75 @martin1tab @fe51 ?

from pyro-api.

This can become an issue once we deploy the API more widely. Any thoughts @jeanpasquier75 @florianriche @fe51 @martin1tab ?

from pyro-api.

Moving this to a later release since we need to decide first what to do in this specific situation (and it doesn't sound critical before receiving any live alerts)

from pyro-api.