Merge lp://staging/~jderose/filestore/no-more-ext into lp://staging/filestore

Proposed by Jason Gerard DeRose
Status: Merged
Merged at revision: 157
Proposed branch: lp://staging/~jderose/filestore/no-more-ext
Merge into: lp://staging/filestore
Diff against target: 735 lines (+79/-302)
2 files modified
filestore.py (+45/-112)
test_filestore.py (+34/-190)
To merge this branch: bzr merge lp://staging/~jderose/filestore/no-more-ext
Reviewer Review Type Date Requested Status
dmedia Dev Pending
Review via email: mp+71299@code.staging.launchpad.net

Description of the change

I've generally chosen pragmatism over purity in the design of dmedia. A good example of this is using a base32-encoded content-hash (_id) so that the canonical filename can be constructed in the simplest way possible.

Another example was including a file extension on the canonical file name. The rationale was:

 1) Some software doesn't work without the file extension, which is used for type determination

 2) Including the file extension would allow a vanilla web server like Apache to server the files with the correct Content-Type

However, I've changed my mind on the file extension, for a few reasons, the biggest reason being that with the file extension we don't have a true canonical filename anymore, which makes it difficult to make these critical code paths as robust as they need to be. Using the untrusted file extension to construct a filename is also a potential security vulnerability (although carefully validated the extension).

I may add similar functionality back eventually, but not on the canonical filenames. If a content-hash based name is needed within the filestore with an extension, I feel the correct route is to symlink to the canonical file. So for example the canonical filename would be:

  files/V3/ODQNYR62VFR4KGIKS3IE4SQKCYI7GIYLPD3UA4RDSCR7HG

And the symlink name would be:

  symlinks/V3/ODQNYR62VFR4KGIKS3IE4SQKCYI7GIYLPD3UA4RDSCR7HG.txt

The canonical filename is critical for data durability, is not the place for software compatibility hacks.

To post a comment you must log in.
Revision history for this message
Michael Chang (thenewme91) wrote :

Are the only people who've got this running so far devs? Will already-imported files "disappear" as a result of this change because we're looking for "<filename>" instead of "<filename>.<ext>"?

Revision history for this message
Jason Gerard DeRose (jderose) wrote :

Ah, very good question! Thus far it has only been devs running dmedia... I've tried to make it clear that dmedia is not yet production ready, that there will be changes that will break your current filestores and database.

Revision history for this message
Jason Gerard DeRose (jderose) wrote :

Oops, didn't mean to post that just yet...

But as there are so many non-devs looking at dmedia these days, I added a healthy disclaimer on the dmedia project page: https://launchpad.net/dmedia

Revision history for this message
Jason Gerard DeRose (jderose) wrote :

Michael, also, thanks for taking the time to look at this! Good luck on your move! I promise not to bug you for at least 48 hours :P

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches