Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Non URI file scheme equal type name support #688

Merged
merged 22 commits into from
Mar 29, 2022
Merged

Non URI file scheme equal type name support #688

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
8e72c9b
Removed file scheme check
Feb 28, 2022
bd9e0c3
Merge branch 'master' into bug/proxy-fmu-equal-type-name
Feb 28, 2022
8fced5e
Allow proxyfmu URI scheme
Feb 28, 2022
fab82a6
Removed URI authority check
Feb 28, 2022
ae14c69
Check model relative path for OspModelDescription with fmuproxy
Mar 2, 2022
f3b2330
Fixed fmuproxy model path
Mar 2, 2022
4485b9f
File path fix
Mar 2, 2022
7433063
Added logging for debugging
Mar 3, 2022
555ca0b
Fix prototype
Mar 3, 2022
7173d9a
Absolute path query file support
Mar 4, 2022
b89fbdb
Merge branch 'master' into bug/proxy-fmu-equal-type-name
Mar 4, 2022
f3c80d6
Added some tests
Mar 4, 2022
3b3472a
Final testing implementation with logging
Mar 4, 2022
ebd8ace
Temporary removal of Linux file query uri tests
Mar 4, 2022
67312cc
Edited invalid tests
Mar 4, 2022
3fb00eb
Reintroduced non Windows tests
Mar 7, 2022
53f4f52
Non Windows tests corrections
Mar 7, 2022
f1937d8
UNIX filesystem compatible function rewrite
Mar 7, 2022
b9aba4a
Removed scheme assertion
Mar 8, 2022
42aecf7
Changed input parameter from baseUri to baseParentUri
Mar 21, 2022
6213f43
Moved file_query_uri_to_path function from uri to osp_config_parser
Mar 23, 2022
a5ae80c
Removed unused proxyfmu client dependency for tests
Mar 23, 2022
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
18 changes: 17 additions & 1 deletion include/cosim/uri.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,13 +266,29 @@ uri path_to_file_uri(const cosim::filesystem::path& path);
*
* \param [in] fileUri
* An URI where the scheme component is equal to `file` and the `authority`
* component is either empty or equel to `localhost` (but not undefined).
* component is either empty or equal to `localhost` (but not undefined).
*
* \returns
* The path that corresponds to `fileUri`.
*/
cosim::filesystem::path file_uri_to_path(const uri& fileUri);


/**
* Converts a URI with query parameter to a local filesystem path.
*
* \param [in] baseUri
* An (absolute) base URI.
* \param [in] queryUri
* An URI with arbitrary scheme component. Query file parameter path (if any)
* is specified after `file=`. Prefix `file:///` can be used for absolute path.
*
* \returns
* The local absolute path that corresponds to the `queryUri`, or the `baseUri`
* parent path if no file query is available.
*/
cosim::filesystem::path file_query_uri_to_path(const uri& baseUri, const uri& queryUri);


} // namespace cosim
#endif // header guard
17 changes: 5 additions & 12 deletions src/cosim/osp_config_parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -938,19 +938,12 @@ osp_config load_osp_config(
const auto modelUri = resolve_reference(baseURI, simulator.source);
cosim::filesystem::path msmiFilePath;

if (modelUri.scheme() == "file") {
msmiFilePath = file_uri_to_path(modelUri).remove_filename() / msmiFileName;
if (cosim::filesystem::exists(msmiFilePath)) {
emds.emplace(simulator.name, msmiFilePath);
} else {
msmiFilePath = configFile.parent_path() / msmiFileName;
if (cosim::filesystem::exists(msmiFilePath)) {
emds.emplace(simulator.name, msmiFilePath);
}
}
msmiFilePath = (modelUri.scheme() == "file")
? file_uri_to_path(modelUri).remove_filename() / msmiFileName
: file_query_uri_to_path(baseURI, modelUri).remove_filename() / msmiFileName;
if (cosim::filesystem::exists(msmiFilePath)) {
emds.emplace(simulator.name, msmiFilePath);
} else {
// Makes it possible to keep OspModelDescription at configuration path
// even when there are FMUs with other URI than file (fmu-proxy).
msmiFilePath = configFile.parent_path() / msmiFileName;
if (cosim::filesystem::exists(msmiFilePath)) {
emds.emplace(simulator.name, msmiFilePath);
Expand Down
16 changes: 16 additions & 0 deletions src/cosim/uri.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,4 +559,20 @@ cosim::filesystem::path file_uri_to_path(const uri& fileUri)
}


cosim::filesystem::path file_query_uri_to_path(
const cosim::uri& baseUri,
const cosim::uri& queryUri)
{
const auto query = queryUri.query();
if (query && query->find("file=") < query->size()) {
if (query->find("file=file:///") < query->size()) {
Copy link
Member

Choose a reason for hiding this comment

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

Would this effectively tie us down to the "classical" three slashes file URI scheme only? Sounds to me like it is now considered optional, quote from RFC 8089:

"According to the definition in [[RFC1738](https://www.rfc-editor.org/rfc/rfc1738)], a file URL always started
   with the token "file://", followed by an (optionally blank) host name
   and a "/".  The syntax given in [Section 2](https://www.rfc-editor.org/rfc/rfc8089#section-2) makes the entire authority
   component, including the double slashes "//", optional."

https://www.rfc-editor.org/rfc/rfc8089#appendix-A

Should we take into account that the file URI may be of the form file:/ also?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case I think there also have to be made changes to proxy_uri_sub_resolver::lookup_model for consistency. file:/// in the query is used there to distinguish between relative and absolute path. Report as separate issue?

Copy link
Member

Choose a reason for hiding this comment

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

Let's do that then. There are a couple other things I'm skeptical about in the resolvers, like the reliance on substr(specific_number). Should be worth a closer look.

return file_uri_to_path(std::string(query->substr(5)));
}
const auto uriToAppend = cosim::path_to_file_uri(cosim::file_uri_to_path(baseUri).parent_path());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better for readability if this function just gets the parent of baseUri as the first argument, rather than computing it here. I think the function signature might mislead users that it will use the full baseUri for computing the final path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with the reasoning and if this was an isolated case it would make sense to rename it to baseUriParentPath or similar. The reason I am still leaning towards keeping the current code is that baseUri is consistently used in multiple similar functions and I find it strange to have a one-off parameter that is very similar but not quite equal. Further inputs are much appreciated

Copy link
Member

Choose a reason for hiding this comment

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

If this would be an improvement to readability and/or usability of the code, then I'd say just do it. No need to rely on adherence to a similar implementation elsewhere (in fact, perhaps that too should be updated).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New commit with baseParentUri instead of baseUri with required updates is added. Parent path uri is found before calling the file_query_uri_to_path function

return file_uri_to_path(std::string(uriToAppend.view()) + "/" + std::string(query->substr(5)));
}
return file_uri_to_path(baseUri).parent_path();
}


} // namespace cosim
24 changes: 24 additions & 0 deletions tests/uri_unittest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,3 +222,27 @@ BOOST_AUTO_TEST_CASE(file_uri_conversions)
BOOST_CHECK_THROW(file_uri_to_path("http://foo/bar"), std::invalid_argument);
BOOST_CHECK_THROW(file_uri_to_path("file://foo/bar"), std::invalid_argument);
}


BOOST_AUTO_TEST_CASE(file_query_uri_conversions)
{
// From URI file query to path
const auto baseURI = uri("file:///c:/foo/bar");
#ifdef _WIN32
BOOST_TEST(file_query_uri_to_path(baseURI, "proxyfmu://foo%20bar/bar?file=baz.txt") == "c:\\foo\\baz.txt");
BOOST_TEST(file_query_uri_to_path(baseURI, "proxyfmu://foo%20bar/bar/foo?file=bar/baz.txt") == "c:\\foo\\bar\\baz.txt");
BOOST_TEST(file_query_uri_to_path(baseURI, "http://foo%20bar/bar?file=baz.txt") == "c:\\foo\\baz.txt");
BOOST_TEST(file_query_uri_to_path(baseURI, "http://foo%20bar/foo/bar?file=bar%20foo/baz.txt") == "c:\\foo\\bar foo\\baz.txt");
BOOST_TEST(file_query_uri_to_path(baseURI, "proxyfmu://foo/bar?file=file:///c:/baz.txt") == "c:\\baz.txt");
BOOST_TEST(file_query_uri_to_path(baseURI, "http://foo%20baz/bar?file=file:///c:/foo/bar/baz.txt") == "c:\\foo\\bar\\baz.txt");
BOOST_TEST(file_query_uri_to_path(baseURI, "http://foo%20baz/bar?foo=baz.txt") == "c:\\foo");
#else
BOOST_TEST(file_query_uri_to_path(baseURI, "proxyfmu://foo%20bar/bar?file=baz.txt") == "/c:/foo/baz.txt");
BOOST_TEST(file_query_uri_to_path(baseURI, "proxyfmu://foo%20bar/bar/foo?file=bar/baz.txt") == "/c:/foo/bar/baz.txt");
BOOST_TEST(file_query_uri_to_path(baseURI, "http://foo%20bar/bar?file=baz.txt") == "/c:/foo/baz.txt");
BOOST_TEST(file_query_uri_to_path(baseURI, "http://foo%20bar/foo/bar?file=bar%20foo/baz.txt") == "/c:/foo/bar foo/baz.txt");
BOOST_TEST(file_query_uri_to_path(baseURI, "proxyfmu://foo/bar?file=file:///c:/baz.txt") == "/c:/baz.txt");
BOOST_TEST(file_query_uri_to_path(baseURI, "http://foo%20baz/bar?file=file:///c:/foo/bar/baz.txt") == "/c:/foo/bar/baz.txt");
BOOST_TEST(file_query_uri_to_path(baseURI, "http://foo%20baz/bar?foo=baz.txt") == "/c:/foo");
#endif
}