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.

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

Facing Problem when embedding file #2595

Closed
chanjunweimy opened this issue Oct 12, 2017 · 29 comments
Closed

Facing Problem when embedding file #2595

chanjunweimy opened this issue Oct 12, 2017 · 29 comments
Assignees
Milestone

Comments

@chanjunweimy
Copy link

chanjunweimy commented Oct 12, 2017

  • Your Abp package version: 3.0.0.0
  • Your base framework: .Net Core.

EmbeddedResourceSet has bug if the embedded file has "-"

This is because of this segment of code:

private string ConvertToRelativePath(string resourceName) {
    resourceName = resourceName.Substring(ResourceNamespace.Length + 1);

    var pathParts = resourceName.Split('.');
    if (pathParts.Length <= 2)
    {
        return resourceName;
    }            
    var folder = pathParts.Take(pathParts.Length - 2).Select(NormalizeFolderName).JoinAsString("/");
    var fileName = pathParts[pathParts.Length - 2] + "." + pathParts[pathParts.Length - 1];

    return folder + "/" + fileName;
}

When pathParts.Length <= 2, the code did not normalize the string,

However, when retrieving fileinfo. EmbeddedResourceManager normalizes the string no matter when:

public EmbeddedResourceItem GetResource(string fullPath)
{
    return _resources.Value.GetOrDefault(EmbeddedResourcePathHelper.NormalizePath(fullPath));
}

To reproduce it:
Embed a file with "-" located in the root level,
then try to get file info of it using EmbeddedResourceFileProvider

Expected: Get file info successfully
Actual: File Info not found

Furthermore,
since:

public IDirectoryContents GetDirectoryContents(string subpath)
        {
            if (!IsInitialized())
            {
                return new NotFoundDirectoryContents();
            }

            //TODO: Implement...?

            return new NotFoundDirectoryContents();
        }

May I ask if there is a plan to implement this method?

Thank you.

@chanjunweimy
Copy link
Author

chanjunweimy commented Oct 12, 2017

I did a workaround at here for the GetDirectoryContents (however, not the dash issue) if anyone who needs help, can take a look at there I think. Hopefully, it helps ^^

@hikalkan hikalkan added this to the v3.2 milestone Oct 12, 2017
@chanjunweimy
Copy link
Author

chanjunweimy commented Oct 20, 2017

Other than that, I also realized that since

private string ConvertToRelativePath(string resourceName) {
    resourceName = resourceName.Substring(ResourceNamespace.Length + 1);

    var pathParts = resourceName.Split('.');
    if (pathParts.Length <= 2)
    {
        return resourceName;
    }            
    var folder = pathParts.Take(pathParts.Length - 2).Select(NormalizeFolderName).JoinAsString("/");
    var fileName = pathParts[pathParts.Length - 2] + "." + pathParts[pathParts.Length - 1];

    return folder + "/" + fileName;
}

Embedded file would have problem with getting the correct name if the embedded file has more than one "." in the name, for example: app.component.ts.

Currently, my workaround is to create another file info and a EmbeddedResourceItem "Wrapper" to allow myself to define correct file name in the file info, see: chanjunweimy/abp_plugin_with_ui@9f7db13

@hikalkan
Copy link
Member

I created an issue for implementing EmbeddedResourceFileProvider.GetDirectoryContents #2687

How did you solved "." issue. .net converts folder splitter "/" to "." so we can not understand if the "." is a folder or a char in the file name.

@hikalkan hikalkan self-assigned this Nov 14, 2017
@chanjunweimy
Copy link
Author

Hello @hikalkan, thanks for your reply! The workaround I did at the end was to add an "if-else" statement in the ConvertToRelativePath() function. Alternatively, I was hoping for a solution like:

  1. Add a folder as embedded resources, at the same time preserve the file structure in an embedded xml/json file.
  2. Get the original file name and path from the embedded xml/json file.

Thank you.

Jun Wei

@hikalkan
Copy link
Member

The solution you provided may work but it's not so pratical. Alternative and easier solution:

  • Add a __ char (or another special char) at the beginning of file name so we can understand the rest of the embedded resource name is file name.

But I don't like both solutions, because it's not so natural. Think that we wanted to add a folder with many files and subfolders into our solution as embedded resource, I don't want to make additional work other than just adding files. Especially if I'm updating these files from some other source (think that I'm embedding a NPM js package files).

@robsiera
Copy link

You might want to look at aspnet/FileSystem#184

@sachatrauwaen
Copy link
Contributor

I am ready to propose a pull request to adresse the issue of double dots in embeded resource file names.

The idea is to store the items in the EmbeddedResourceSet with filepath containing dots for all folders separators. (+/- like the way they are stored in the asembly) included for the basepath.

By doing this retrieving the resource with the same Normalize function will retrieve always the right resource.

I make a clone of the item stored in the set to set the right filename.

The side effect is that no difference is done betweed a '.' and a '/' but this is already the case in the assembly except for the base path.

here the code sachatrauwaen@d777f04

A added also 2 unit tests for resources with double dots, fix unit test (MyScriptFile1.js was used in place of MyScriptFile-2.js for the dash test).

Let me know if you think it's good for pull request or want some adjustments...

@hikalkan
Copy link
Member

@sachatrauwaen as I understand, this way breaks foldering (we will lose the folder structure because we can not distinguish / and .). However, embedding a file into an assembly just works like that.
But I didn't investigate your code fully. If you want, send a PR and explain what it does and what problems it solve, then we review your PR.
Thanks.

@sachatrauwaen
Copy link
Contributor

It not breaks the foldering. I store the foldering with a "." in place of a "/" as folder separator. Like in the assemblies. And make the retrieve of a file in the resourceset folowing this same rule.

So the ResourceSet make no difference between a "/" and a "." in the filepath.

I think it is a solution that sound good, because it use the same convension used internally in the asembly.

Like in some references in the comments, to do in better, the only way is to store a manifest with the mapping between the real filename and name in the assembly. And that's for the dotnet team...

I will do some additional tests and make a pullrequest.

@hikalkan
Copy link
Member

hikalkan commented Dec 1, 2017

I would not like to make files flat in same directory with "." for dir seperator. I also don't want to add a manifest/metadata file (as you said, MS should do it). But waiting for your PR to understand your implementation better.

@chanjunweimy
Copy link
Author

@sachatrauwaen but this method doesn't work yet if we want to download the embedded file?
@hikalkan hmm, actually my previous workaround for the embedded files include embed a zip file instead of the original folder: this approach preserve the original file structure and we can unzip the embedded files if we need to use them directly. It still needs an extra step which is to zip the folder though. What do you think?

@sachatrauwaen
Copy link
Contributor

@chanjunweimy actually there are problems with double dots in filenames and dashes in foldernames. Can you clarify your question.

@chanjunweimy
Copy link
Author

@sachatrauwaen yup~ what I mean was, after taking a look of your pull request, I think your pull request couldn't handle the case where we want to download the embedded files and the embedded files have double dots in filenames. This is common because many angular files have double dot, ie x.routing.ts, x.pipe.ts etc. Just want to clarify if my understanding is correct.

@sachatrauwaen
Copy link
Contributor

sachatrauwaen commented Dec 7, 2017

Yes my pull request handle double dots in file names.

Look at the tests. I add some test for double dots in file names.

https://github.com/sachatrauwaen/aspnetboilerplate/blob/embeded_resources/test/Abp.Tests/Resources/Embedded/EmbeddedResourceTests.cs

@chanjunweimy
Copy link
Author

@sachatrauwaen yes, you handled double dots in file names, what I mean is if we want to preserve the folder structure, i.e. if I embed the whole angular ui src file, we should be able to download all files in the same structure, otherwise it is not that meaningful.

@sachatrauwaen
Copy link
Contributor

@chanjunweimy Yes thats covered.

But to be sure i covered your use case, can you tell me a real example of folder/file structure.
And i will add it to the the unit tests .

@chanjunweimy
Copy link
Author

@sachatrauwaen from my understanding, you need to specify the exact file path after embedded the file to keep the structure right? i.e.

 [Fact]
        public void Should_Get_Embedded_Resource_With_Dash_In_Name()
        {
            var filepath = "/MyApp/MyResources/js/MyScriptFile-2.js";
            var resource = _embeddedResourceManager.GetResource(filepath);
            var filename = System.IO.Path.GetFileName(filepath);
            var extension = System.IO.Path.GetExtension(filepath);

            resource.ShouldNotBeNull();
            Assert.True(resource.Assembly == GetType().GetAssembly());
            Assert.True(resource.Content.Length > 0);
            Assert.EndsWith(filename, resource.FileName);
            Assert.True(resource.FileExtension == extension.Substring(1)); // without dot
        }

However, what I mean is to download the file without knowing the file path.

You can get example folders here: https://github.com/chanjunweimy/abp_plugin_with_ui/tree/master/aspnet-core/Todo.DemoPlugin/Todo.DemoPlugin.AngularUI/src

Thank you.

Regards,
Jun Wei

@sachatrauwaen
Copy link
Contributor

@chanjunweimy You mean list the files in a folder with dowload links ?

@chanjunweimy
Copy link
Author

@sachatrauwaen nope, I mean just downloading an embedded folder.

@robsiera
Copy link

@chanjunweimy can you be more precise? Where in the code sample that you pointed too can one find an example of what you mean?

@chanjunweimy
Copy link
Author

chanjunweimy commented Dec 20, 2017

@robsiera , sorry for the late reply, I have been a little bit busy.
Downloading the whole embedded folder is already the use case, I can't find any more precise sentence for that. :D

Currently in the pull request, embedded resources is added to csproj like this:

<ItemGroup>
     <EmbeddedResource Include="Resources\Embedded\MyResources\js-dash\MyScriptFile.js" />
     <EmbeddedResource Include="Resources\Embedded\MyResources\js_underscore\MyScriptFile.js" />
</ItemGroup>

But imagine if you just want to embed the whole "Resources" folder, what you want to do is probably something like this:

<ItemGroup>
     <EmbeddedResource Include="Resources\**\*.*" />
</ItemGroup>

Now that you have embedded everything, how can you download the whole "Resources" folder, by maybe specifying something like:

    var filepath = "Resources";
    var resource = _embeddedResourceManager.GetResource(filepath);

Now, you might say that maybe I should request this from the Microsoft team, not here: you might be right, but I think this is also one of the common use case. ^^

As a workaround, I suggest doing:

<ItemGroup>
     <EmbeddedResource Include="Resources.zip" />
</ItemGroup>

Then we can get:

    var filepath = "Resources.zip";
    var resource = _embeddedResourceManager.GetResource(filepath);

Problem solved.

You might wonder how we can do something like this:

var filepath = "/MyApp/MyResources/js/MyScriptFile1.js";
var resource = _embeddedResourceManager.GetResource(filepath);

That's simple:

  1. Download the zip file
  2. Unzip the zip file into a folder
  3. Load the file

You can unzip it by:

var filepath = "Resources.zip";
var resource = _embeddedResourceManager.GetResource(filepath);
...
//save resource to source file path
//set dest as another file path
ZipFile.ExtractToDirectory(Source, Dest);

I have a similar implementation:
Download resource part: https://github.com/chanjunweimy/abp_plugin_with_ui/tree/master/aspnet-core/src/Todo.MainProject.Web.Host/Services
and
https://github.com/chanjunweimy/abp_plugin_with_ui/blob/master/aspnet-core/src/Todo.MainProject.Web.Host/Controllers/PluginController.cs
Unzip part: https://github.com/chanjunweimy/abp_plugin_with_ui/blob/master/plugin-downloader/Todo.PluginDownloader/PluginHandler.cs

Hope that this is detailed enough ^^

Thank you.

@robsiera
Copy link

robsiera commented Dec 20, 2017

@chanjunweimy thank you for the extensive explanation.

I'm following this thread because I have a similar issue as your initial issue description, only not with hyphens as you experienced, but with dots. I was hopeful that this issue would be fixed soon with the PR of @sachatrauwaen. But I see that @hikalkan has now labeled it as an enhancement.

You initial issue description was a bug. It worked v3.1.2 but is broken now.
It is fair to say that your request "to be able to download a complete folder" is actually a feature request? Or did this feature already work in v3.1.2?
If not, I would strongly recommend to separate this feature request from the bug fix PR of @sachatrauwaen .

@chanjunweimy
Copy link
Author

@robsiera no problem. ^^ Actually @hikalkan has already solved the initial issue at this commit , but it is not yet released if I am not wrong. This is why this issue is marked as closed.

However, we continue the discussion in order to come out with a good method to implement GetDirectoryContents as specified in a new issue #2687 opened by @hikalkan . The pull request submitted by @sachatrauwaen is targeted for this issue I believe, that's why I point out the potential issue of "not being able to download a complete folder" and suggest using ".zip" as a workaround.

Of course, none of these methods are perfect yet. Hopefully Microsoft implements something to solve this issue soon. ^^

@acjh
Copy link
Contributor

acjh commented Dec 20, 2017

it is not yet released if I am not wrong

It was released in ABP v3.2.0. Current version is ABP v3.2.5.

@sachatrauwaen
Copy link
Contributor

It solves the issue with dashes "-" in filenames but introduce a new issue for dashes in foldernames.
Because they are transformed by dotnet to underscores "_".
And double dots "." in filenames is still a issue.

Can this issue reopened or is it better to create a new one for it ?

@acjh
Copy link
Contributor

acjh commented Dec 20, 2017

No need for either. @hikalkan will review your PR in time to come.

@chanjunweimy
Copy link
Author

@acjh Great to know! Thanks! ^^
@robsiera If you are still facing issue, as a workaround, you can implement the IFileProvider yourself, see: https://github.com/chanjunweimy/abp_plugin_with_ui/blob/master/aspnet-core/src/Todo.MainProject.Web.Host/Services/PluginEmbeddedResourceFileProvider.cs

Good luck!

Cheers,
Jun Wei

@chanjunweimy
Copy link
Author

An update from the Microsoft team based on aspnet/FileSystem/#184, the embedded file issue is now solved! We can wait for their release and this issue will be solved.

@hikalkan
Copy link
Member

That's very good and seems solving all problems: aspnet/FileSystem#184 (comment)

We will try it when MS releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants