Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ bytes ─▢ magic/open_strategy ─▢ DecodedFile ─▢ Document ─▢ Eleme
1. **Detect** β€” `internal/magic.cpp` sniffs the head of the file;
`internal/open_strategy.cpp` picks a `FileType` + `DecoderEngine` and builds
the matching `abstract::DecodedFile`. `odr::mimetype` composes the two, so a
zip is named by what is inside it.
zip is named by what is inside it. Only bytes can *claim* a file; a name adds
a candidate the bytes already allow, which is the sole way in for a
signature-less format (`file_type_by_name`, markdown).
2. **Decode** β€” a document file yields an `abstract::Document`.
3. **Element tree** β€” a `Document` exposes a root `ElementIdentifier` plus an
`abstract::ElementAdapter`. Public value-semantics handles (`Element`, `Slide`,
Expand Down
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ The release run heads these entries with the version and opens a fresh

## Unreleased

- A `.md` opened by path decodes as markdown rather than as plain text, and
`list_file_types` offers it. The extension only adds a candidate the bytes
already allow; `File::from_memory` has no name, so it still needs
`FileType::markdown`. Closes #760.

- **Breaking**: a document decrypted from a password-protected package is no
longer savable β€” every `save` overload throws `UnsupportedOperation`, where it
used to write the content out unprotected. Towards #64.
Expand Down
14 changes: 9 additions & 5 deletions src/odr/internal/markdown/AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,19 @@ Decoding to a `TextRoot` is the whole argument for a decoder rather than a
markdown→HTML renderer next to `html/text_file.cpp`: the latter would produce
html only, with no element api and nothing for JNI/embind/pybind/ObjC.

## Nothing detects it, and decoding never rejects
## The name detects it, not the content

`detect_by_content` is **false**. Markdown has no signature, and a content probe
for it is a probe for "prose with occasional punctuation" β€” every plain text
file with a `#` comment or an `*` bullet in it. Sniffing would steal `text_file`
matches and be confidently wrong. The only way in is
`DecodedFile(file, FileType::markdown)`; callers route on the file name, which
is what they already have. **A `.md` still opens as `text_file` by default**,
and that is correct for a format that is by construction valid plain text.
matches and be confidently wrong.

So the file name does it instead: `open_strategy::file_type_by_name` reads the
extension off `File::disk_path` and offers markdown once the bytes have already
decoded as text, ahead of the csv/json/xml probes. A name only ever *adds* a
candidate β€” a `.md` holding a zip is still a zip β€” and a file with no name on
disk has no hint, so `File::from_memory` still needs
`DecodedFile(file, FileType::markdown)`.

`NoMarkdownFile` exists only for the `as_markdown_file()` cast: every other
format's `No*File` is also what detection throws, and nothing rejects here β€”
Expand Down
22 changes: 8 additions & 14 deletions src/odr/internal/markdown/PLAN.md
Original file line number Diff line number Diff line change
Expand Up @@ -78,20 +78,14 @@ rule the csv plan sets for the sheet path, and for the same reason:
`Text::content()` is UTF-8 to every binding, so passing legacy bytes through
and letting a browser sort it out is not available to us.

**Detection is by caller, not by content.** `open_strategy` is entirely
content-driven β€” magic plus speculative probes β€” and has no extension path
anywhere. Markdown has no signature, and a content probe for it is a probe for
"prose with occasional punctuation", which is every plain text file with a `#`
comment or an `*` bullet in it. Sniffing would steal `text_file` matches and be
confidently wrong. So: `detect_by_content` stays **false**, markdown never joins
the speculative chain in `list_file_types` (`open_strategy.cpp:272`), and the
only way in is `DecodedFile(file, FileType::markdown)` (`file.hpp:341`) via a
new branch in `open_file` next to the text/csv/json ones
(`open_strategy.cpp:146`). Callers route on the filename, which is what they
already have.

The consequence to accept: `.md` still opens as `text_file` by default. That is
correct behaviour for a format that is, by construction, valid plain text.
**Detection is by name, not by content.** Markdown has no signature, and a
content probe for it is a probe for "prose with occasional punctuation", which
is every plain text file with a `#` comment or an `*` bullet in it. Sniffing
would steal `text_file` matches and be confidently wrong, so `detect_by_content`
stays **false** and markdown never joins the speculative chain. The extension
offers it instead, once the bytes have decoded as text β€” see
[`AGENTS.md`](AGENTS.md) and #760. `File::from_memory` has no name, so it still
needs `DecodedFile(file, FileType::markdown)`.

**There is no `NoMarkdownFile`.** Every other format's exception exists because
detection rejects. Nothing rejects here: md4c is total β€” any UTF-8 byte
Expand Down
38 changes: 38 additions & 0 deletions src/odr/internal/open_strategy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <odr/internal/common/file.hpp>
#include <odr/internal/common/image_file.hpp>
#include <odr/internal/common/media_file.hpp>
#include <odr/internal/common/path.hpp>
#include <odr/internal/csv/csv_file.hpp>
#include <odr/internal/font/font_file.hpp>
#include <odr/internal/iwork/iwork_file.hpp>
Expand All @@ -29,6 +30,7 @@

#include <algorithm>
#include <memory>
#include <optional>

namespace odr::internal {

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

/// The type @p file's name claims that no content probe can produce
/// (`detect_by_content == false`), or `unknown`. A name only ever adds a
/// candidate the bytes already allow β€” it never claims them.
FileType file_type_by_name(const abstract::File &file) {
const std::optional<AbsPath> path = file.disk_path();
if (!path.has_value()) {
return FileType::unknown;
}
const FileType type = file_type_by_file_extension(path->extension());
return capabilities_by_file_type(type).detect_by_content ? FileType::unknown
: type;
}

/// Whether @p file is the ooxml that was asked for. An encrypted one names no
/// inner type until it is decrypted, so it answers for whichever was asked.
bool is_the_requested_ooxml(const ooxml::OfficeOpenXmlFile &file,
Expand Down Expand Up @@ -418,6 +433,16 @@ open_strategy::list_file_types(const std::shared_ptr<abstract::File> &file,
} catch (...) {
ODR_VERBOSE(logger, "failed to open as xml");
}

// last, so it outranks the probes: markdown has no signature and every
// text file is valid markdown, leaving the name the only thing that can
// say so
if (const FileType by_name = file_type_by_name(*file);
by_name != FileType::unknown) {
ODR_VERBOSE(logger,
"name says " << file_type_to_string(by_name) << ", adding");
result.push_back(by_name);
}
} catch (...) {
ODR_VERBOSE(logger, "failed to open as text");
}
Expand Down Expand Up @@ -525,6 +550,19 @@ open_strategy::open_file(const std::shared_ptr<abstract::File> &file,

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

// before the probes: a markdown file parsing as csv is still markdown,
// and the name is the only thing that can say so
if (const FileType by_name = file_type_by_name(*file);
by_name != FileType::unknown) {
ODR_VERBOSE(logger,
"name says " << file_type_to_string(by_name) << ", try it");
try {
return open_file_as(file, by_name, logger);
} catch (...) {
ODR_VERBOSE(logger, "failed to open as what the name says");
}
}

try {
ODR_VERBOSE(logger, "try open as csv");
return std::make_unique<csv::CsvFile>(text);
Expand Down
37 changes: 35 additions & 2 deletions test/src/odr_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
#include <odr/html.hpp>
#include <odr/odr.hpp>

#include <odr/internal/common/path.hpp>

#include <test_util.hpp>

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

/// Markdown has no signature, so only the name can offer it.
TEST(odr, types_md) {
const auto logger = Logger::create_stdio("odr-test", LogLevel::verbose);

const auto path = TestData::test_file_path("odr-public/md/feature-matrix.md");
const auto types = list_file_types(path, logger);
ASSERT_FALSE(types.empty());
EXPECT_EQ(types.front(), FileType::text_file);
EXPECT_EQ(types.back(), FileType::markdown);

// the name only adds a candidate; opening by path takes it
EXPECT_EQ(open(path, logger).file_type(), FileType::markdown);
EXPECT_EQ(mimetype(path, logger), "text/markdown");
}

/// A name claims nothing on its own β€” the bytes still decide.
TEST(odr, a_misnamed_file_is_what_its_bytes_are) {
const auto logger = Logger::create_stdio("odr-test", LogLevel::verbose);

const auto path = TestData::test_file_path("odr-public/odt/about.odt");
EXPECT_EQ(open(path, logger).file_type(), FileType::opendocument_text);

// no name at all, so no hint: the same bytes come back as plain text
const DecodedFile from_memory(File::from_memory("# heading\n"), logger);
EXPECT_EQ(from_memory.file_type(), FileType::text_file);
}

TEST(FileTypeTable, covers_every_file_type_exactly_once) {
const std::vector<FileType> expected = every_file_type();
const std::vector<FileType> actual = all_file_types();
Expand Down Expand Up @@ -264,11 +293,15 @@ TEST(FileTypeCapabilities, declaration_matches_the_engines) {
continue;
}

// whatever detection sees, the table has to admit to
// whatever detection sees, the table has to admit to β€” from the bytes,
// or from the name for a type that has no signature to find
const std::vector<FileType> detected =
list_file_types(test_file.absolute_path, logger);
if (std::ranges::find(detected, type) != std::ranges::end(detected)) {
EXPECT_TRUE(declared.detect_by_content) << test_file.short_path;
EXPECT_TRUE(declared.detect_by_content ||
file_type_by_file_extension(
Path(test_file.absolute_path).extension()) == type)
<< test_file.short_path;
}

// an encrypted OOXML package decodes as its own file type
Expand Down
Loading