Non URI file scheme equal type name support #688
Conversation
ljamt
requested review from
markaren,
kyllingstad,
davidhjp01,
eidekrist and
restenb
src/cosim/uri.cpp
Outdated
| { | ||
| const auto query = queryUri.query(); | ||
| if (query && query->find("file=") < query->size()) { | ||
| if (query->find("file=file:///") < query->size()) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
src/cosim/uri.cpp
Outdated
| if (query->find("file=file:///") < query->size()) { | ||
| 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()); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
|
Looks good to me too. The only thing I'm wondering is whether |
Depends if the function is expected to be reused later outside |
|
I'd look at it the other way around. If we later find that the function is useful elsewhere, it can easily be moved to the public API then. But once a function is in the public API, it's difficult to change or get rid of, so we shouldn't add it prematurely. |
tests/CMakeLists.txt
Outdated
| endif() | ||
|
|
||
| include("AddTestExecutable") | ||
| foreach(test IN LISTS tests) | ||
| if (LIBCOSIM_WITH_PROXYFMU) | ||
| list(APPEND dependencies "proxyfmu::proxyfmu-client") |
There was a problem hiding this comment.
This line does nothing as dependencies is not used.
There was a problem hiding this comment.
Removed in latest commit
Closes #685 by allowing the OspModelDescription file to be located in a different directory than the OspSystemStructure file togheter with the model fmu. A new function for parsing a query file parameter is added for non
fileURI schemes with a fallback of the existing function. Both relative and absolute path is supported. This has previously only been possible for local simulations.