Skip to content

Match media file extensions case-insensitively - #1911

Open
h-mayorquin wants to merge 5 commits into
dandi:masterfrom
h-mayorquin:fix_videos_case_bug
Open

Match media file extensions case-insensitively#1911
h-mayorquin wants to merge 5 commits into
dandi:masterfrom
h-mayorquin:fix_videos_case_bug

Conversation

@h-mayorquin

Copy link
Copy Markdown
Contributor

When preparing a re-upload of dandiset 001538 I realized that 150 of its 242 behavioral videos had never reached the archive. The missing ones were exactly the files named .MP4 instead of .mp4. Two places check a suffix against a lowercase whitelist without lowercasing it first. _get_image_series drops the video before organize can symlink it, and DandiFileType.classify types it as a GenericAsset that find_dandi_files then skips on upload. This PR lowercases the suffix in both.

One thing I am not sure about, should the organized copy normalize the extension? Right now I am respecting what the user has, so a .MP4 video still lands as ..._external_file_0.MP4. But maybe you guys want a sanitized version of the extensions on DANDI. What do you think?

A video or image whose extension is uppercase was dropped twice over: `_get_image_series` skipped it so `organize` never symlinked it, and `DandiFileType.classify` typed it as a `GenericAsset` that `find_dandi_files` skips on upload.
@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.29630% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 77.28%. Comparing base (44e0d8b) to head (b01069c).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
dandi/tests/fixtures.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1911      +/-   ##
==========================================
+ Coverage   77.24%   77.28%   +0.04%     
==========================================
  Files          89       89              
  Lines       13264    13288      +24     
==========================================
+ Hits        10246    10270      +24     
  Misses       3018     3018              
Flag Coverage Δ
unittests 77.28% <96.29%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@yarikoptic

Copy link
Copy Markdown
Member

oh hoh hoh! we then at least need somehow to add a check that we do not have "case collisions", as already a file with that name in different casing! In BIDS we added for that https://bids-specification.readthedocs.io/en/stable/common-principles.html#case-collision-intolerance

@yarikoptic yarikoptic added the patch Increment the patch version when merged label Aug 27, 2026

@yarikoptic yarikoptic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"not sure"! Allowing for a cocktail of upper and lower cased extensions, besides collisions (must be avoided) likely to cause users and tool developers pains, as some inventive fellas would then gain .Mp4 and alike and good luck find or glob'ing files of interest!

I didn't check (please do) but in all likeliness, that even if filenames (e.g. entity values, but not entity names) are permitted in BIDS to have upper case letters, I bet extensions are assumed to be lower case!

@CodyCBakerPhD

Copy link
Copy Markdown
Contributor

Personally, I would either

(i) mandate via validation that users MUST normalize their file suffixes to lowercase

or (inclusively [since organization is not a required operation])

(ii) coerce such things via organization like we do with the rest of the filenames

@h-mayorquin

Copy link
Copy Markdown
Contributor Author

As a user I am for (ii).

  1. I didn't notice the missing files the first time that I uploaded. It was only a couple of days later, when I went looking for the video files directly, that I saw they were missing so the current situation might suprise users in a bad way. On the other hand, I feel that asking users to rename their suffixes first is a lot to put on top of something they already experience as a burden.

  2. dandi organize is already the right place for this. We rewrite the media file names there anyway, so it is one more ergonomic change on top of a rename we are already doing.

@yarikoptic this also takes care of the collision concern, as organize disambiguates by the ImageSeries object id and the index of the entry. Two files that differ only in case end up as:

sub-mouse0/sub-mouse0_image/22dd2870-7799-498b-a1ca-da881b006cc4_external_file_0.mp4
sub-mouse0/sub-mouse0_image/22dd2870-7799-498b-a1ca-da881b006cc4_external_file_1.mp4

So the organized output can't collide by construction.

I checked the extension question you raised and you are right, all 44 extensions in the BIDS schema are lower case:

>>> exts = json.load(open(bidsschematools_data / "schema.json"))["objects"]["extensions"]
>>> [v["value"] for v in exts.values() if v["value"] != v["value"].lower()]
[]

@CodyCBakerPhD

Copy link
Copy Markdown
Contributor

I didn't notice the missing files the first time that I uploaded. It was only a couple of days later, when I went looking for the video files directly, that I saw they were missing so the current situation might suprise users in a bad way. On the other hand, I feel that asking users to rename their suffixes first is a lot to put on top of something they already experience as a burden.

I agree we should avoid the silent failure case at all costs, I just think the more proper way to do that is in the validation stage, look for things in a case-insensitive way, and flag, notify, and prevent the case of not all lowercase

I checked the extension question you raised and you are right, all 44 extensions in the BIDS schema are lower case:

Thanks for checking - consistency is good and as more and more modalities are absorbed and DANDI layout itself is retired in favor of full BIDS, this will be the responsibility of the BIDS validator to ensure

@yarikoptic

Copy link
Copy Markdown
Member

so it feels like we need both

  1. (generalizing) warn if not error about files we simply skip and not upload since not matching our "patterns" (upper cased suffixes or just other stuff). Note -- that .dotdirs/ and .dotfiles should just be ignored and no warning on those.
  2. dandi organize to lower-case external filename extensions.

correct?

any takers for any/both of such PRs? (since IMHO independent)

`organize` already renames media files to `<object_id>_external_file_<n>`, so the
source extension was the only part of the original name that survived.  Normalizing
it there makes the `DandiFileType.classify` change unnecessary, so revert it: an
upper-case suffix on a file that was never organized should be reported by the
upload-time warning discussed in the review instead of being silently accepted.
@h-mayorquin

Copy link
Copy Markdown
Contributor Author

Yes, that's correct.

I implemented 2. here. organize now lower-cases the suffix when it builds the organized name, so a .MP4 video lands as:

sub-mouse0/sub-mouse0_image/22dd2870-7799-498b-a1ca-da881b006cc4_external_file_0.mp4

I also reverted the DandiFileType.classify change that was in my first commit. It was making an upper cased suffix upload silently, and 1. is where that should be reported instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

patch Increment the patch version when merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants