Skip to content
This repository has been archived by the owner on Jul 2, 2021. It is now read-only.

Provide file metadata in storj_finished_upload_cb, not only file id #410

Merged
merged 2 commits into from
Jan 2, 2018

Conversation

kaloyan-raev
Copy link
Contributor

The bridge API returns complete file metadata for uploaded files, but the storj_finished_upload_cb callback passed only the file id to libstorj clients. Thus, clients needed to execute an additional call to the bridge for retrieving metadata of interest like Created Time or HMAC. This causes unnecessary complexity and latency for clients and unnecessary load to the bridge.

The bridge API returns complete file metadata for uploaded files, but
the storj_finished_upload_cb callback passed only the file id to
libstorj clients. Thus, clients needed to execute an additional call to
the bridge for retrieving metadata of interest like Created Time or
HMAC. This causes unnecessary complexity and latency for clients and
unnecessary load to the bridge.
@kaloyan-raev
Copy link
Contributor Author

@braydonf Note that this is a backward incompatible change for libstorj clients as the signature of the storj_finished_upload_cb is changed. If backward compatibility is important, please advise how to do it properly.

I would also appreciate a close review of the memory management as it is very easy to make it wrong.

STORJ_API void storj_free_uploaded_file_info(storj_file_meta_t *file)
{
if (file) {
free((char *)file->id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we need to check if these are allocated before freeing too?

Copy link
Contributor Author

@kaloyan-raev kaloyan-raev Jan 2, 2018

Choose a reason for hiding this comment

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

free(NULL) is noop, isn't it? https://stackoverflow.com/a/1938758/2356099

test/tests.c Outdated
{
assert(handle == NULL);
if (error_code == 0) {
if (strcmp(file_id, "85fb0ed00de1196dc22e0f6d") == 0 ) {
if (strcmp(file->id, "85fb0ed00de1196dc22e0f6d") == 0 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

check if file is NULL

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.

@storj-archived storj-archived deleted a comment from kaloyan-raev Jan 2, 2018
@aleitner aleitner merged commit e46345f into storj-archived:master Jan 2, 2018
@kaloyan-raev kaloyan-raev deleted the file-upload branch January 3, 2018 12:04
@braydonf
Copy link
Contributor

We should remember to bump the so version before this is released.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants