From 583ab0da291bbac7d39ee7f81585255dad7ba8d4 Mon Sep 17 00:00:00 2001 From: Alan Garny Date: Fri, 9 Oct 2026 17:02:54 +1300 Subject: [PATCH 1/3] New version. --- VERSION.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION.txt b/VERSION.txt index e34ebf6bb..56a2b781e 100644 --- a/VERSION.txt +++ b/VERSION.txt @@ -1 +1 @@ -1.20261009.4 +1.20261009.5 From a5e6788564aa94e4e132aaf46ba9dcdb10dac64d Mon Sep 17 00:00:00 2001 From: Alan Garny Date: Fri, 9 Oct 2026 17:35:08 +1300 Subject: [PATCH 2/3] Fixed remote file identity and temporary copy handling. --- src/api/libopencor/file.h | 3 +- src/file/file.cpp | 24 +++-- src/file/filemanager.cpp | 5 +- src/misc/utils.cpp | 28 ++++- src/support/combine/combinearchive.cpp | 29 +++++- tests/api/file/basictests.cpp | 102 ++++++++++++++++++- tests/api/file/childtests.cpp | 53 ++++++++++ tests/bindings/javascript/file.basic.test.js | 20 +++- tests/bindings/javascript/file.child.test.js | 44 ++++++++ tests/bindings/python/test_file_basic.py | 92 ++++++++++++++++- tests/bindings/python/test_file_child.py | 63 +++++++++++- 11 files changed, 434 insertions(+), 29 deletions(-) diff --git a/src/api/libopencor/file.h b/src/api/libopencor/file.h index 4981e0053..8422ba767 100644 --- a/src/api/libopencor/file.h +++ b/src/api/libopencor/file.h @@ -111,7 +111,8 @@ class LIBOPENCOR_EXPORT File: public Logger /** * @brief Return the file name. * - * Return the file name. If the file is remote then we return the file name of its local copy. + * Return the file name. If the file is remote then we return the file name of its local copy or an empty string if + * it has no local copy (i.e. its contents were not retrieved or could not be downloaded). * * @sa url() * @sa path() diff --git a/src/file/file.cpp b/src/file/file.cpp index b380207ca..c354e74e5 100644 --- a/src/file/file.cpp +++ b/src/file/file.cpp @@ -36,9 +36,11 @@ File::Impl::Impl(const std::string &pFileNameOrUrl, bool pRetrieveContents) } #ifndef __EMSCRIPTEN__ - // Download a local copy of the remote file, if needed and requested. + // Download a local copy of the remote file, if requested. + // Note: a remote file that is not downloaded (or that cannot be downloaded) has no local copy and therefore no file + // path. - if (mFilePath.empty()) { + if (!mUrl.empty()) { if (pRetrieveContents) { auto [res, filePath] {downloadFile(mUrl)}; @@ -50,8 +52,6 @@ File::Impl::Impl(const std::string &pFileNameOrUrl, bool pRetrieveContents) addError("The file could not be downloaded."); } - } else { - mFilePath = stringToPath("/some/path/file"); } } else if (pRetrieveContents) { // Note: we use the std::error_code version of std::filesystem::exists() so that nothing gets thrown (e.g., if @@ -65,10 +65,6 @@ File::Impl::Impl(const std::string &pFileNameOrUrl, bool pRetrieveContents) addError("The file does not exist."); } } -#else - if (mFilePath.empty()) { - mFilePath = stringToPath("/some/path/file"); - } #endif mFileName = pathToString(mFilePath); @@ -77,7 +73,8 @@ File::Impl::Impl(const std::string &pFileNameOrUrl, bool pRetrieveContents) File::Impl::~Impl() { // Delete the local copy of a remote file, if we downloaded it. - // Note #1: a remote file that we didn't download has a dummy file path, which must obviously not be deleted. + // Note #1: we must only ever delete a file that we downloaded ourselves, never a file that just happens to have the + // same file path. // Note #2: we use the std::error_code version of std::filesystem::remove() so that nothing gets thrown since we are // in a destructor. @@ -90,12 +87,19 @@ File::Impl::~Impl() void File::Impl::checkType(const FilePtr &pOwner, bool pResetType) { - // Reset he type of the file, if needed. + // Reset the type of the file, if needed. + // Note: we also release our previous CellML file, SED-ML file, or COMBINE archive, if any, so that we don't keep + // using it (and, for a COMBINE archive, keep its child files managed) if our new contents are of a different + // type or are not recognised. if (pResetType) { removeAllIssues(); mType = Type::UNKNOWN_FILE; + + mCellmlFile = nullptr; + mSedmlFile = nullptr; + mCombineArchive = nullptr; #ifndef __EMSCRIPTEN__ } else if (mType == Type::IRRETRIEVABLE_FILE) { return; diff --git a/src/file/filemanager.cpp b/src/file/filemanager.cpp index ddc2d51ed..75d680b95 100644 --- a/src/file/filemanager.cpp +++ b/src/file/filemanager.cpp @@ -51,7 +51,10 @@ FilePtr FileManager::Impl::managedFile(bool pIsLocalFile, const std::string &pFi std::erase(pLockedFiles, nullptr); for (const auto &lockedFile : pLockedFiles) { - if (pIsLocalFile ? lockedFile->fileName() == pFileNameOrUrl : lockedFile->url() == pFileNameOrUrl) { + // Note: a local file must only ever match a local file, not the local copy of a remote file (or a remote file + // that has no local copy, i.e. an empty file name). + + if (pIsLocalFile ? lockedFile->url().empty() && (lockedFile->fileName() == pFileNameOrUrl) : lockedFile->url() == pFileNameOrUrl) { return lockedFile; } } diff --git a/src/misc/utils.cpp b/src/misc/utils.cpp index d835a392c..4035df5ca 100644 --- a/src/misc/utils.cpp +++ b/src/misc/utils.cpp @@ -35,6 +35,7 @@ limitations under the License. #include #include #include +#include #include #include #include @@ -470,9 +471,12 @@ std::filesystem::path uniqueFilePath() static const std::string LETTERS {"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"}; static const uint64_t LETTERS_SIZE {LETTERS.size()}; - static auto testFile {pathToString(std::filesystem::temp_directory_path() / "libOpenCOR_XXXXXX.tmp")}; + // Note: our caller must ensure that only one thread at a time calls us and creates the file (see downloadFile()), + // so that two threads cannot end up with the same file path. - static const size_t XXXXXX_POS {testFile.size() - 6 - 4}; + static const auto TEST_FILE {pathToString(std::filesystem::temp_directory_path() / "libOpenCOR_XXXXXX.tmp")}; + + static const size_t XXXXXX_POS {TEST_FILE.size() - 6 - 4}; static constexpr uint64_t MICROSECONDS_SHIFT {16U}; static constexpr uint64_t PID_SHIFT {32U}; # ifndef CODE_COVERAGE_ENABLED @@ -480,6 +484,7 @@ std::filesystem::path uniqueFilePath() # endif static constexpr uint64_t XXXXXX_POS_SHIFT {6U}; + auto testFile {TEST_FILE}; TimeVal timeVal; getTimeOfDay(timeVal); @@ -555,8 +560,23 @@ std::tuple downloadFile(const std::string &pUrl) { static const std::tuple NO_TUPLE {false, std::filesystem::path()}; - auto filePath {uniqueFilePath()}; - std::ofstream file(filePath, std::ios_base::binary); + // Create a file with a unique file path. + // Note: we must generate the file path and create the file in one go, so that another thread cannot generate the + // same file path in between (in which case both threads would download to the same file and the local copy + // of one remote file would get deleted with the other remote file). + + static std::mutex uniqueFilePathMutex; + + std::filesystem::path filePath; + std::ofstream file; + + { + const std::scoped_lock lock(uniqueFilePathMutex); + + filePath = uniqueFilePath(); + + file.open(filePath, std::ios_base::binary); + } # ifndef CODE_COVERAGE_ENABLED if (!file.is_open()) { diff --git a/src/support/combine/combinearchive.cpp b/src/support/combine/combinearchive.cpp index a48d15a98..2b92c2905 100644 --- a/src/support/combine/combinearchive.cpp +++ b/src/support/combine/combinearchive.cpp @@ -22,6 +22,8 @@ limitations under the License. #include "combine/combinearchive.h" #include "omex/CaContent.h" +#include +#include #include namespace libOpenCOR { @@ -44,12 +46,37 @@ bool isSedmlFormat(const std::string &pFormat) return pFormat == SEDML_FORMAT; } +std::string archiveLocation(const FilePtr &pFile) +{ + // Determine the location of the COMBINE archive, i.e. the location where its files are "virtually" extracted (e.g., + // "/some/path/archive.omex.contents/"). + // Note #1: the files of a COMBINE archive are managed by the file manager using their file name, so each COMBINE + // archive must have its own location. This means that a remote COMBINE archive that has no local copy + // (i.e. it was not downloaded), and therefore no file name, must use a virtual file name that is unique to + // it. + // Note #2: we canonicalise the location since the files of the COMBINE archive have a canonical file name (see + // File::create()), which we need to be able to compare against (see CombineArchive::Impl::file()). For + // instance, the temporary directory on macOS is a symbolic link (i.e. "/var/..." vs. "/private/var/..."). + + static std::atomic virtualFileNameCounter {0}; + + auto fileName {pFile->fileName()}; + + if (fileName.empty()) { + std::error_code errorCode; + + fileName = pathToString(std::filesystem::temp_directory_path(errorCode) / ("libOpenCOR_" + std::to_string(++virtualFileNameCounter) + ".virtual")); + } + + return pathToString(stringToPath(canonicalFileName(fileName + ".contents") + "/")); +} + } // namespace CombineArchive::Impl::Impl(const FilePtr &pFile, libcombine::CombineArchive *pArchive, UnsignedChars &&pArchiveContents) : mArchiveContents(std::move(pArchiveContents)) , mArchive(pArchive) - , mArchiveLocation(libOpenCOR::pathToString(libOpenCOR::stringToPath(pFile->fileName() + ".contents/"))) + , mArchiveLocation(archiveLocation(pFile)) , mArchiveLocationSize(mArchiveLocation.size()) { // Extract all the files contained in the COMBINE archive. diff --git a/tests/api/file/basictests.cpp b/tests/api/file/basictests.cpp index eff842f44..067340bcd 100644 --- a/tests/api/file/basictests.cpp +++ b/tests/api/file/basictests.cpp @@ -22,6 +22,7 @@ limitations under the License. #include #include #include +#include #include namespace { @@ -170,6 +171,45 @@ TEST(BasicFileTest, remoteFile) EXPECT_FALSE(file->contents().empty()); } +TEST(BasicFileTest, remoteFileLocalCopy) +{ + // The local copy of a remote file must exist for as long as the remote file exists, and be deleted with it. + + std::string fileName; + + { + auto file {libOpenCOR::File::create(libOpenCOR::REMOTE_FILE)}; + + fileName = file->fileName(); + + EXPECT_TRUE(std::filesystem::exists(fileName)); + } + + EXPECT_FALSE(std::filesystem::exists(fileName)); +} + +TEST(BasicFileTest, localFileMatchingLocalCopyOfRemoteFile) +{ + // A local file must never be confused with the local copy of a remote file, and deleting it must not delete that + // local copy (since it is not a file that we downloaded). + + auto remoteFile {libOpenCOR::File::create(libOpenCOR::REMOTE_FILE)}; + const auto &fileName {remoteFile->fileName()}; + + { + auto localFile {libOpenCOR::File::create(fileName)}; + + EXPECT_NE(localFile, remoteFile); + EXPECT_EQ(localFile->fileName(), libOpenCOR::canonicalFileName(fileName)); + EXPECT_EQ(localFile->url(), ""); + EXPECT_EQ(localFile->contents(), remoteFile->contents()); + EXPECT_EQ(libOpenCOR::FileManager::instance().fileCount(), 2U); + } + + EXPECT_TRUE(std::filesystem::exists(fileName)); + EXPECT_EQ(libOpenCOR::FileManager::instance().file(fileName), nullptr); +} + TEST(BasicFileTest, encodedRemoteFile) { auto file {libOpenCOR::File::create("https://models.physiomeproject.org/workspace/aed/@@rawfile/d4accf8429dbf5bdd5dfa1719790f361f5baddbe/FAIRDO%20BG%20example%203.1.cellml")}; @@ -252,6 +292,46 @@ TEST(BasicFileTest, concurrentFileCreations) EXPECT_EQ(std::filesystem::current_path(), origDir); } +TEST(BasicFileTest, concurrentFileDownloads) +{ + // Remote files that are downloaded at the same time must each have their own local copy, so that deleting the local + // copy of one of them doesn't delete the local copy of another one. + + static constexpr size_t THREAD_COUNT {8}; + + std::vector files(THREAD_COUNT); + std::vector threads; + + threads.reserve(THREAD_COUNT); + + for (size_t i {0}; i < THREAD_COUNT; ++i) { + threads.emplace_back([&files, i] { + files[i] = libOpenCOR::File::create(std::string(libOpenCOR::REMOTE_FILE) + "?thread=" + std::to_string(i)); + }); + } + + for (auto &thread : threads) { + thread.join(); + } + + std::set fileNames; + + for (const auto &file : files) { + EXPECT_EQ(file->type(), libOpenCOR::File::Type::CELLML_FILE); + EXPECT_EQ(file->contents(), files[0]->contents()); + + fileNames.insert(file->fileName()); + } + + EXPECT_EQ(fileNames.size(), THREAD_COUNT); + + files.clear(); + + for (const auto &fileName : fileNames) { + EXPECT_FALSE(std::filesystem::exists(fileName)); + } +} + TEST(BasicFileTest, localVirtualFile) { auto filePath {libOpenCOR::resourcePath("unknown_file.txt")}; @@ -278,11 +358,7 @@ TEST(BasicFileTest, remoteVirtualFile) auto file {libOpenCOR::File::create("https://raw.githubusercontent.com/opencor/libopencor/master/tests/res/unknown_file.txt", false)}; EXPECT_EQ(file->type(), libOpenCOR::File::Type::UNKNOWN_FILE); -#ifdef BUILDING_USING_MSVC - EXPECT_EQ(file->fileName(), "\\some\\path\\file"); -#else - EXPECT_EQ(file->fileName(), "/some/path/file"); -#endif + EXPECT_EQ(file->fileName(), ""); EXPECT_EQ(file->url(), "https://raw.githubusercontent.com/opencor/libopencor/master/tests/res/unknown_file.txt"); EXPECT_EQ(file->path(), "https://raw.githubusercontent.com/opencor/libopencor/master/tests/res/unknown_file.txt"); EXPECT_TRUE(file->contents().empty()); @@ -298,6 +374,22 @@ TEST(BasicFileTest, remoteVirtualFile) EXPECT_EQ_ISSUES(file, expectedUnknownFileIssues()); } +TEST(BasicFileTest, remoteVirtualFileMatchingLocalFile) +{ + // A remote file that has no local copy has an empty file name, so it must never be confused with a local file that + // has an empty file name. + + auto remoteFile {libOpenCOR::File::create(libOpenCOR::REMOTE_FILE, false)}; + auto localFile {libOpenCOR::File::create("", false)}; + + EXPECT_NE(localFile, remoteFile); + EXPECT_EQ(remoteFile->fileName(), ""); + EXPECT_EQ(localFile->fileName(), ""); + EXPECT_EQ(localFile->url(), ""); + EXPECT_EQ(libOpenCOR::FileManager::instance().file(""), localFile); + EXPECT_EQ(libOpenCOR::FileManager::instance().file(libOpenCOR::REMOTE_FILE), remoteFile); +} + TEST(BasicFileTest, fileManager) { auto &fileManager {libOpenCOR::FileManager::instance()}; diff --git a/tests/api/file/childtests.cpp b/tests/api/file/childtests.cpp index 9e5e7f602..418566c2f 100644 --- a/tests/api/file/childtests.cpp +++ b/tests/api/file/childtests.cpp @@ -72,3 +72,56 @@ TEST(ChildFileTest, dataset157) { doTestDataset("157", {"fabbri_et_al_based_composite_SAN_model.cellml", "fabbri_et_al_based_composite_SAN_model.sedml"}); } + +TEST(ChildFileTest, remoteVirtualCombineArchives) +{ + // Two remote COMBINE archives that are not downloaded must not share their child files. + + auto file135 {libOpenCOR::File::create(std::string(libOpenCOR::REMOTE_BASE_PATH) + "/api/file/dataset_135.omex", false)}; + auto file157 {libOpenCOR::File::create(std::string(libOpenCOR::REMOTE_BASE_PATH) + "/api/file/dataset_157.omex", false)}; + + file135->setContents(libOpenCOR::fileContents(libOpenCOR::resourcePath("api/file/dataset_135.omex"))); + file157->setContents(libOpenCOR::fileContents(libOpenCOR::resourcePath("api/file/dataset_157.omex"))); + + EXPECT_EQ(file135->type(), libOpenCOR::File::Type::COMBINE_ARCHIVE); + EXPECT_EQ(file157->type(), libOpenCOR::File::Type::COMBINE_ARCHIVE); + + auto simulationFile135 {file135->childFile("simulation.json")}; + auto simulationFile157 {file157->childFile("simulation.json")}; + + ASSERT_NE(simulationFile135, nullptr); + ASSERT_NE(simulationFile157, nullptr); + EXPECT_NE(simulationFile135, simulationFile157); + EXPECT_EQ(libOpenCOR::toString(simulationFile135->contents()), libOpenCOR::textFileContents(libOpenCOR::resourcePath("api/file/dataset_135.json"))); + EXPECT_EQ(libOpenCOR::toString(simulationFile157->contents()), libOpenCOR::textFileContents(libOpenCOR::resourcePath("api/file/dataset_157.json"))); +} + +TEST(ChildFileTest, remoteCombineArchive) +{ + // The child files of a remote COMBINE archive must be retrievable using their file name, even if the local copy of + // the COMBINE archive is in a directory that is accessed through a symbolic link (e.g., on macOS, the temporary + // directory is "/var/...", which is a symbolic link to "/private/var/..."). + + auto file {libOpenCOR::File::create(std::string(libOpenCOR::REMOTE_BASE_PATH) + "/api/file/dataset_135.omex")}; + auto simulationFile {file->childFile("simulation.json")}; + + EXPECT_EQ(file->type(), libOpenCOR::File::Type::COMBINE_ARCHIVE); + ASSERT_NE(simulationFile, nullptr); + EXPECT_EQ(libOpenCOR::toString(simulationFile->contents()), libOpenCOR::textFileContents(libOpenCOR::resourcePath("api/file/dataset_135.json"))); +} + +TEST(ChildFileTest, combineArchiveReplacedWithOtherContents) +{ + // Replacing the contents of a COMBINE archive with some other contents must release its child files. + + auto file {libOpenCOR::File::create(libOpenCOR::resourcePath("cellml_2.omex"))}; + auto &fileManager {libOpenCOR::FileManager::instance()}; + + EXPECT_EQ(fileManager.fileCount(), 3U); + + file->setContents(libOpenCOR::fileContents(libOpenCOR::resourcePath("cellml_2.cellml"))); + + EXPECT_EQ(file->type(), libOpenCOR::File::Type::CELLML_FILE); + EXPECT_FALSE(file->hasChildFiles()); + EXPECT_EQ(fileManager.fileCount(), 1U); +} diff --git a/tests/bindings/javascript/file.basic.test.js b/tests/bindings/javascript/file.basic.test.js index aa2be60e3..df99c4bf2 100644 --- a/tests/bindings/javascript/file.basic.test.js +++ b/tests/bindings/javascript/file.basic.test.js @@ -91,7 +91,7 @@ test.describe('File basic tests', () => { const file = new loc.File(utils.REMOTE_FILE); assert.strictEqual(file.type.value, loc.File.Type.UNKNOWN_FILE.value); - assert.strictEqual(file.fileName, '/some/path/file'); + assert.strictEqual(file.fileName, ''); assert.strictEqual(file.url, utils.REMOTE_FILE); assert.strictEqual(file.path, utils.REMOTE_FILE); assert.deepStrictEqual(file.contents(), Uint8Array.from([])); @@ -112,7 +112,7 @@ test.describe('File basic tests', () => { ); assert.strictEqual(file.type.value, loc.File.Type.UNKNOWN_FILE.value); - assert.strictEqual(file.fileName, '/some/path/file'); + assert.strictEqual(file.fileName, ''); assert.strictEqual( file.url, 'https://models.physiomeproject.org/workspace/aed/@@rawfile/d4accf8429dbf5bdd5dfa1719790f361f5baddbe/FAIRDO BG example 3.1.cellml' @@ -200,6 +200,22 @@ test.describe('File basic tests', () => { assert.strictEqual(process.cwd(), origDir); }); + test('Remote virtual file matching local file', () => { + // A remote file that has no local copy has an empty file name, so it must never be confused with a local file that + // has an empty file name. + + const fileManager = loc.FileManager.instance(); + const remoteFile = new loc.File(utils.REMOTE_FILE); + const localFile = new loc.File(''); + + assert.strictEqual(fileManager.fileCount, 2); + assert.strictEqual(remoteFile.fileName, ''); + assert.strictEqual(localFile.fileName, ''); + assert.strictEqual(localFile.url, ''); + assert.strictEqual(fileManager.fileFromFileNameOrUrl('').url, ''); + assert.strictEqual(fileManager.fileFromFileNameOrUrl(utils.REMOTE_FILE).url, utils.REMOTE_FILE); + }); + test('File manager', () => { const fileManager = loc.FileManager.instance(); const fileName = utils.resourcePath('file.txt'); diff --git a/tests/bindings/javascript/file.child.test.js b/tests/bindings/javascript/file.child.test.js index ff5347b3a..97053dbc8 100644 --- a/tests/bindings/javascript/file.child.test.js +++ b/tests/bindings/javascript/file.child.test.js @@ -79,4 +79,48 @@ test.describe('File type tests', () => { 'fabbri_et_al_based_composite_SAN_model.sedml' ]); }); + + test('Remote virtual COMBINE archives', () => { + // Two remote COMBINE archives that are not downloaded must not share their child files. + + const file135 = new loc.File(`${utils.REMOTE_BASE_PATH}/api/file/dataset_135.omex`); + const file157 = new loc.File(`${utils.REMOTE_BASE_PATH}/api/file/dataset_157.omex`); + + file135.setContents(utils.fileContents(utils.resourcePath('api/file/dataset_135.omex'))); + file157.setContents(utils.fileContents(utils.resourcePath('api/file/dataset_157.omex'))); + + assert.strictEqual(file135.type.value, loc.File.Type.COMBINE_ARCHIVE.value); + assert.strictEqual(file157.type.value, loc.File.Type.COMBINE_ARCHIVE.value); + + const simulationFile135 = file135.childFileFromFileName('simulation.json'); + const simulationFile157 = file157.childFileFromFileName('simulation.json'); + + assert.notStrictEqual(simulationFile135, null); + assert.notStrictEqual(simulationFile157, null); + assert.deepStrictEqual( + simulationFile135.contents(), + utils.fileContents(utils.resourcePath('api/file/dataset_135.json')) + ); + assert.deepStrictEqual( + simulationFile157.contents(), + utils.fileContents(utils.resourcePath('api/file/dataset_157.json')) + ); + }); + + test('COMBINE archive replaced with other contents', () => { + // Replacing the contents of a COMBINE archive with some other contents must release its child files. + + const file = new loc.File(utils.resourcePath('cellml_2.omex')); + const fileManager = loc.FileManager.instance(); + + file.setContents(utils.fileContents(file.path)); + + assert.strictEqual(fileManager.fileCount, 3); + + file.setContents(utils.fileContents(utils.resourcePath('cellml_2.cellml'))); + + assert.strictEqual(file.type.value, loc.File.Type.CELLML_FILE.value); + assert.strictEqual(file.hasChildFiles, false); + assert.strictEqual(fileManager.fileCount, 1); + }); }); diff --git a/tests/bindings/python/test_file_basic.py b/tests/bindings/python/test_file_basic.py index cb5cf899c..149a2f4f2 100644 --- a/tests/bindings/python/test_file_basic.py +++ b/tests/bindings/python/test_file_basic.py @@ -151,6 +151,39 @@ def test_remote_file(): assert file.contents != [] +def test_remote_file_local_copy(): + # The local copy of a remote file must exist for as long as the remote file exists, and be deleted with it. + + file = loc.File(utils.RemoteFile) + file_name = file.file_name + + assert os.path.exists(file_name) + + del file + + assert not os.path.exists(file_name) + + +def test_local_file_matching_local_copy_of_remote_file(): + # A local file must never be confused with the local copy of a remote file, and deleting it must not delete that + # local copy (since it is not a file that we downloaded). + + file_manager = loc.FileManager.instance() + remote_file = loc.File(utils.RemoteFile) + file_name = remote_file.file_name + local_file = loc.File(file_name) + + assert local_file != remote_file + assert local_file.url == "" + assert local_file.contents == remote_file.contents + assert file_manager.file_count == 2 + + del local_file + + assert os.path.exists(file_name) + assert file_manager.file(file_name) is None + + def test_encoded_remote_file(): file = loc.File( "https://models.physiomeproject.org/workspace/aed/@@rawfile/d4accf8429dbf5bdd5dfa1719790f361f5baddbe/FAIRDO%20BG%20example%203.1.cellml" @@ -241,6 +274,44 @@ def create_files(): assert os.getcwd() == orig_dir +def test_concurrent_file_downloads(): + # Remote files that are downloaded at the same time must each have their own local copy, so that deleting the local + # copy of one of them doesn't delete the local copy of another one. + + THREAD_COUNT = 8 + + files = [None] * THREAD_COUNT + + def download_file(index): + files[index] = loc.File(utils.RemoteFile + "?thread=" + str(index)) + + threads = [ + threading.Thread(target=download_file, args=(i,)) for i in range(THREAD_COUNT) + ] + + for thread in threads: + thread.start() + + for thread in threads: + thread.join() + + file_names = set() + + for file in files: + assert file.type == loc.File.Type.CellmlFile + assert file.contents == files[0].contents + + file_names.add(file.file_name) + + assert len(file_names) == THREAD_COUNT + + del file + files.clear() + + for file_name in file_names: + assert not os.path.exists(file_name) + + def test_local_virtual_file(): file_path = utils.resource_path("unknown_file.txt") file = loc.File(file_path, False) @@ -269,10 +340,7 @@ def test_remote_virtual_file(): assert file.type == loc.File.Type.UnknownFile - if platform.system() == "Windows": - assert file.file_name == "\\some\\path\\file" - else: - assert file.file_name == "/some/path/file" + assert file.file_name == "" assert ( file.url @@ -296,6 +364,22 @@ def test_remote_virtual_file(): assert_issues(file, expected_unknown_file_issues) +def test_remote_virtual_file_matching_local_file(): + # A remote file that has no local copy has an empty file name, so it must never be confused with a local file that + # has an empty file name. + + file_manager = loc.FileManager.instance() + remote_file = loc.File(utils.RemoteFile, False) + local_file = loc.File("", False) + + assert local_file != remote_file + assert remote_file.file_name == "" + assert local_file.file_name == "" + assert local_file.url == "" + assert file_manager.file("") == local_file + assert file_manager.file(utils.RemoteFile) == remote_file + + def test_file_manager(): file_manager = loc.FileManager.instance() file_path = utils.resource_path("file.txt") diff --git a/tests/bindings/python/test_file_child.py b/tests/bindings/python/test_file_child.py index c38d53fce..e07f2f57e 100644 --- a/tests/bindings/python/test_file_child.py +++ b/tests/bindings/python/test_file_child.py @@ -13,7 +13,7 @@ # limitations under the License. -from libopencor import File +from libopencor import File, FileManager import os import utils @@ -70,3 +70,64 @@ def test_type_dataset_157(): "fabbri_et_al_based_composite_SAN_model.sedml", ], ) + + +def test_type_remote_virtual_combine_archives(): + # Two remote COMBINE archives that are not downloaded must not share their child files. + + file_135 = File(utils.RemoteBasePath + "/api/file/dataset_135.omex", False) + file_157 = File(utils.RemoteBasePath + "/api/file/dataset_157.omex", False) + + file_135.contents = utils.binary_to_list( + utils.binary_file_contents(utils.resource_path("api/file/dataset_135.omex")) + ) + file_157.contents = utils.binary_to_list( + utils.binary_file_contents(utils.resource_path("api/file/dataset_157.omex")) + ) + + assert file_135.type == File.Type.CombineArchive + assert file_157.type == File.Type.CombineArchive + + simulation_file_135 = file_135.child_file("simulation.json") + simulation_file_157 = file_157.child_file("simulation.json") + + assert simulation_file_135 is not None + assert simulation_file_157 is not None + assert utils.to_string(simulation_file_135.contents) == utils.text_file_contents( + utils.resource_path("api/file/dataset_135.json") + ) + assert utils.to_string(simulation_file_157.contents) == utils.text_file_contents( + utils.resource_path("api/file/dataset_157.json") + ) + + +def test_type_remote_combine_archive(): + # The child files of a remote COMBINE archive must be retrievable using their file name, even if the local copy of + # the COMBINE archive is in a directory that is accessed through a symbolic link (e.g., on macOS, the temporary + # directory is "/var/...", which is a symbolic link to "/private/var/..."). + + file = File(utils.RemoteBasePath + "/api/file/dataset_135.omex") + simulation_file = file.child_file("simulation.json") + + assert file.type == File.Type.CombineArchive + assert simulation_file is not None + assert utils.to_string(simulation_file.contents) == utils.text_file_contents( + utils.resource_path("api/file/dataset_135.json") + ) + + +def test_type_combine_archive_replaced_with_other_contents(): + # Replacing the contents of a COMBINE archive with some other contents must release its child files. + + file = File(utils.resource_path("cellml_2.omex")) + file_manager = FileManager.instance() + + assert file_manager.file_count == 3 + + file.contents = utils.text_to_list( + utils.text_file_contents(utils.resource_path("cellml_2.cellml")) + ) + + assert file.type == File.Type.CellmlFile + assert not file.has_child_files + assert file_manager.file_count == 1 From 5c17734e411ef7087ed08e4342e09d76a3860f71 Mon Sep 17 00:00:00 2001 From: Alan Garny Date: Fri, 9 Oct 2026 17:49:48 +1300 Subject: [PATCH 3/3] Ignore COMBINE entries outside the archive. --- src/support/combine/combinearchive.cpp | 27 ++++++++++++- tests/api/file/childtests.cpp | 27 +++++++++++++ tests/bindings/javascript/file.child.test.js | 36 +++++++++++++++++ tests/bindings/python/test_file_child.py | 37 +++++++++++++++++- .../api/file/entries_outside_of_archive.omex | Bin 0 -> 4731 bytes 5 files changed, 125 insertions(+), 2 deletions(-) create mode 100644 tests/res/api/file/entries_outside_of_archive.omex diff --git a/src/support/combine/combinearchive.cpp b/src/support/combine/combinearchive.cpp index 2b92c2905..8ba589dcf 100644 --- a/src/support/combine/combinearchive.cpp +++ b/src/support/combine/combinearchive.cpp @@ -92,7 +92,32 @@ CombineArchive::Impl::Impl(const FilePtr &pFile, libcombine::CombineArchive *pAr for (int i {0}; i < mArchive->getNumEntries(); ++i) { const auto *entry {mArchive->getEntry(i)}; auto location {entry->getLocation()}; - auto file {File::create(mArchiveLocation + location)}; + + // Make sure that the file is located within the COMBINE archive. + // Note: a location may have some ".." segments (e.g., "../file.cellml"), possibly percent-encoded (e.g., + // "%2E%2E/file.cellml"), in which case the file would be located outside of the COMBINE archive. It + // could then be confused with another managed file (which contents would get replaced) or even with the + // COMBINE archive itself (resulting in an infinite recursion), so we ignore such a file. To determine + // where the file is located, we use its file name as determined by File::create(). + + auto fileNameOrUrl {mArchiveLocation + location}; + auto [isLocalFile, fileName] {retrieveFileInfo(decodeUrl(fileNameOrUrl))}; + + if (!fileName.starts_with(mArchiveLocation)) { + std::string warning; + + warning.reserve(location.size() + 64); // NOLINT + + warning += "The file '"; + warning += location; + warning += "' is not located within the COMBINE archive. It has been ignored."; + + addWarning(warning); + + continue; + } + + auto file {File::create(fileNameOrUrl)}; file->setContents(mArchive->extractEntryToBuffer(location)); diff --git a/tests/api/file/childtests.cpp b/tests/api/file/childtests.cpp index 418566c2f..6d69cc701 100644 --- a/tests/api/file/childtests.cpp +++ b/tests/api/file/childtests.cpp @@ -125,3 +125,30 @@ TEST(ChildFileTest, combineArchiveReplacedWithOtherContents) EXPECT_FALSE(file->hasChildFiles()); EXPECT_EQ(fileManager.fileCount(), 1U); } + +TEST(ChildFileTest, combineArchiveWithFilesOutsideOfIt) +{ + // A COMBINE archive may reference files that are located outside of it (e.g., "../sibling.cellml" or even the + // COMBINE archive itself), in which case they must be ignored rather than replace the contents of another managed + // file (or result in an infinite recursion). + + static const libOpenCOR::ExpectedIssues EXPECTED_ISSUES {{ + {libOpenCOR::Issue::Type::WARNING, "COMBINE archive: the file '../sibling.cellml' is not located within the COMBINE archive. It has been ignored."}, + {libOpenCOR::Issue::Type::WARNING, "COMBINE archive: the file '%2E%2E/encoded_sibling.cellml' is not located within the COMBINE archive. It has been ignored."}, + {libOpenCOR::Issue::Type::WARNING, "COMBINE archive: the file '../entries_outside_of_archive.omex' is not located within the COMBINE archive. It has been ignored."}, + }}; + + auto sibling {libOpenCOR::File::create(libOpenCOR::resourcePath("api/file/sibling.cellml"), false)}; + auto someContents {libOpenCOR::charArrayToUnsignedChars("Some contents.")}; + + sibling->setContents(someContents); + + auto file {libOpenCOR::File::create(libOpenCOR::resourcePath("api/file/entries_outside_of_archive.omex"))}; + + EXPECT_EQ(file->type(), libOpenCOR::File::Type::COMBINE_ARCHIVE); + EXPECT_EQ(file->childFileCount(), 1U); + EXPECT_NE(file->childFile("model.cellml"), nullptr); + EXPECT_EQ_ISSUES(file, EXPECTED_ISSUES); + EXPECT_EQ(sibling->contents(), someContents); + EXPECT_EQ(libOpenCOR::FileManager::instance().fileCount(), 3U); +} diff --git a/tests/bindings/javascript/file.child.test.js b/tests/bindings/javascript/file.child.test.js index 97053dbc8..22634d9f5 100644 --- a/tests/bindings/javascript/file.child.test.js +++ b/tests/bindings/javascript/file.child.test.js @@ -123,4 +123,40 @@ test.describe('File type tests', () => { assert.strictEqual(file.hasChildFiles, false); assert.strictEqual(fileManager.fileCount, 1); }); + + test('COMBINE archive with files outside of it', () => { + // A COMBINE archive may reference files that are located outside of it (e.g., "../sibling.cellml" or even the + // COMBINE archive itself), in which case they must be ignored rather than replace the contents of another managed + // file (or result in an infinite recursion). + + const expectedIssues = [ + [ + loc.Issue.Type.WARNING, + "COMBINE archive: the file '../sibling.cellml' is not located within the COMBINE archive. It has been ignored." + ], + [ + loc.Issue.Type.WARNING, + "COMBINE archive: the file '%2E%2E/encoded_sibling.cellml' is not located within the COMBINE archive. It has been ignored." + ], + [ + loc.Issue.Type.WARNING, + "COMBINE archive: the file '../entries_outside_of_archive.omex' is not located within the COMBINE archive. It has been ignored." + ] + ]; + const sibling = new loc.File(utils.resourcePath('api/file/sibling.cellml')); + const someContents = Uint8Array.from([1, 2, 3]); + + sibling.setContents(someContents); + + const file = new loc.File(utils.resourcePath('api/file/entries_outside_of_archive.omex')); + + file.setContents(utils.fileContents(file.path)); + + assert.strictEqual(file.type.value, loc.File.Type.COMBINE_ARCHIVE.value); + assert.strictEqual(file.childFileCount, 1); + assert.notStrictEqual(file.childFileFromFileName('model.cellml'), null); + utils.assertIssues(loc, file, expectedIssues); + assert.deepStrictEqual(sibling.contents(), someContents); + assert.strictEqual(loc.FileManager.instance().fileCount, 3); + }); }); diff --git a/tests/bindings/python/test_file_child.py b/tests/bindings/python/test_file_child.py index e07f2f57e..98b04dbda 100644 --- a/tests/bindings/python/test_file_child.py +++ b/tests/bindings/python/test_file_child.py @@ -13,7 +13,7 @@ # limitations under the License. -from libopencor import File, FileManager +from libopencor import File, FileManager, Issue import os import utils @@ -131,3 +131,38 @@ def test_type_combine_archive_replaced_with_other_contents(): assert file.type == File.Type.CellmlFile assert not file.has_child_files assert file_manager.file_count == 1 + + +def test_type_combine_archive_with_files_outside_of_it(): + # A COMBINE archive may reference files that are located outside of it (e.g., "../sibling.cellml" or even the + # COMBINE archive itself), in which case they must be ignored rather than replace the contents of another managed + # file (or result in an infinite recursion). + + expected_issues = [ + [ + Issue.Type.Warning, + "COMBINE archive: the file '../sibling.cellml' is not located within the COMBINE archive. It has been ignored.", + ], + [ + Issue.Type.Warning, + "COMBINE archive: the file '%2E%2E/encoded_sibling.cellml' is not located within the COMBINE archive. It has been ignored.", + ], + [ + Issue.Type.Warning, + "COMBINE archive: the file '../entries_outside_of_archive.omex' is not located within the COMBINE archive. It has been ignored.", + ], + ] + + sibling = File(utils.resource_path("api/file/sibling.cellml"), False) + some_contents = utils.text_to_list("Some contents.") + + sibling.contents = some_contents + + file = File(utils.resource_path("api/file/entries_outside_of_archive.omex")) + + assert file.type == File.Type.CombineArchive + assert file.child_file_count == 1 + assert file.child_file("model.cellml") is not None + utils.assert_issues(file, expected_issues) + assert sibling.contents == some_contents + assert FileManager.instance().file_count == 3 diff --git a/tests/res/api/file/entries_outside_of_archive.omex b/tests/res/api/file/entries_outside_of_archive.omex new file mode 100644 index 0000000000000000000000000000000000000000..c30436a2d9afd2d69ca995958f045ef2de3f554d GIT binary patch literal 4731 zcmeHLcTkgE_6{8(2qIEc-~*-C1VK(q!r^goFA}8|9>KQ7k(ezb0!Vz$q|Mw|3sEs)HVUnnEPao0UVY*4 zB+Z-8xivWja-(=AaR-e6e&+{sTm<%@J!NTvOhYR;3vx9n%xF=UpC&=SdZTE->%)t? z3BuZEoJ|wlIwCA@mkhGWO*_J;m$PvXxp{G|7Ur9ExpMijO^{pbz@!mfH%&3*a>@Xe!mix= zF_nN2v6fcUDJ9^Z*YAbKgdxSF7VJ5@jl3j+B)n=PKWy=ez7!F)e`Pw)5y_BT_|&4f zIuK{{LpFy`E>#3=2<3@=)#lWLfAh&fC9YM!kT~M%Kg-b>g}jxZX)m3i5h~)^ZE-Jj z#En4@k$!#*D5cs}gbNMK9BVc5EMieoX09HyF=MUkt?~d`>iT!~muOqh@>CKG;UlW# z@7}RUM<(kf6}rV4!P@pWxL{1A+X%L10x325%3ia)sDFF`Dd!KVD_MDGW^7hnu+9=( zr|IivN@6=Q!`DBNuepHghtoRNJ^N=@L-m}1s2r!Jvo+mkzcdlxeQXR~?1>hc-tf%t;eW zc@W~>Ea9`^&qkT$r$xl%libKE^j5o_n8lIzxfl}7N~9$itlFlhn0VXGdG9tr0XyB` zXjNQt!PuT>GB}1dW6^jIA4S_F=3FSjZaZ3MZz2`rTSB+^6BN2eWIw@ zm$*TGh=*|j-zVM7(P3!53pJJ&R4C#DK{|;2#vB~gkFtnCIGPd{5o&5fngjNdL@bYj zvPGC#i{3_nT6~(=>M(DQdON>;@%9urdbl`;s<;+`vgckc>%JRD*yupLZT|i>K-qWz z9T^{Ske$Y2C@Cx^9k$e0_+FXr_UBJr@REaJV3g*v^1jGc!Nd8Nr=N75eQ_xsG#pK- zI4WV`q8$w|sOkx2mE={`5W)ia*%e6keTJ7h{;edDBFEuHHC= zbqfD1+QBb2sBdt|S)kjeX^DUZdzrq`jlyEVSl)Fm@gnA!-E+5VKqMJ9$;du62pto~ zC^%-f@4QV%eQC?$OgRuxv5A5OHTRW87m`w~Hm$0#=dD|$zk&C{oL0z&77Z5Y_O34T%6Au&h{MqZ;O=Sk;89}+yK3T85 z)9wn4ZP91DtAszM%IXIq1QdA07nA!`pR%3BT&=BD^sl$Kr{on1<|<45HgE1yuxV!( zf8d~6IPJ52(Lj5M&bYiAwnHf^-nL^$ze+03=X82xekI*X&W_HE7~n4fXJh-4=v^O> zpq_9ppb{#)^j5muJcTkA^YF;TrqM|FJRD9p?3YRoIeCIMT%p`s@Bw55(r_}YE-;fY zHx|@Al){o7X-8&6Z@7q57oHNqcqaAxwJfL66pGLm7jy_8bP|mX)2i)P= z<;d)idU5Nk+g8D1c7)LDoV!~=DqpwG3Q@rP6peJ~f$*#H%h8vA`~)kEgG9bS+j_j% zvY#w=ZTeaU{mSpTXyQmpb#$!OFiM{sFC<;np3rm(GB}*%wdb_!Q-67NCqZT|I@Z5p z%Kzm)vyHK8$fyF0`NApA9+~=+-79uU>e9@8e1auFB+hlQ+Ust$_Mmf2e^S<}QicTc zruC!Pz~Gy;x*}&79L8Xj(-f)zk_o4REvI!$Azb387&(YVJ-K`?GfPD+VI4!K0}_0O z>URrjz5FG=?Led5Jie%yhR3c>qITT3AvNi~iB&*SGII~sY;ZWEsDC7B(3#2s3_r&l z+t(E}G$GgF+R){`QNGJB#ifZ@)1Y84c*rHXRz9o#UMwF>L)Zo_@7mt5AZMIQ;ZMFD zzxX^qeveP1a(+_7=o;$43|@0>Lz{AU&a20pg6AS{6RQTXx;e#h_v4JUSmY^+pec^9 zEbvt*Q_QQK>6x-SoWs1CZ!V{(2`(V<J_cFNga@$()uEw<5E4k@#a)mphd=QVYymen>AW9cz5E`i@mmf_$ z$tS4s(WxN`mTE#OyD(}+1;^>mo8ETyWUJMuJQK2~TEEqUG$`#u^jT&z&cF3KTcuFN z&(uJbTz^EvO9d;gh;AgRIAu45jD2CY=}d~z#vdSId1}2~O$1@GdH@HQst%pPcOc!< zpG)e540Emlc_JHC8Z`5CP9?R-9JFPOF66XA)(iL*FuBZ8;t%#3SHg0g?GRft*zh zt-B4+xi(8%`ZYA{xCiYtL1wm37ewLSf#7y31onJwcG>a$!94BV7xTL5E)~;pXkTWk z!ZqP7RYP_uqQ|V1G~U~K1y#Sn+r~xNfB$GSFiV|Y;1yWm#?&V*t*IsJC<7bDu&c84 zGj&!?lCc}w8Wuy*`@LllQqOgsg6VVtZ=flPXO1QKA#Vh>0pRPOR8rS1QKj4`)Y@&y&`$Q`O@2_X7-$k;9 z3LJ4I2pfD%7+t5Gvpg)RKcp7(%hu(^K7DVW1$>lq7bFFIxL026+$a+X!%8Y**Z^}Y$Mc9c6*K5oxDA|2CPu|FVf z?WZCGC0qx4cxTNmD7aM?T_3~_+E(~8V)Hf8D~8n0)kqd7!O~omT5@A4$PtpEi^#UtepD)l5eMyuyD?kbO2W zSJw_kWYXSZj-p94>P!Ql+%M18k(HK18WXxEhafPn{%C#rhYKV9uUT%Y`rg;qqf`!+ zEJ~N!A$+iLyvImp0|g$vjLPnxJjBP)g#0hl0;hj#iKPGKLCSb8knW@PY*{ZFHqSjw zsF|iSF`%R8Vffd=$Z_%dhlL!g2LCLR{2BRYZQ-vt03d{p`aAOfC=LG(`FlV6cMG9w zI+kbtUw8ZO;J^1!f42}i9OHlJsQ&Eg&vpAhT)~hgwbqv5 literal 0 HcmV?d00001