From 6f1b740c8f01fb8cdeebd55272764415cb759e94 Mon Sep 17 00:00:00 2001 From: SquidDev Date: Mon, 3 Jun 2019 19:58:16 +0100 Subject: [PATCH] Un-localise paths in mount error messages This is probably a little more complex than it needs to be, as we try to handle the mounts as well (which generally don't error). Fixes #229 --- .../filesystem/FileOperationException.java | 41 ++++++++ .../core/filesystem/ComboMount.java | 9 +- .../core/filesystem/EmptyMount.java | 5 +- .../core/filesystem/FileMount.java | 29 +++--- .../core/filesystem/FileSystem.java | 86 +++++++++-------- .../core/filesystem/JarMount.java | 7 +- .../resources/test-rom/spec/apis/fs_spec.lua | 95 +++++++++++++++++++ 7 files changed, 211 insertions(+), 61 deletions(-) create mode 100644 src/main/java/dan200/computercraft/api/filesystem/FileOperationException.java diff --git a/src/main/java/dan200/computercraft/api/filesystem/FileOperationException.java b/src/main/java/dan200/computercraft/api/filesystem/FileOperationException.java new file mode 100644 index 000000000..79c873051 --- /dev/null +++ b/src/main/java/dan200/computercraft/api/filesystem/FileOperationException.java @@ -0,0 +1,41 @@ +/* + * This file is part of ComputerCraft - http://www.computercraft.info + * Copyright Daniel Ratcliffe, 2011-2019. Do not distribute without permission. + * Send enquiries to dratcliffe@gmail.com + */ + +package dan200.computercraft.api.filesystem; + +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import java.io.IOException; +import java.util.Objects; + +/** + * An {@link IOException} which occurred on a specific file. + * + * This may be thrown from a {@link IMount} or {@link IWritableMount} to give more information about a failure. + */ +public class FileOperationException extends IOException +{ + private static final long serialVersionUID = -8809108200853029849L; + + private String filename; + + public FileOperationException( @Nullable String filename, @Nonnull String message ) + { + super( Objects.requireNonNull( message, "message cannot be null" ) ); + this.filename = filename; + } + + public FileOperationException( String message ) + { + super( Objects.requireNonNull( message, "message cannot be null" ) ); + } + + @Nullable + public String getFilename() + { + return filename; + } +} diff --git a/src/main/java/dan200/computercraft/core/filesystem/ComboMount.java b/src/main/java/dan200/computercraft/core/filesystem/ComboMount.java index 1dce543c1..58bc62047 100644 --- a/src/main/java/dan200/computercraft/core/filesystem/ComboMount.java +++ b/src/main/java/dan200/computercraft/core/filesystem/ComboMount.java @@ -6,6 +6,7 @@ package dan200.computercraft.core.filesystem; +import dan200.computercraft.api.filesystem.FileOperationException; import dan200.computercraft.api.filesystem.IMount; import javax.annotation.Nonnull; @@ -95,7 +96,7 @@ else if( foundDirs > 1 ) } else { - throw new IOException( "/" + path + ": Not a directory" ); + throw new FileOperationException( path, "Not a directory" ); } } @@ -110,7 +111,7 @@ public long getSize( @Nonnull String path ) throws IOException return part.getSize( path ); } } - throw new IOException( "/" + path + ": No such file" ); + throw new FileOperationException( path, "No such file" ); } @Nonnull @@ -126,7 +127,7 @@ public InputStream openForRead( @Nonnull String path ) throws IOException return part.openForRead( path ); } } - throw new IOException( "/" + path + ": No such file" ); + throw new FileOperationException( path, "No such file" ); } @Nonnull @@ -141,6 +142,6 @@ public ReadableByteChannel openChannelForRead( @Nonnull String path ) throws IOE return part.openChannelForRead( path ); } } - throw new IOException( "/" + path + ": No such file" ); + throw new FileOperationException( path, "No such file" ); } } diff --git a/src/main/java/dan200/computercraft/core/filesystem/EmptyMount.java b/src/main/java/dan200/computercraft/core/filesystem/EmptyMount.java index 634a76649..5ad38de9b 100644 --- a/src/main/java/dan200/computercraft/core/filesystem/EmptyMount.java +++ b/src/main/java/dan200/computercraft/core/filesystem/EmptyMount.java @@ -6,6 +6,7 @@ package dan200.computercraft.core.filesystem; +import dan200.computercraft.api.filesystem.FileOperationException; import dan200.computercraft.api.filesystem.IMount; import javax.annotation.Nonnull; @@ -44,7 +45,7 @@ public long getSize( @Nonnull String path ) @Deprecated public InputStream openForRead( @Nonnull String path ) throws IOException { - throw new IOException( "/" + path + ": No such file" ); + throw new FileOperationException( path, "No such file" ); } @Nonnull @@ -52,6 +53,6 @@ public InputStream openForRead( @Nonnull String path ) throws IOException @Deprecated public ReadableByteChannel openChannelForRead( @Nonnull String path ) throws IOException { - throw new IOException( "/" + path + ": No such file" ); + throw new FileOperationException( path, "No such file" ); } } diff --git a/src/main/java/dan200/computercraft/core/filesystem/FileMount.java b/src/main/java/dan200/computercraft/core/filesystem/FileMount.java index 45cad041e..58c66228f 100644 --- a/src/main/java/dan200/computercraft/core/filesystem/FileMount.java +++ b/src/main/java/dan200/computercraft/core/filesystem/FileMount.java @@ -7,6 +7,7 @@ package dan200.computercraft.core.filesystem; import com.google.common.collect.Sets; +import dan200.computercraft.api.filesystem.FileOperationException; import dan200.computercraft.api.filesystem.IWritableMount; import javax.annotation.Nonnull; @@ -167,12 +168,12 @@ public void list( @Nonnull String path, @Nonnull List contents ) throws { if( !created() ) { - if( !path.isEmpty() ) throw new IOException( "/" + path + ": Not a directory" ); + if( !path.isEmpty() ) throw new FileOperationException( path, "Not a directory" ); return; } File file = getRealPath( path ); - if( !file.exists() || !file.isDirectory() ) throw new IOException( "/" + path + ": Not a directory" ); + if( !file.exists() || !file.isDirectory() ) throw new FileOperationException( path, "Not a directory" ); String[] paths = file.list(); for( String subPath : paths ) @@ -194,7 +195,7 @@ public long getSize( @Nonnull String path ) throws IOException if( file.exists() ) return file.isDirectory() ? 0 : file.length(); } - throw new IOException( "/" + path + ": No such file" ); + throw new FileOperationException( path, "No such file" ); } @Nonnull @@ -208,7 +209,7 @@ public InputStream openForRead( @Nonnull String path ) throws IOException if( file.exists() && !file.isDirectory() ) return new FileInputStream( file ); } - throw new IOException( "/" + path + ": No such file" ); + throw new FileOperationException( path, "No such file" ); } @Nonnull @@ -221,7 +222,7 @@ public ReadableByteChannel openChannelForRead( @Nonnull String path ) throws IOE if( file.exists() && !file.isDirectory() ) return FileChannel.open( file.toPath(), READ_OPTIONS ); } - throw new IOException( "/" + path + ": No such file" ); + throw new FileOperationException( path, "No such file" ); } // IWritableMount implementation @@ -233,7 +234,7 @@ public void makeDirectory( @Nonnull String path ) throws IOException File file = getRealPath( path ); if( file.exists() ) { - if( !file.isDirectory() ) throw new IOException( "/" + path + ": File exists" ); + if( !file.isDirectory() ) throw new FileOperationException( path, "File exists" ); return; } @@ -247,7 +248,7 @@ public void makeDirectory( @Nonnull String path ) throws IOException if( getRemainingSpace() < dirsToCreate * MINIMUM_FILE_SIZE ) { - throw new IOException( "/" + path + ": Out of space" ); + throw new FileOperationException( path, "Out of space" ); } if( file.mkdirs() ) @@ -256,14 +257,14 @@ public void makeDirectory( @Nonnull String path ) throws IOException } else { - throw new IOException( "/" + path + ": Access denied" ); + throw new FileOperationException( path, "Access denied" ); } } @Override public void delete( @Nonnull String path ) throws IOException { - if( path.isEmpty() ) throw new IOException( "/" + path + ": Access denied" ); + if( path.isEmpty() ) throw new FileOperationException( path, "Access denied" ); if( created() ) { @@ -319,7 +320,7 @@ public WritableByteChannel openChannelForWrite( @Nonnull String path ) throws IO { create(); File file = getRealPath( path ); - if( file.exists() && file.isDirectory() ) throw new IOException( "/" + path + ": Cannot write to directory" ); + if( file.exists() && file.isDirectory() ) throw new FileOperationException( path, "Cannot write to directory" ); if( file.exists() ) { @@ -327,7 +328,7 @@ public WritableByteChannel openChannelForWrite( @Nonnull String path ) throws IO } else if( getRemainingSpace() < MINIMUM_FILE_SIZE ) { - throw new IOException( "/" + path + ": Out of space" ); + throw new FileOperationException( path, "Out of space" ); } m_usedSpace += MINIMUM_FILE_SIZE; @@ -340,12 +341,12 @@ public WritableByteChannel openChannelForAppend( @Nonnull String path ) throws I { if( !created() ) { - throw new IOException( "/" + path + ": No such file" ); + throw new FileOperationException( path, "No such file" ); } File file = getRealPath( path ); - if( !file.exists() ) throw new IOException( "/" + path + ": No such file" ); - if( file.isDirectory() ) throw new IOException( "/" + path + ": Cannot write to directory" ); + if( !file.exists() ) throw new FileOperationException( path, "No such file" ); + if( file.isDirectory() ) throw new FileOperationException( path, "Cannot write to directory" ); // Allowing seeking when appending is not recommended, so we use a separate channel. return new WritableCountingChannel( diff --git a/src/main/java/dan200/computercraft/core/filesystem/FileSystem.java b/src/main/java/dan200/computercraft/core/filesystem/FileSystem.java index e36d41af3..98185cc96 100644 --- a/src/main/java/dan200/computercraft/core/filesystem/FileSystem.java +++ b/src/main/java/dan200/computercraft/core/filesystem/FileSystem.java @@ -8,6 +8,7 @@ import com.google.common.io.ByteStreams; import dan200.computercraft.ComputerCraft; +import dan200.computercraft.api.filesystem.FileOperationException; import dan200.computercraft.api.filesystem.IFileSystem; import dan200.computercraft.api.filesystem.IMount; import dan200.computercraft.api.filesystem.IWritableMount; @@ -107,7 +108,7 @@ public boolean isDirectory( String path ) throws FileSystemException } catch( IOException e ) { - throw new FileSystemException( e.getMessage() ); + throw localExceptionOf( e ); } } @@ -122,12 +123,12 @@ public void list( String path, List contents ) throws FileSystemExceptio } else { - throw new FileSystemException( "/" + path + ": Not a directory" ); + throw localExceptionOf( path, "Not a directory" ); } } catch( IOException e ) { - throw new FileSystemException( e.getMessage() ); + throw localExceptionOf( e ); } } @@ -149,12 +150,12 @@ public long getSize( String path ) throws FileSystemException } else { - throw new FileSystemException( "/" + path + ": No such file" ); + throw localExceptionOf( path, "No such file" ); } } catch( IOException e ) { - throw new FileSystemException( e.getMessage() ); + throw localExceptionOf( e ); } } @@ -169,12 +170,12 @@ public ReadableByteChannel openForRead( String path ) throws FileSystemException } else { - throw new FileSystemException( "/" + path + ": No such file" ); + throw localExceptionOf( path, "No such file" ); } } catch( IOException e ) { - throw new FileSystemException( e.getMessage() ); + throw localExceptionOf( e ); } } @@ -182,19 +183,14 @@ public ReadableByteChannel openForRead( String path ) throws FileSystemException public void makeDirectory( String path ) throws FileSystemException { - if( m_writableMount == null ) - { - throw new FileSystemException( "/" + path + ": Access denied" ); - } + if( m_writableMount == null ) throw exceptionOf( path, "Access denied" ); + + path = toLocal( path ); try { - path = toLocal( path ); if( m_mount.exists( path ) ) { - if( !m_mount.isDirectory( path ) ) - { - throw new FileSystemException( "/" + path + ": File exists" ); - } + if( !m_mount.isDirectory( path ) ) throw localExceptionOf( path, "File exists" ); } else { @@ -203,16 +199,14 @@ public void makeDirectory( String path ) throws FileSystemException } catch( IOException e ) { - throw new FileSystemException( e.getMessage() ); + throw localExceptionOf( e ); } } public void delete( String path ) throws FileSystemException { - if( m_writableMount == null ) - { - throw new FileSystemException( "/" + path + ": Access denied" ); - } + if( m_writableMount == null ) throw exceptionOf( path, "Access denied" ); + try { path = toLocal( path ); @@ -227,22 +221,20 @@ public void delete( String path ) throws FileSystemException } catch( IOException e ) { - throw new FileSystemException( e.getMessage() ); + throw localExceptionOf( e ); } } public WritableByteChannel openForWrite( String path ) throws FileSystemException { - if( m_writableMount == null ) - { - throw new FileSystemException( "/" + path + ": Access denied" ); - } + if( m_writableMount == null ) throw exceptionOf( path, "Access denied" ); + + path = toLocal( path ); try { - path = toLocal( path ); if( m_mount.exists( path ) && m_mount.isDirectory( path ) ) { - throw new FileSystemException( "/" + path + ": Cannot write to directory" ); + throw localExceptionOf( path, "Cannot write to directory" ); } else { @@ -263,19 +255,17 @@ public WritableByteChannel openForWrite( String path ) throws FileSystemExceptio } catch( IOException e ) { - throw new FileSystemException( e.getMessage() ); + throw localExceptionOf( e ); } } public WritableByteChannel openForAppend( String path ) throws FileSystemException { - if( m_writableMount == null ) - { - throw new FileSystemException( "/" + path + ": Access denied" ); - } + if( m_writableMount == null ) throw exceptionOf( path, "Access denied" ); + + path = toLocal( path ); try { - path = toLocal( path ); if( !m_mount.exists( path ) ) { if( !path.isEmpty() ) @@ -290,7 +280,7 @@ public WritableByteChannel openForAppend( String path ) throws FileSystemExcepti } else if( m_mount.isDirectory( path ) ) { - throw new FileSystemException( "/" + path + ": Cannot write to directory" ); + throw localExceptionOf( path, "Cannot write to directory" ); } else { @@ -303,16 +293,36 @@ else if( m_mount.isDirectory( path ) ) } catch( IOException e ) { - throw new FileSystemException( e.getMessage() ); + throw localExceptionOf( e ); } } - // private members - private String toLocal( String path ) { return FileSystem.toLocal( path, m_location ); } + + private FileSystemException localExceptionOf( IOException e ) + { + if( !m_location.isEmpty() && e instanceof FileOperationException ) + { + FileOperationException ex = (FileOperationException) e; + if( ex.getFilename() != null ) return localExceptionOf( ex.getFilename(), ex.getMessage() ); + } + + return new FileSystemException( e.getMessage() ); + } + + private FileSystemException localExceptionOf( String path, String message ) + { + if( !m_location.isEmpty() ) path = path.isEmpty() ? m_location : m_location + "/" + path; + return exceptionOf( path, message ); + } + + private static FileSystemException exceptionOf( String path, String message ) + { + return new FileSystemException( "/" + path + ": " + message ); + } } private final FileSystemWrapperMount m_wrapper = new FileSystemWrapperMount( this ); diff --git a/src/main/java/dan200/computercraft/core/filesystem/JarMount.java b/src/main/java/dan200/computercraft/core/filesystem/JarMount.java index 941837c94..3bedd1eca 100644 --- a/src/main/java/dan200/computercraft/core/filesystem/JarMount.java +++ b/src/main/java/dan200/computercraft/core/filesystem/JarMount.java @@ -9,6 +9,7 @@ import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; import com.google.common.io.ByteStreams; +import dan200.computercraft.api.filesystem.FileOperationException; import dan200.computercraft.api.filesystem.IMount; import dan200.computercraft.core.apis.handles.ArrayByteChannel; import dan200.computercraft.shared.util.IoUtil; @@ -166,7 +167,7 @@ public boolean isDirectory( @Nonnull String path ) public void list( @Nonnull String path, @Nonnull List contents ) throws IOException { FileEntry file = get( path ); - if( file == null || !file.isDirectory() ) throw new IOException( "/" + path + ": Not a directory" ); + if( file == null || !file.isDirectory() ) throw new FileOperationException( path, "Not a directory" ); file.list( contents ); } @@ -176,7 +177,7 @@ public long getSize( @Nonnull String path ) throws IOException { FileEntry file = get( path ); if( file != null ) return file.size; - throw new IOException( "/" + path + ": No such file" ); + throw new FileOperationException( path, "No such file" ); } @Nonnull @@ -218,7 +219,7 @@ public ReadableByteChannel openChannelForRead( @Nonnull String path ) throws IOE } } - throw new IOException( "/" + path + ": No such file" ); + throw new FileOperationException( path, "No such file" ); } private static class FileEntry diff --git a/src/test/resources/test-rom/spec/apis/fs_spec.lua b/src/test/resources/test-rom/spec/apis/fs_spec.lua index 883b8c575..157e0be88 100644 --- a/src/test/resources/test-rom/spec/apis/fs_spec.lua +++ b/src/test/resources/test-rom/spec/apis/fs_spec.lua @@ -11,4 +11,99 @@ describe("The fs library", function() expect.error(fs.complete, "", "", true, 1):eq("bad argument #4 (expected boolean, got number)") end) end) + + describe("fs.list", function() + it("fails on files", function() + expect.error(fs.list, "rom/startup.lua"):eq("/rom/startup.lua: Not a directory") + expect.error(fs.list, "startup.lua"):eq("/startup.lua: Not a directory") + end) + + it("fails on non-existent nodes", function() + expect.error(fs.list, "rom/x"):eq("/rom/x: Not a directory") + expect.error(fs.list, "x"):eq("/x: Not a directory") + end) + end) + + describe("fs.list", function() + it("fails on files", function() + expect.error(fs.list, "rom/startup.lua"):eq("/rom/startup.lua: Not a directory") + expect.error(fs.list, "startup.lua"):eq("/startup.lua: Not a directory") + end) + + it("fails on non-existent nodes", function() + expect.error(fs.list, "rom/x"):eq("/rom/x: Not a directory") + expect.error(fs.list, "x"):eq("/x: Not a directory") + end) + end) + + describe("fs.getSize", function() + it("fails on non-existent nodes", function() + expect.error(fs.getSize, "rom/x"):eq("/rom/x: No such file") + expect.error(fs.getSize, "x"):eq("/x: No such file") + end) + end) + + describe("fs.open", function() + describe("reading", function() + it("fails on directories", function() + expect { fs.open("rom", "r") }:same { nil, "/rom: No such file" } + expect { fs.open("", "r") }:same { nil, "/: No such file" } + end) + + it("fails on non-existent nodes", function() + expect { fs.open("rom/x", "r") }:same { nil, "/rom/x: No such file" } + expect { fs.open("x", "r") }:same { nil, "/x: No such file" } + end) + end) + + describe("writing", function() + it("fails on directories", function() + expect { fs.open("", "w") }:same { nil, "/: Cannot write to directory" } + end) + + it("fails on read-only mounts", function() + expect { fs.open("rom/x", "w") }:same { nil, "/rom/x: Access denied" } + end) + end) + + describe("appending", function() + it("fails on directories", function() + expect { fs.open("", "a") }:same { nil, "/: Cannot write to directory" } + end) + + it("fails on read-only mounts", function() + expect { fs.open("rom/x", "a") }:same { nil, "/rom/x: Access denied" } + end) + end) + end) + + describe("fs.makeDir", function() + it("fails on files", function() + expect.error(fs.makeDir, "startup.lua"):eq("/startup.lua: File exists") + end) + + it("fails on read-only mounts", function() + expect.error(fs.makeDir, "rom/x"):eq("/rom/x: Access denied") + end) + end) + + describe("fs.delete", function() + it("fails on read-only mounts", function() + expect.error(fs.delete, "rom/x"):eq("/rom/x: Access denied") + end) + end) + + describe("fs.copy", function() + it("fails on read-only mounts", function() + expect.error(fs.copy, "rom", "rom/startup"):eq("/rom/startup: Access denied") + end) + end) + + describe("fs.move", function() + it("fails on read-only mounts", function() + expect.error(fs.move, "rom", "rom/move"):eq("Access denied") + expect.error(fs.move, "test-files", "rom/move"):eq("Access denied") + expect.error(fs.move, "rom", "test-files"):eq("Access denied") + end) + end) end)