Skip to content

Conversation

@gloriacai01
Copy link
Member

@gloriacai01 gloriacai01 commented Jan 2, 2026

associated api changes

@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Jan 2, 2026
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 8, 2026
@gloriacai01 gloriacai01 requested a review from RobertXu January 8, 2026 16:51
default:
return data.ExtDefault
mimeTypeString := data.MimeTypeFromProto(t).String()
strs, err := mime.ExtensionsByType(mimeTypeString)
Copy link
Member

Choose a reason for hiding this comment

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

mime.ExtensionsByType supports many kinds of mime types, but it seems like we're locked into the mime types in this enum since this entire RDK pathway uses datasyncPB.SensorData?

Copy link
Member Author

Choose a reason for hiding this comment

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

oh yea good point 😅 do we even need to update getFileExtFromMimeType function right now then (while we are keeping that as an enum and not changing it to string)?

Copy link
Member

Choose a reason for hiding this comment

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

Thinking out loud here. There could be some edge cases where a user passes in a MIME type, but not a file extension and our "fill in file extension" logic does nothing.

If the user passes in mime_type: image/png and file_ext: nil, we use getFileExtFromMimeType to update file_ext: '.png'.

However, if they pass in mime_type: audio/mp3 and file_ext: nil, then getFileExtFromMimeType return nothing.

We have logic on the APP side to infer file_ext from the file name, but if there's no file name then file_ext remains blank.

So I guess what it ultimately boils down to is: Is it important to automatically fill in file_ext when the field is empty and the file name does not have an extension?

I'm leaning towards no. I suppose there could be issues if someone wanted to filter on file extension? But BinaryDataByFilter doesn't even support filtering by file_ext.

I think I'll briefly bring this up at data-eng to make sure I'm not missing a use case here.

Copy link
Member

Choose a reason for hiding this comment

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

^ Actually, I checked again, and the part about APP inferring file_ext is not accurate. It does infer file_ext, but only to help figure out the MIMEType. It doesn't use the inferred file_ext during the upload process, instead it just calls md.GetFileExtension().

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it important to automatically fill in file_ext when the field is empty and the file name does not have an extension?

im also leaning towards no, especially if we are leaning towards mime type as source of truth.
i guess we may want the file extension if when downloading files we want to append the correct file extension to the file name? but that can be done on the app side too right?

However, if they pass in mime_type: audio/mp3 and file_ext: nil, then getFileExtFromMimeType return nothing.

how would this even happen? i think the only time this rdk code path would get hit is if we are uploading data from a robot, and from there it can only set one of the allowed datasyncPB.MimeType enum values. if user is simply uploading file via api calls, it wouldn't hit this code path, am i understanding this correctly?

Copy link
Member

Choose a reason for hiding this comment

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

when downloading files we want to append the correct file extension to the file name? but that can be done on the app side too right?

Yep, we can make an edit here if needed. There is a call to GetDataCaptureBinaryBlobHandle that uses the file extension, though I don't think we'd want to update the file extension there as well.

if we are uploading data from a robot, and from there it can only set one of the allowed datasyncPB.MimeType enum values

Yep, my bad on missing this! That enum seems to be constraining the MIME types and file extensions that are available in the robot upload scenario. Let me think a little more about this since we may want to change this to support arbitrary MIME types in this scenario.

Copy link
Member

Choose a reason for hiding this comment

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

As discussed in today's #data-eng meeting, there's still a little more exploration to be done. Let me look into SensorMetadata and whether it can be consolidated with UploadMetadata. That potential project seems like something larger than this ticket, so I can approve this ticket if the next step is not a simple one.

@gloriacai01 gloriacai01 requested a review from RobertXu January 13, 2026 18:34
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 13, 2026
return data.ExtPng
case datasyncPB.MimeType_MIME_TYPE_APPLICATION_PCD:
return data.ExtPcd
case datasyncPB.MimeType_MIME_TYPE_VIDEO_MP4:
Copy link
Member

Choose a reason for hiding this comment

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

I believe we want to leave this case in to support the Video tab and the side panel display of file extension.

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry definitely did not mean to also delete this line agh

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 14, 2026
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 14, 2026
@gloriacai01 gloriacai01 requested a review from RobertXu January 14, 2026 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test This pull request is marked safe to test from a trusted zone

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants