wxMemoryFS bug with Windows filenames

[helser]

I added a file to a MemoryFS

wxString name = "C:\data\my_file.txt";
wxMemoryFSHandler::AddFile(name, "some content");

but then I couldn't retrieve it:

wxFileSystem fs;
wxFSFile *fs_file = fs.OpenFile("memory:"+name);
if (!fs_file) return; // fs_file always NULL.....

Turns out, when trying to look up the file, filesys.cpp:: MakeCorrectPath is called on the string that's passed in, so it converted back-slashes to forward-slashes, and the names weren't equivalent. I did this:

wxString name = "C:\data\my_file.txt";
name.Replace("\\", "/");
wxMemoryFSHandler::AddFile(name, "some content");

and now I'm able to retrieve it, using either / or \ in the path.

I believe this should be fixed internal to wxWidgets. What I suggest is making MakeCorrectPath() a static member of wxFileSystem, and then having wxMemoryFSHandler::AddFile() call it on the path that is passed in for the file. Reasonable?

[vadz]

Replying to helser:

I added a file to a MemoryFS

wxString name = "C:\data\my_file.txt";
wxMemoryFSHandler::AddFile(name, "some content");

Is the above really accurate? I.e. did you really use non-escaped backslashes in a C string like this?

[helser]

Sorry, I was simplifying. I really do this:

bool MyClass::convertToFoo(const wxString& file_name)
{
  wxFileName fn(file_name);
  wxStringOutputStream out_s;
  //.... fill in out_s
  wxString name = fn.GetFullPath();
  name.Replace("\\", "/");
  wxMemoryFSHandler::AddFile(name, out_s.GetString());
}

so I'm sure this is a well-formed Windows filename with sensible contents.

[vaclavslavik]

wxFileSystem works with URLs, where '/' is used as path deliminer, not '\', so this isn't entirely unreasonable behavior. MakeCurrectPath is just a compatibility hack to allow using filenames with wxFileSystem (this was a mistake, but it's too late to fix it). So I'd very much prefer to clarify wxMemoryFSHandler documentation instead.

[vadz]

I agree, wxMemoryFSHandler::AddFile() clearly should take a file name but the rest of the wxFS functions work with URIs. OTOH I don't really know what should be added to wxMemoryFSHandler documentation -- it already works just fine unless I'm missing something. Or do you mean to write something like

  Notice that while a file name is used here, meaning that either slash or
backslash can be used as the path separator, you need to use the slashes
which are the only valid separators in an URI when accessing this file later
using "memory:" URI schema.

?

[vaclavslavik]

Actually, no. The filename argument to AddFile* functions is badly named, it dates back to when wxFileSystem tried to handle filenames too. It's value is actually the part of URI after "memory:" (as it says) that the data will be available under, i.e. it should follow URI conventions. In particular, it is not a file name.

[helser]

I documented the result of my problem, but not one of the underlying causes. In my original example,
I added a file to a MemoryFS

wxString filename = "C:\\data\\my_file.txt";
wxMemoryFSHandler::AddFile(filename, "some content");

At this point, the wxMemoryFSHandler stores the file using 'filename' as the key,
calling

m_Hash->Put(filename, ....)

It is not transformed in any way, meaning it still contains back-slash as the path delimiter.

Then I when I try to retrieve it:

wxFileSystem fs;
filename.Replace("\\", "/"); // Doesn't matter!
wxFSFile *fs_file = fs.OpenFile("memory:"+filename);
if (!fs_file) return; // fs_file always NULL.....

it is inaccessible - 'MakeCorrectPath' is called on 'filename', and so the back-slashes are converted to forward-slashes before using it as a key for the hash-table.

That means there is no way to retrieve my file from the memoryFS.

I would advocate one of two possible fixes:
1:
Call MakeCorrectPath on the filename argument of every 'AddFile' method of wxMemoryFSHandler

2: Change the 'filename' argument to a 'location' argument, and make some forms illegal

• an assert in the 'AddFile' to check for illegal characters, like '\'
• documentation changes
• MakeCorrectPath or something similar is a documented, public, static method that takes a filename as an argument and returns a legal 'location' that can be passed to wxMemoryFSHandler::AddFile

I'm not saying 'uri' or 'url' because I'm not sure how to use the terms correctly in this context.

Which is preferred?

Thanks!

[vaclavslavik]

Replying to helser:

wxString filename = "C:\\data\\my_file.txt";
wxMemoryFSHandler::AddFile(filename, "some content");

As I said, this is wrong, the filename argument should be URI part, e.g. "/data/my_file.txt" here.

2: Change the 'filename' argument to a 'location' argument, and make some forms illegal

• an assert in the 'AddFile' to check for illegal characters, like '\'
• documentation changes

Yes, that's what I wrote above. The asserts may be useful, but also relatively expensive, and if they're added there, they should be in other places where wxFileSystem URIs are used.

• MakeCorrectPath or something similar is a documented, public, static method

No, it's an internal helper. Its slashes translation is a side effect that you shouldn't expect or rely on.

Which is preferred?

I'm not sure, see the rest of wxFileSystem (and related classes) docs.

[helser]

Replying to vaclavslavik:

Replying to helser:

wxString filename = "C:\\data\\my_file.txt";
wxMemoryFSHandler::AddFile(filename, "some content");

As I said, this is wrong, the filename argument should be URI part, e.g. "/data/my_file.txt" here.

I agree, currently this doesn't work. I expected it to work based on the documentation, and I would like it to work for my current application. I'm caching a set of files in memory, that are quick enough to generate for some sources that they don't need to be stored on disk, but for larger sources they might need to be saved on disk.

I would guess this is a typical use-case for wxMemoryFS - sometimes the files are on disk, sometimes they are in memory. I would like to my app to use it by adding 'memory:' to the filename I retrieve, but I am willing to add a few more steps if needed.

2: Change the 'filename' argument to a 'location' argument, and make some forms illegal

• an assert in the 'AddFile' to check for illegal characters, like '\'
• documentation changes

Yes, that's what I wrote above. The asserts may be useful, but also relatively expensive, and if they're added there, they should be in other places where wxFileSystem URIs are used.

I was proposing asserts only in the debug build, which would have caught my problem, and wouldn't affect performance in release.

• MakeCorrectPath or something similar is a documented, public, static method

No, it's an internal helper. Its slashes translation is a side effect that you shouldn't expect or rely on.

Can we make available a method that I can expect and rely on? The alternative that I see right now is calling wxFileSystem::FileNameToURL(), and then replacing the leading 'file:' with 'memory:'. (Does anyone do it that way?)

Which is preferred?

I'm not sure, see the rest of wxFileSystem (and related classes) docs.

What are the other common uses for wxMemoryFS? I was kind of expecting it to be similar to wxArchiveFS, which I have not used, but is documented as dealing with filenames that are stored in a Zip file, for example. It seems that platform-legal filenames would be allowed as input. Is this not the case?

I'm pushing towards allowing filenames as input to wxMemoryFS because that's the way I want to use it and expected it to function. I see Vaclav's point coming from http: and wxInternetFSHandler too.

[vadz]

I see Vaclav's point too but it's tempting to just add a MakeCorrectPath() call here too because we already do it in other places and it's not clear why shouldn't we do it here as well if we do it inside OpenFile(). I.e. IMO we should either never call MakeCorrectPath() or call it everywhere. And the latter is more backwards compatible, of course.

I don't think MakeCorrectPath() should be made public however. Whatever the wxFS methods should just be documented.