Skip to content

Commit

Permalink
misc: exclude system drive formats
Browse files Browse the repository at this point in the history
  • Loading branch information
@revam
revam committed Oct 8, 2023
1 parent a9fa0ad commit f92cfeb
Showing 1 changed file with 56 additions and 4 deletions.
60 changes: 56 additions & 4 deletions Shoko.Server/API/v3/Controllers/FolderController.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,69 @@ namespace Shoko.Server.API.v3.Controllers;
[Authorize]
public class FolderController : BaseController
{
private static HashSet<string> ExcludedFormats = new()
{
"msdos", // fat32 - might be overkill, but the esp (u)efi partition is usually formatted as such.

This comment has been minimized.

Sign in to view

Copy link
@revam

revam Oct 9, 2023 •
edited
Loading

Author Member

This entry may be removed, as it will only ever be relevant to bare-metal installs (which are technically not supported but do work)
"ramfs",
"configfs",
"fusectl",
"tracefs",
"hugetlbfs",
"mqueue",
"debugfs",
"binfmt_misc",
"devpts",
"pstorefs",
"bpf_fs",
"cgroup2fs",
"securityfs",
"proc",
"tmpfs",
"sysfs",
};

This comment has been minimized.

Sign in to view

Copy link
@Cazzar

Cazzar Oct 9, 2023

Member

This should be stored somewhere other than in the controller logic, some resource/helper class would be the better place, or even settings.
Else we need to try to remember that this is stored in the controller

[HttpGet("Drives")]
public ActionResult<IEnumerable<Drive>> GetDrives()
{
return DriveInfo.GetDrives().Select(d =>
{
if (d.DriveType == DriveType.Unknown)
return null;

string fullName;
try
{
fullName = d.RootDirectory.FullName;
}
catch
{
return null;
}

string driveFormat;
try
{
driveFormat = d.DriveFormat;
}
catch
{
return null;
}

foreach (var format in ExcludedFormats)
{
if (driveFormat == format)
return null;
}

This comment has been minimized.

Sign in to view

Copy link
@Cazzar

Cazzar Oct 9, 2023

Member

Would all this logic be better to be put somewhere else other than the API Route?

In case we use it elsewhere.

This comment has been minimized.

Sign in to view

Copy link
@Cazzar

Cazzar Oct 9, 2023 •
edited
Loading

Member

Also looking at the above logic, wouldn't it be better to do something along the lines of

DriveInfo.GetDrives()
    .Where(d => d.DriveType != DriveType.Unknown)
    .Where(d => d.RootDirectory.FullName is not null)
    .Where(d => d.DriveFormat is not null && !ExcludedFormats.Contains(d.driveFormat)

This comment has been minimized.

Sign in to view

Copy link
@revam

revam Oct 9, 2023

Author Member

The problem is that they won't be null, it will throw.

This comment has been minimized.

Sign in to view

Copy link
@revam

revam Oct 9, 2023

Author Member

For both the full name and drive format.

This comment has been minimized.

Sign in to view

Copy link
@revam

revam Oct 9, 2023 •
edited
Loading

Author Member

Would all this logic be better to be put somewhere else other than the API Route?

In case we use it elsewhere

We can create a helper, but so far it's only used in the v3 api (though similar may exist for v1, haven't checked) to give the user the starting point(s) to finding their import folder path. Yes, we can probably re-use this as the internal logic for later if we decide to do use different models for v4 or later but keeping the same internal logic.

This comment has been minimized.

Sign in to view

Copy link
@Cazzar

Cazzar Oct 9, 2023

Member

Then why are we using them all of a sudden? We have never needed them before.

Other than that, if you look at the example on MSDN they use DriveInfo.IsReady to gate that, so we reasonably could.

The example I gave there was an example, especially the case of using HashSet.Contains as the proper way of handling that.

This comment has been minimized.

Sign in to view

Copy link
@Cazzar

Cazzar Oct 9, 2023 •
edited
Loading

Member

As for the comment around putting that lgoic elsewhere, there is generally an API design comment about minimising the amount of logic in the controller.

Massive controller files are always a bit of a pain to read and handle too when you are trying to work out what the hell is going on.

Also add to the fact that V1 has a different set of logic there would make it quite... annoying/fiddly. because, yes, v1 has logic that uses the mountpoints simlilarly

This comment has been minimized.

Sign in to view

Copy link
@revam

revam Oct 9, 2023

Author Member

Then why are we using them all of a sudden? We have never needed them before.

Other than that, if you look at the example on MSDN they use DriveInfo.IsReady to gate that, so we reasonably could.

IsReady doesn't seem like it will help for PathTooLongExceptions and UnauthorizedAccessExceptions, the former which can be tbrowen when accessing the full name, and the latter which can be thrown when accessing the file system type (drive format).

The example I gave there was an example, especially the case of using HashSet.Contains as the proper way of handling that.

It can be converted to a simple .Contains check, yes. If was just getting late for me last night so i didn't think to simplify it.

This comment has been minimized.

Sign in to view

Copy link
@Cazzar

Cazzar Oct 9, 2023

Member

But, a mounted path, wouldn't throw reasonably a PathTooLongException? This would be for the case where you manually create the DirectoryInfo with a path that's too long.

This comment has been minimized.

Sign in to view

Copy link
@revam

revam Oct 9, 2023

Author Member

Maybe not, but i did encounter an UnauthorizedAccessException during my local testing, so the last try...catch statement is needed.

ChildItems childItems = null;
try
{
childItems = d.IsReady
? new ChildItems()
{
Files = d.RootDirectory.GetFiles()?.Length ?? 0,
Folders = d.RootDirectory.GetDirectories()?.Length ?? 0
Folders = d.RootDirectory.GetDirectories()?.Length ?? 0,
}
: null;
}
Expand All @@ -38,12 +88,14 @@ public ActionResult<IEnumerable<Drive>> GetDrives()

return new Drive()
{
Path = d.RootDirectory.FullName,
Path = fullName,
IsAccessible = childItems != null,
Sizes = childItems,
Type = d.DriveType
Type = d.DriveType,
};
}).ToList();
})
.Where(mountPoint => mountPoint != null)
.ToList();
}

[HttpGet]
Expand Down

0 comments on commit f92cfeb

Please sign in to comment.