Harden ImportBase: open read-only and fail fast on an unreadable file.
ImportBase opened the input with a default std::fstream (in|out), which had two consequences: a missing file silently produced an empty module (no error), and a present-but-read-only file failed to open and also loaded as empty. Open the stream read-only (std::ios::in) instead, and expose is_open(). System::Load now builds the importer first, checks is_open(), and throws "cannot open file: <path>" before creating the module — so a failed load surfaces as `load failed: …` and leaves no empty module behind. A read-only but present file now loads correctly. Flip the test that pinned the old silent-empty behaviour to assert the clean failure (error + no module created). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -21,14 +21,8 @@ System::~System()
|
|||||||
|
|
||||||
void System::Load(std::string module_name, std::string file_name, ImportType type)
|
void System::Load(std::string module_name, std::string file_name, ImportType type)
|
||||||
{
|
{
|
||||||
|
// Build the importer first, based on the import type.
|
||||||
ImportBase *imp;
|
ImportBase *imp;
|
||||||
Module *mod = nullptr;
|
|
||||||
Parts *prts = nullptr;
|
|
||||||
|
|
||||||
// Creation or retrieval of the module.
|
|
||||||
mod = mods->merge(module_name);
|
|
||||||
|
|
||||||
// Parsing of the file based on the import type.
|
|
||||||
if (type == ImportType::IMPORT_MENTOR)
|
if (type == ImportType::IMPORT_MENTOR)
|
||||||
{
|
{
|
||||||
imp = new ImportMentor(file_name);
|
imp = new ImportMentor(file_name);
|
||||||
@@ -43,7 +37,17 @@ void System::Load(std::string module_name, std::string file_name, ImportType typ
|
|||||||
{
|
{
|
||||||
throw std::runtime_error("Unknown import type");
|
throw std::runtime_error("Unknown import type");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Fail fast on a missing/unreadable file, before touching the module table,
|
||||||
|
// so a failed load never leaves behind an empty module.
|
||||||
|
if (!imp->is_open())
|
||||||
|
{
|
||||||
|
delete imp;
|
||||||
|
throw std::runtime_error("cannot open file: " + file_name);
|
||||||
|
}
|
||||||
|
|
||||||
|
// Creation or retrieval of the module, then parse into it.
|
||||||
|
Module *mod = mods->merge(module_name);
|
||||||
imp->parse(mod->signals);
|
imp->parse(mod->signals);
|
||||||
prts = imp->parts();
|
mod->add(imp->parts());
|
||||||
mod->add(prts);
|
|
||||||
}
|
}
|
||||||
@@ -27,11 +27,22 @@ public:
|
|||||||
*
|
*
|
||||||
* @param file_name Name of the file to be imported.
|
* @param file_name Name of the file to be imported.
|
||||||
*/
|
*/
|
||||||
ImportBase(std::string file_name) : file_lines(std::fstream(file_name))
|
ImportBase(std::string file_name) : file_lines(file_name, std::ios::in)
|
||||||
{
|
{
|
||||||
prts = new Parts();
|
prts = new Parts();
|
||||||
};
|
};
|
||||||
|
|
||||||
|
/**
|
||||||
|
* @brief Whether the input file was opened successfully.
|
||||||
|
*
|
||||||
|
* Opened read-only, so this is false only when the file is genuinely
|
||||||
|
* missing or unreadable (a read-only but present file still opens).
|
||||||
|
* System::Load checks it to fail fast instead of producing an empty module.
|
||||||
|
*
|
||||||
|
* @return true if the file stream is open.
|
||||||
|
*/
|
||||||
|
bool is_open() const { return file_lines.is_open(); }
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* @brief Pure virtual method for parsing the file.
|
* @brief Pure virtual method for parsing the file.
|
||||||
*
|
*
|
||||||
|
|||||||
@@ -1,6 +1,7 @@
|
|||||||
#include <doctest/doctest.h>
|
#include <doctest/doctest.h>
|
||||||
|
|
||||||
#include "core/app/load.hpp"
|
#include "core/app/load.hpp"
|
||||||
|
#include "core/domain/modules.hpp"
|
||||||
#include "core/domain/system.hpp"
|
#include "core/domain/system.hpp"
|
||||||
|
|
||||||
#include <cstdio>
|
#include <cstdio>
|
||||||
@@ -50,15 +51,13 @@ TEST_CASE("load_module imports, drops singletons and reports counts") {
|
|||||||
std::remove(path);
|
std::remove(path);
|
||||||
}
|
}
|
||||||
|
|
||||||
TEST_CASE("load_module on a missing file currently succeeds empty (no throw)") {
|
TEST_CASE("load_module fails cleanly on a missing file") {
|
||||||
// Pre-existing behaviour, preserved by the extraction: ImportBase opens the
|
// ImportBase opens read-only and System::Load checks is_open(), so a missing
|
||||||
// stream without checking is_open(), so a missing file yields an empty
|
// file is a clean error — and no empty module is left in the system.
|
||||||
// module rather than an error. Pinned here so any future hardening of this
|
|
||||||
// path is a deliberate, visible change.
|
|
||||||
System sys;
|
System sys;
|
||||||
app::LoadResult r = app::load_module(
|
app::LoadResult r = app::load_module(
|
||||||
&sys, "M", "/nonexistent-dir-xyz/nope.net", ImportType::IMPORT_MENTOR);
|
&sys, "M", "/nonexistent-dir-xyz/nope.net", ImportType::IMPORT_MENTOR);
|
||||||
CHECK(r.ok);
|
CHECK_FALSE(r.ok);
|
||||||
CHECK(r.parts == 0);
|
CHECK(r.error.find("cannot open") != std::string::npos);
|
||||||
CHECK(r.signals == 0);
|
CHECK(sys.modules()->size() == 0);
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user