Web API for reading file system

Question

I will have this code reviewed by back-end devs tomorrow as part of an interview process tomorrow and would like comments on how to improve A) structure B) performance. For structure, it's too late to switch from the "get all" approach as this would involve rewriting the entire front end. Mainly I am wondering if I should move a bunch of the logic into a data service project and if so, do I build a console app to host it or how would that work? Also open to removing the custom routes and letting the HTTP verbs handle what they're supposed to if it's important and doable in a short time.

public class FileSystemController : ApiController
{
    private int _currentId;
    private readonly string _folderName = ConfigurationManager.AppSettings["FileSystemFolder"];
    private readonly string _basePath = HttpContext.Current.Server.MapPath(@"~\" + ConfigurationManager.AppSettings["FileSystemFolder"]);

    // GET: api/FileSystem
    public IEnumerable<FileSystemNode> Get()
    {

        List<FileSystemNode> nodes = new List<FileSystemNode>();
        var baseNode = AddFolderContents(_basePath, _folderName, _folderName, null);
        nodes.Add(baseNode);
        return nodes.ToArray();
    }

    private FileSystemNode AddFolderContents(string serverPath, string publicPath, string name, int? parentId)
    {
        var node = new FileSystemNode
        {
            Id = _currentId,
            ParentId = parentId,
            Name = name,
            Path = publicPath,
            Files = new List<FileObject>(),
            Children = new List<FileSystemNode>()
        };

        _currentId++;

        DirectoryInfo dInfo = new DirectoryInfo(serverPath);
        foreach (FileInfo file in dInfo.GetFiles())
        {
            node.Files.Add(new FileObject
            {
                Name = file.Name,
                Size = String.Format("{0:n0}", file.Length),
                ParentId = node.Id,
                Path = publicPath
            });
        }

        foreach (DirectoryInfo folder in dInfo.GetDirectories())
        {
            var childServerPath = serverPath + @"\" + folder.Name;
            node.Children.Add(AddFolderContents(childServerPath, publicPath + @"\" + folder.Name, folder.Name, node.Id));
        }

        return node;
    }

    [Route("api/FileSystem/Upload")]
    [HttpPost]
    public string Upload()
    {
        try
        {
            var request = HttpContext.Current.Request;
            var fileName = request.Headers["filename"];
            var filePath = request.Headers["path"];
            var fullPath = HttpContext.Current.Server.MapPath(@"~\" + filePath + @"\" + fileName);

            using (var fs = new FileStream(fullPath, FileMode.Create))
            {
                request.InputStream.CopyTo(fs);
            }

            return "File " + fileName + " uploaded successfully";
        }
        catch (Exception e)
        {
            Console.WriteLine(e);
            return "File upload failed";
        }

    }

    [Route("api/FileSystem/Delete")]
    [HttpPost]
    public string Delete()
    {
        try
        {
            var request = HttpContext.Current.Request;
            var fileName = request.Headers["filename"];
            var filePath = request.Headers["path"];
            var fullPath = HttpContext.Current.Server.MapPath(@"~\" + filePath + @"\" + fileName);

            File.Delete(fullPath);

            return "File " + fileName + " deleted successfully";
        }
        catch (Exception e)
        {
            Console.WriteLine(e);
            return "File deletion failed";
        }
    }
}

Here are the front-end calls to upload and delete. I imagine I could just hit "api/FileSystem" URL with the proper verbs (POST and DELETE) and get the same results? (Note I've already had the front end review, so while this may not be the ideal way to do things, I'm really only asking about making it compliant with the API)

 self.onUploadClicked = function() {

    var fileInput = $('#uploadFileName');

    if (fileInput[0].files[0]) {
        var file = fileInput[0].files[0];

        var xhr = new XMLHttpRequest();
        xhr.open("POST", "/api/filesystem/Upload", false);
        xhr.setRequestHeader("filename", file.name);
        xhr.setRequestHeader("path", this.Path);

        showSpinner('Uploading...');
        xhr.send(file);
        hideSpinner();

        self.onRefreshClicked(xhr.responseText);            
    } else {
        alert("Please select a file to upload");
    }
};

self.onDeleteClicked = function() {
    if (confirm('Are you sure you want to permanently delete ' + this.Name + '?')) {
        var xhr = new XMLHttpRequest();
        xhr.open("POST", "/api/filesystem/Delete", false);
        xhr.setRequestHeader("filename", this.Name);
        xhr.setRequestHeader("path", this.Path);

        showSpinner('Deleting...');
        xhr.send();
        hideSpinner();

        self.onRefreshClicked(xhr.responseText);
    };
};

Answer 1

I think one of the most common errors in today times, is for example handling potential errors, as you try to do here:

[Route("api/FileSystem/Upload")]
[HttpPost]
public string Upload()
{
    try
    {
       // ... code
    }
    catch (Exception e)
    {
        Console.WriteLine(e);
        return "File upload failed";
    }

}

Why is this bad:

• You seem to want to catch all exceptions, not just specific ones that might be caught willingly ;
• You are writing the exception to the console, this cannot be seen as logging, and is pretty much useless once the system is up and running ;
• You reply with a simple string ;
• Since you don't do throw; afterwards, you will completely loose the stacktrace ;
• You are returning essentially a 200 OK result with an error message as a string masked as a valid response, any testing might be troublesome to set up.

Another approach for this would be to not catch any errors at all on the backend, let it fail, and let it return the full failed response. Asp.net has ExceptionFilters that can handle potential errors a lot better than how you have handled them here.

Related to this, the code on the JavaScript side seems to completely ignore potential errors, there is no check if your text contains the "File upload failed" message, so how is a user going to know that it failed (true it could be missing from the review).

Your JavaScript also has some interesting code, nl:

var xhr = new XMLHttpRequest();
xhr.open("POST", "/api/filesystem/Upload", false);
xhr.setRequestHeader("filename", file.name);
xhr.setRequestHeader("path", this.Path);

showSpinner('Uploading...');
xhr.send(file);
hideSpinner();

In all honestly, simply choosing that your xhr is synchronous might not seem a big problem on your local server, but your code has more problems, nl:

• JavaScript is singlethreaded, due to the fact that your xhr is synchronous, your spinner will never show, there was no time to update the DOM tree to show the spinner itself
• Path is PascalCased, javascript properties are typically camelCased
• There seems to be a mix of this / self, without real indications what this would be, a code comment mentioning why this is appropriate in that case, seems to be missing
• self.onRefreshClicked gets a string as a parameter, that seems to be unexpected from my point of view, why would an eventhandler be sent a string parameter?

Answer 2

It's a very bad idea to call the controller a FileSystem and inform the user that he's going to be working with well, a file system. He should not know how you are storing the data he uploads. You should not reveal this implementation.

A Web-API is a resource and should be seen by the client like one (whereever possible), this means your controller should be called FilesController. Whether you store the data in a database, a file system directly or in a cloud it's none of the user's business. Knowing the underlying technology he might try to exploit it.

Answer 3

 _currentId++;

Are you sure your id system works as expected? If I recall correctly, a new controller instance is created for each request. That means the id will get incremented, the class will go out of scope, and then the next call to the controller will create another entry with an id of 0.