Skip to content

Commit d60e01e

Browse files
andiwandclaude
andauthored
feat(file): let the file name offer a type no content probe can find (#814)
`FileType::markdown` was unreachable by detection: it has no signature, and a content probe for it is a probe for "prose with occasional punctuation", so `detect_by_content` is false and `list_file_types` never offered it. A `.md` came back as `text_file` and the caller had to already know. The name is at the boundary and was being thrown away — `abstract::File` already carries `disk_path`. `file_type_by_name` reads the extension off it and offers the type when the table says no probe can produce it, once the bytes have decoded as text. A name only ever adds a candidate: a `.md` holding a zip is still a zip, and `File::from_memory` has no name, so it still needs `FileType::markdown`. Closes #760. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 9b54f1f commit d60e01e

6 files changed

Lines changed: 98 additions & 22 deletions

File tree

AGENTS.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,9 @@ bytes ─▶ magic/open_strategy ─▶ DecodedFile ─▶ Document ─▶ Eleme
2121
1. **Detect**`internal/magic.cpp` sniffs the head of the file;
2222
`internal/open_strategy.cpp` picks a `FileType` + `DecoderEngine` and builds
2323
the matching `abstract::DecodedFile`. `odr::mimetype` composes the two, so a
24-
zip is named by what is inside it.
24+
zip is named by what is inside it. Only bytes can *claim* a file; a name adds
25+
a candidate the bytes already allow, which is the sole way in for a
26+
signature-less format (`file_type_by_name`, markdown).
2527
2. **Decode** — a document file yields an `abstract::Document`.
2628
3. **Element tree** — a `Document` exposes a root `ElementIdentifier` plus an
2729
`abstract::ElementAdapter`. Public value-semantics handles (`Element`, `Slide`,

CHANGELOG.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,11 @@ The release run heads these entries with the version and opens a fresh
1616

1717
## Unreleased
1818

19+
- A `.md` opened by path decodes as markdown rather than as plain text, and
20+
`list_file_types` offers it. The extension only adds a candidate the bytes
21+
already allow; `File::from_memory` has no name, so it still needs
22+
`FileType::markdown`. Closes #760.
23+
1924
- **Breaking**: a document decrypted from a password-protected package is no
2025
longer savable — every `save` overload throws `UnsupportedOperation`, where it
2126
used to write the content out unprotected. Towards #64.

src/odr/internal/markdown/AGENTS.md

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,15 +16,19 @@ Decoding to a `TextRoot` is the whole argument for a decoder rather than a
1616
markdown→HTML renderer next to `html/text_file.cpp`: the latter would produce
1717
html only, with no element api and nothing for JNI/embind/pybind/ObjC.
1818

19-
## Nothing detects it, and decoding never rejects
19+
## The name detects it, not the content
2020

2121
`detect_by_content` is **false**. Markdown has no signature, and a content probe
2222
for it is a probe for "prose with occasional punctuation" — every plain text
2323
file with a `#` comment or an `*` bullet in it. Sniffing would steal `text_file`
24-
matches and be confidently wrong. The only way in is
25-
`DecodedFile(file, FileType::markdown)`; callers route on the file name, which
26-
is what they already have. **A `.md` still opens as `text_file` by default**,
27-
and that is correct for a format that is by construction valid plain text.
24+
matches and be confidently wrong.
25+
26+
So the file name does it instead: `open_strategy::file_type_by_name` reads the
27+
extension off `File::disk_path` and offers markdown once the bytes have already
28+
decoded as text, ahead of the csv/json/xml probes. A name only ever *adds* a
29+
candidate — a `.md` holding a zip is still a zip — and a file with no name on
30+
disk has no hint, so `File::from_memory` still needs
31+
`DecodedFile(file, FileType::markdown)`.
2832

2933
`NoMarkdownFile` exists only for the `as_markdown_file()` cast: every other
3034
format's `No*File` is also what detection throws, and nothing rejects here —

src/odr/internal/markdown/PLAN.md

Lines changed: 8 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -78,20 +78,14 @@ rule the csv plan sets for the sheet path, and for the same reason:
7878
`Text::content()` is UTF-8 to every binding, so passing legacy bytes through
7979
and letting a browser sort it out is not available to us.
8080

81-
**Detection is by caller, not by content.** `open_strategy` is entirely
82-
content-driven — magic plus speculative probes — and has no extension path
83-
anywhere. Markdown has no signature, and a content probe for it is a probe for
84-
"prose with occasional punctuation", which is every plain text file with a `#`
85-
comment or an `*` bullet in it. Sniffing would steal `text_file` matches and be
86-
confidently wrong. So: `detect_by_content` stays **false**, markdown never joins
87-
the speculative chain in `list_file_types` (`open_strategy.cpp:272`), and the
88-
only way in is `DecodedFile(file, FileType::markdown)` (`file.hpp:341`) via a
89-
new branch in `open_file` next to the text/csv/json ones
90-
(`open_strategy.cpp:146`). Callers route on the filename, which is what they
91-
already have.
92-
93-
The consequence to accept: `.md` still opens as `text_file` by default. That is
94-
correct behaviour for a format that is, by construction, valid plain text.
81+
**Detection is by name, not by content.** Markdown has no signature, and a
82+
content probe for it is a probe for "prose with occasional punctuation", which
83+
is every plain text file with a `#` comment or an `*` bullet in it. Sniffing
84+
would steal `text_file` matches and be confidently wrong, so `detect_by_content`
85+
stays **false** and markdown never joins the speculative chain. The extension
86+
offers it instead, once the bytes have decoded as text — see
87+
[`AGENTS.md`](AGENTS.md) and #760. `File::from_memory` has no name, so it still
88+
needs `DecodedFile(file, FileType::markdown)`.
9589

9690
**There is no `NoMarkdownFile`.** Every other format's exception exists because
9791
detection rejects. Nothing rejects here: md4c is total — any UTF-8 byte

src/odr/internal/open_strategy.cpp

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
#include <odr/internal/common/file.hpp>
1111
#include <odr/internal/common/image_file.hpp>
1212
#include <odr/internal/common/media_file.hpp>
13+
#include <odr/internal/common/path.hpp>
1314
#include <odr/internal/csv/csv_file.hpp>
1415
#include <odr/internal/font/font_file.hpp>
1516
#include <odr/internal/iwork/iwork_file.hpp>
@@ -29,6 +30,7 @@
2930

3031
#include <algorithm>
3132
#include <memory>
33+
#include <optional>
3234

3335
namespace odr::internal {
3436

@@ -50,6 +52,19 @@ template <typename T> auto priority_comparator(const std::vector<T> &priority) {
5052
};
5153
}
5254

55+
/// The type @p file's name claims that no content probe can produce
56+
/// (`detect_by_content == false`), or `unknown`. A name only ever adds a
57+
/// candidate the bytes already allow — it never claims them.
58+
FileType file_type_by_name(const abstract::File &file) {
59+
const std::optional<AbsPath> path = file.disk_path();
60+
if (!path.has_value()) {
61+
return FileType::unknown;
62+
}
63+
const FileType type = file_type_by_file_extension(path->extension());
64+
return capabilities_by_file_type(type).detect_by_content ? FileType::unknown
65+
: type;
66+
}
67+
5368
/// Whether @p file is the ooxml that was asked for. An encrypted one names no
5469
/// inner type until it is decrypted, so it answers for whichever was asked.
5570
bool is_the_requested_ooxml(const ooxml::OfficeOpenXmlFile &file,
@@ -418,6 +433,16 @@ open_strategy::list_file_types(const std::shared_ptr<abstract::File> &file,
418433
} catch (...) {
419434
ODR_VERBOSE(logger, "failed to open as xml");
420435
}
436+
437+
// last, so it outranks the probes: markdown has no signature and every
438+
// text file is valid markdown, leaving the name the only thing that can
439+
// say so
440+
if (const FileType by_name = file_type_by_name(*file);
441+
by_name != FileType::unknown) {
442+
ODR_VERBOSE(logger,
443+
"name says " << file_type_to_string(by_name) << ", adding");
444+
result.push_back(by_name);
445+
}
421446
} catch (...) {
422447
ODR_VERBOSE(logger, "failed to open as text");
423448
}
@@ -525,6 +550,19 @@ open_strategy::open_file(const std::shared_ptr<abstract::File> &file,
525550

526551
auto text = std::make_shared<text::TextFile>(file);
527552

553+
// before the probes: a markdown file parsing as csv is still markdown,
554+
// and the name is the only thing that can say so
555+
if (const FileType by_name = file_type_by_name(*file);
556+
by_name != FileType::unknown) {
557+
ODR_VERBOSE(logger,
558+
"name says " << file_type_to_string(by_name) << ", try it");
559+
try {
560+
return open_file_as(file, by_name, logger);
561+
} catch (...) {
562+
ODR_VERBOSE(logger, "failed to open as what the name says");
563+
}
564+
}
565+
528566
try {
529567
ODR_VERBOSE(logger, "try open as csv");
530568
return std::make_unique<csv::CsvFile>(text);

test/src/odr_test.cpp

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44
#include <odr/html.hpp>
55
#include <odr/odr.hpp>
66

7+
#include <odr/internal/common/path.hpp>
8+
79
#include <test_util.hpp>
810

911
#include <algorithm>
@@ -67,6 +69,33 @@ TEST(odr, types_wpd) {
6769
EXPECT_EQ(mimetype(path, logger), "application/vnd.wordperfect");
6870
}
6971

72+
/// Markdown has no signature, so only the name can offer it.
73+
TEST(odr, types_md) {
74+
const auto logger = Logger::create_stdio("odr-test", LogLevel::verbose);
75+
76+
const auto path = TestData::test_file_path("odr-public/md/feature-matrix.md");
77+
const auto types = list_file_types(path, logger);
78+
ASSERT_FALSE(types.empty());
79+
EXPECT_EQ(types.front(), FileType::text_file);
80+
EXPECT_EQ(types.back(), FileType::markdown);
81+
82+
// the name only adds a candidate; opening by path takes it
83+
EXPECT_EQ(open(path, logger).file_type(), FileType::markdown);
84+
EXPECT_EQ(mimetype(path, logger), "text/markdown");
85+
}
86+
87+
/// A name claims nothing on its own — the bytes still decide.
88+
TEST(odr, a_misnamed_file_is_what_its_bytes_are) {
89+
const auto logger = Logger::create_stdio("odr-test", LogLevel::verbose);
90+
91+
const auto path = TestData::test_file_path("odr-public/odt/about.odt");
92+
EXPECT_EQ(open(path, logger).file_type(), FileType::opendocument_text);
93+
94+
// no name at all, so no hint: the same bytes come back as plain text
95+
const DecodedFile from_memory(File::from_memory("# heading\n"), logger);
96+
EXPECT_EQ(from_memory.file_type(), FileType::text_file);
97+
}
98+
7099
TEST(FileTypeTable, covers_every_file_type_exactly_once) {
71100
const std::vector<FileType> expected = every_file_type();
72101
const std::vector<FileType> actual = all_file_types();
@@ -264,11 +293,15 @@ TEST(FileTypeCapabilities, declaration_matches_the_engines) {
264293
continue;
265294
}
266295

267-
// whatever detection sees, the table has to admit to
296+
// whatever detection sees, the table has to admit to — from the bytes,
297+
// or from the name for a type that has no signature to find
268298
const std::vector<FileType> detected =
269299
list_file_types(test_file.absolute_path, logger);
270300
if (std::ranges::find(detected, type) != std::ranges::end(detected)) {
271-
EXPECT_TRUE(declared.detect_by_content) << test_file.short_path;
301+
EXPECT_TRUE(declared.detect_by_content ||
302+
file_type_by_file_extension(
303+
Path(test_file.absolute_path).extension()) == type)
304+
<< test_file.short_path;
272305
}
273306

274307
// an encrypted OOXML package decodes as its own file type

0 commit comments

Comments
 (0)