Support renaming files directly without copying/deleting

In classic squid tradition: 20% code, and 80% test logic.

Closes #962. Alas, whoever reported this has deleted their account, so
they can't even be happy about it :(.
This commit is contained in:
Jonathan Coates 2022-12-04 21:59:30 +00:00
parent a9c0b02e3c
commit 020c5cd2d3
No known key found for this signature in database
GPG Key ID: B9E431FF07C98D06
15 changed files with 330 additions and 83 deletions

View File

@ -88,5 +88,5 @@ jobs:
./gradlew --configure-on-demand :core:test
- name: Parse test reports
run: python ./tools/parse-reports.py
run: python3 ./tools/parse-reports.py
if: ${{ failure() }}

View File

@ -106,6 +106,7 @@ jqwik-engine = { module = "net.jqwik:jqwik-engine", version.ref = "jqwik" }
junit-jupiter-api = { module = "org.junit.jupiter:junit-jupiter-api", version.ref = "junit" }
junit-jupiter-engine = { module = "org.junit.jupiter:junit-jupiter-engine", version.ref = "junit" }
junit-jupiter-params = { module = "org.junit.jupiter:junit-jupiter-params", version.ref = "junit" }
logback = { module = "ch.qos.logback:logback-classic", version.ref = "logback" }
# Build tools
cctJavadoc = { module = "cc.tweaked:cct-javadoc", version.ref = "cctJavadoc" }

View File

@ -112,6 +112,6 @@
(lint
(globals
:max sleep write
cct_test describe expect howlci fail it pending stub)))
cct_test describe expect howlci fail it pending stub before_each)))
(at /projects/web/src/mount/expr_template.lua (lint (globals :max __expr__)))

View File

@ -10,7 +10,6 @@
import java.io.IOException;
import java.io.OutputStream;
import java.nio.channels.WritableByteChannel;
import java.util.OptionalLong;
/**
* Represents a part of a virtual filesystem that can be mounted onto a computer using {@link IComputerAccess#mount(String, Mount)}
@ -40,6 +39,17 @@ public interface WritableMount extends Mount {
*/
void delete(String path) throws IOException;
/**
* Rename a file or directory, moving it from one path to another.
* <p>
* The destination path should not exist. The parent of the destination should exist and be a directory. If source
* and destination are the same, this method should do nothing.
*
* @param source The source file or directory to move.
* @param dest The destination path.
*/
void rename(String source, String dest) throws IOException;
/**
* Opens a file with a given path, and returns an {@link OutputStream} for writing to it.
*
@ -75,9 +85,7 @@ public interface WritableMount extends Mount {
*
* @return The capacity of this mount, in bytes.
*/
default OptionalLong getCapacity() {
return OptionalLong.empty();
}
long getCapacity();
/**
* Returns whether a file with a given path is read-only or not.

View File

@ -25,6 +25,7 @@ dependencies {
testImplementation(libs.bundles.test)
testRuntimeOnly(libs.bundles.testRuntime)
testRuntimeOnly(libs.logback)
}
tasks.processResources {

View File

@ -14,12 +14,14 @@
import java.io.File;
import java.io.IOException;
import java.nio.ByteBuffer;
import java.nio.channels.*;
import java.nio.channels.ClosedChannelException;
import java.nio.channels.NonReadableChannelException;
import java.nio.channels.SeekableByteChannel;
import java.nio.channels.WritableByteChannel;
import java.nio.file.*;
import java.nio.file.attribute.BasicFileAttributes;
import java.util.Collections;
import java.util.List;
import java.util.OptionalLong;
import java.util.Set;
public class FileMount implements WritableMount {
@ -185,7 +187,7 @@ public long getSize(String path) throws IOException {
public SeekableByteChannel openForRead(String path) throws IOException {
if (created()) {
var file = getRealPath(path);
if (file.exists() && !file.isDirectory()) return FileChannel.open(file.toPath(), READ_OPTIONS);
if (file.exists() && !file.isDirectory()) return Files.newByteChannel(file.toPath(), READ_OPTIONS);
}
throw new FileOperationException(path, "No such file");
@ -259,6 +261,22 @@ private void deleteRecursively(File file) throws IOException {
}
}
@Override
public void rename(String source, String dest) throws IOException {
var sourceFile = getRealPath(source);
var destFile = getRealPath(dest);
if (!sourceFile.exists()) throw new FileOperationException(source, "No such file");
if (destFile.exists()) throw new FileOperationException(dest, "File exists");
var sourcePath = sourceFile.toPath();
var destPath = destFile.toPath();
if (destPath.startsWith(sourcePath)) {
throw new FileOperationException(source, "Cannot move a directory inside itself");
}
Files.move(sourcePath, destPath);
}
@Override
public WritableByteChannel openForWrite(String path) throws IOException {
create();
@ -298,8 +316,8 @@ public long getRemainingSpace() {
}
@Override
public OptionalLong getCapacity() {
return OptionalLong.of(capacity - MINIMUM_FILE_SIZE);
public long getCapacity() {
return capacity - MINIMUM_FILE_SIZE;
}
private File getRealPath(String path) {

View File

@ -248,20 +248,19 @@ public synchronized void delete(String path) throws FileSystemException {
public synchronized void move(String sourcePath, String destPath) throws FileSystemException {
sourcePath = sanitizePath(sourcePath);
destPath = sanitizePath(destPath);
if (isReadOnly(sourcePath) || isReadOnly(destPath)) {
throw new FileSystemException("Access denied");
if (isReadOnly(sourcePath) || isReadOnly(destPath)) throw new FileSystemException("Access denied");
if (!exists(sourcePath)) throw new FileSystemException("No such file");
if (exists(destPath)) throw new FileSystemException("File exists");
if (contains(sourcePath, destPath)) throw new FileSystemException("Can't move a directory inside itself");
var mount = getMount(sourcePath);
if (mount == getMount(destPath)) {
mount.rename(sourcePath, destPath);
} else {
copy(sourcePath, destPath);
delete(sourcePath);
}
if (!exists(sourcePath)) {
throw new FileSystemException("No such file");
}
if (exists(destPath)) {
throw new FileSystemException("File exists");
}
if (contains(sourcePath, destPath)) {
throw new FileSystemException("Can't move a directory inside itself");
}
copy(sourcePath, destPath);
delete(sourcePath);
}
public synchronized void copy(String sourcePath, String destPath) throws FileSystemException {

View File

@ -14,6 +14,8 @@
import java.nio.channels.SeekableByteChannel;
import java.nio.channels.WritableByteChannel;
import java.nio.file.AccessDeniedException;
import java.nio.file.FileAlreadyExistsException;
import java.nio.file.NoSuchFileException;
import java.nio.file.attribute.BasicFileAttributes;
import java.util.List;
import java.util.OptionalLong;
@ -58,7 +60,7 @@ public long getFreeSpace() {
}
public OptionalLong getCapacity() {
return writableMount == null ? OptionalLong.empty() : writableMount.getCapacity();
return writableMount == null ? OptionalLong.empty() : OptionalLong.of(writableMount.getCapacity());
}
public boolean isReadOnly(String path) throws FileSystemException {
@ -163,6 +165,25 @@ public void delete(String path) throws FileSystemException {
}
}
public void rename(String source, String dest) throws FileSystemException {
if (writableMount == null) throw exceptionOf(source, "Access denied");
source = toLocal(source);
dest = toLocal(dest);
try {
if (!dest.isEmpty()) {
var destParent = FileSystem.getDirectory(dest);
if (!destParent.isEmpty() && !mount.exists(destParent)) writableMount.makeDirectory(destParent);
}
writableMount.rename(source, dest);
} catch (AccessDeniedException e) {
throw new FileSystemException("Access denied");
} catch (IOException e) {
throw localExceptionOf(source, e);
}
}
public WritableByteChannel openForWrite(String path) throws FileSystemException {
if (writableMount == null) throw exceptionOf(path, "Access denied");
@ -223,7 +244,7 @@ private FileSystemException localExceptionOf(@Nullable String localPath, IOExcep
if (e instanceof java.nio.file.FileSystemException ex) {
// This error will contain the absolute path, leaking information about where MC is installed. We drop that,
// just taking the reason. We assume that the error refers to the input path.
var message = ex.getReason().trim();
var message = getReason(ex);
return localPath == null ? new FileSystemException(message) : localExceptionOf(localPath, message);
}
@ -238,4 +259,15 @@ private FileSystemException localExceptionOf(String path, String message) {
private static FileSystemException exceptionOf(String path, String message) {
return new FileSystemException("/" + path + ": " + message);
}
private static String getReason(java.nio.file.FileSystemException e) {
var reason = e.getReason();
if (reason != null) return reason.trim();
if (e instanceof FileAlreadyExistsException) return "File exists";
if (e instanceof NoSuchFileException) return "No such file";
if (e instanceof AccessDeniedException) return "Access denied";
return "Operation failed";
}
}

View File

@ -17,10 +17,12 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.util.Locale;
import java.util.concurrent.TimeUnit;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.closeTo;
import static org.hamcrest.Matchers.lessThanOrEqualTo;
import static org.junit.jupiter.api.Assertions.*;
@Timeout(value = 15)
@ -94,7 +96,9 @@ public void testPauseIfSomeOtherMachine() throws Exception {
assertEquals(budget, TimeUnit.MILLISECONDS.toNanos(25), "Budget should be 25ms");
var delay = ConcurrentHelpers.waitUntil(timeout::isPaused);
assertThat("Paused within 25ms", delay * 1e-9, closeTo(0.025, 0.025));
// Linux appears to have much more accurate timing than Windows/OSX. Or at least on CI!
var time = System.getProperty("os.name", "").toLowerCase(Locale.ROOT).contains("linux") ? 0.05 : 0.3;
assertThat("Paused within a short time", delay * 1e-9, lessThanOrEqualTo(time));
computer.shutdown();
return MachineResult.OK;

View File

@ -10,22 +10,14 @@
import dan200.computercraft.api.filesystem.WritableMount;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Assumptions;
import org.junit.jupiter.api.Test;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.attribute.PosixFileAttributeView;
import java.nio.file.attribute.PosixFilePermission;
import java.util.ArrayList;
import java.util.List;
import java.util.Set;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
public class FileMountTest {
private static final long CAPACITY = 1_000_000;
public class FileMountTest implements WritableMountContract {
private final List<Path> cleanup = new ArrayList<>();
@AfterEach
@ -33,43 +25,42 @@ public void cleanup() throws IOException {
for (var mount : cleanup) MoreFiles.deleteRecursively(mount, RecursiveDeleteOption.ALLOW_INSECURE);
}
private Path createRoot() throws IOException {
@Override
public MountAccess createMount(long capacity) throws IOException {
var path = Files.createTempDirectory("cctweaked-test");
cleanup.add(path);
return path;
return new MountAccessImpl(path.resolve("mount"), capacity);
}
private WritableMount getExisting(long capacity) throws IOException {
return new FileMount(createRoot().toFile(), capacity);
}
private static final class MountAccessImpl implements MountAccess {
private final Path root;
private final long capacity;
private final WritableMount mount;
private WritableMount getNotExisting(long capacity) throws IOException {
return new FileMount(createRoot().resolve("mount").toFile(), capacity);
}
private MountAccessImpl(Path root, long capacity) {
this.root = root;
this.capacity = capacity;
mount = new FileMount(root.toFile(), capacity);
}
@Test
public void testRootWritable() throws IOException {
assertFalse(getExisting(CAPACITY).isReadOnly("/"));
assertFalse(getNotExisting(CAPACITY).isReadOnly("/"));
}
@Override
public WritableMount mount() {
return mount;
}
@Test
public void testMissingDirWritable() throws IOException {
assertFalse(getExisting(CAPACITY).isReadOnly("/foo/bar/baz/qux"));
}
@Override
public void makeReadOnly(String path) {
Assumptions.assumeTrue(root.resolve(path).toFile().setReadOnly(), "Change file to read-only");
}
@Test
public void testDirReadOnly() throws IOException {
var root = createRoot();
var mount = new FileMount(root.toFile(), CAPACITY);
mount.makeDirectory("read-only");
@Override
public void ensuresExist() throws IOException {
Files.createDirectories(root);
}
var attributes = Files.getFileAttributeView(root.resolve("read-only"), PosixFileAttributeView.class);
Assumptions.assumeTrue(attributes != null, "POSIX attributes are not available.");
assertFalse(mount.isReadOnly("read-only"), "Directory should not be read-only yet");
attributes.setPermissions(Set.of(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_EXECUTE));
assertTrue(mount.isReadOnly("read-only"), "Directory should not be read-only yet");
assertTrue(mount.isReadOnly("read-only/child"), "Child should be read-only");
@Override
public long computeRemainingSpace() {
return new FileMount(root.toFile(), capacity).getRemainingSpace();
}
}
}

View File

@ -8,8 +8,8 @@
import com.google.common.io.Files;
import dan200.computercraft.api.filesystem.WritableMount;
import dan200.computercraft.api.lua.LuaException;
import dan200.computercraft.api.lua.ObjectArguments;
import dan200.computercraft.core.TestFiles;
import dan200.computercraft.core.apis.ObjectWrapper;
import dan200.computercraft.core.apis.handles.EncodedWritableHandle;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
@ -45,18 +45,18 @@ public void testWriteTruncates() throws FileSystemException, LuaException, IOExc
{
var writer = fs.openForWrite("out.txt", false, EncodedWritableHandle::openUtf8);
var wrapper = new ObjectWrapper(new EncodedWritableHandle(writer.get(), writer));
wrapper.call("write", "This is a long line");
wrapper.call("close");
var handle = new EncodedWritableHandle(writer.get(), writer);
handle.write(new ObjectArguments("This is a long line"));
handle.doClose();
}
assertEquals("This is a long line", Files.asCharSource(new File(ROOT, "out.txt"), StandardCharsets.UTF_8).read());
{
var writer = fs.openForWrite("out.txt", false, EncodedWritableHandle::openUtf8);
var wrapper = new ObjectWrapper(new EncodedWritableHandle(writer.get(), writer));
wrapper.call("write", "Tiny line");
wrapper.call("close");
var handle = new EncodedWritableHandle(writer.get(), writer);
handle.write(new ObjectArguments("Tiny line"));
handle.doClose();
}
assertEquals("Tiny line", Files.asCharSource(new File(ROOT, "out.txt"), StandardCharsets.UTF_8).read());
@ -69,11 +69,11 @@ public void testUnmountCloses() throws FileSystemException {
fs.mountWritable("disk", "disk", mount);
var writer = fs.openForWrite("disk/out.txt", false, EncodedWritableHandle::openUtf8);
var wrapper = new ObjectWrapper(new EncodedWritableHandle(writer.get(), writer));
var handle = new EncodedWritableHandle(writer.get(), writer);
fs.unmount("disk");
var err = assertThrows(LuaException.class, () -> wrapper.call("write", "Tiny line"));
var err = assertThrows(LuaException.class, () -> handle.write(new ObjectArguments("Tiny line")));
assertEquals("attempt to use a closed file", err.getMessage());
}

View File

@ -0,0 +1,114 @@
/*
* This file is part of ComputerCraft - http://www.computercraft.info
* Copyright Daniel Ratcliffe, 2011-2022. Do not distribute without permission.
* Send enquiries to dratcliffe@gmail.com
*/
package dan200.computercraft.core.filesystem;
import dan200.computercraft.api.filesystem.WritableMount;
import org.junit.jupiter.api.Test;
import org.opentest4j.TestAbortedException;
import java.io.IOException;
import static org.junit.jupiter.api.Assertions.*;
/**
* The contract that all {@link WritableMount}s must fulfill.
*/
public interface WritableMountContract {
long CAPACITY = 1_000_000;
/**
* Create a new empty mount.
*
* @param capacity The capacity of this mount
* @return The newly created {@link WritableMount}.
* @throws IOException If the mount could not be created.
*/
MountAccess createMount(long capacity) throws IOException;
/**
* Create a new empty mount, ensuring it exists on disk.
*
* @param capacity The capacity of this mount
* @return The newly created {@link WritableMount}.
* @throws IOException If the mount could not be created.
*/
default MountAccess createExisting(long capacity) throws IOException {
var mount = createMount(capacity);
mount.ensuresExist();
return mount;
}
@Test
default void testRootWritable() throws IOException {
assertFalse(createExisting(CAPACITY).mount().isReadOnly("/"));
assertFalse(createMount(CAPACITY).mount().isReadOnly("/"));
}
@Test
default void testMissingDirWritable() throws IOException {
assertFalse(createExisting(CAPACITY).mount().isReadOnly("/foo/bar/baz/qux"));
}
@Test
default void testDirReadOnly() throws IOException {
var root = createMount(CAPACITY);
var mount = root.mount();
mount.makeDirectory("read-only");
assertFalse(mount.isReadOnly("read-only"), "Directory should not be read-only yet");
root.makeReadOnly("read-only");
assertTrue(mount.isReadOnly("read-only"), "Directory should not be read-only yet");
assertTrue(mount.isReadOnly("read-only/child"), "Child should be read-only");
}
@Test
default void testMovePreservesSpace() throws IOException {
var access = createExisting(CAPACITY);
var mount = access.mount();
mount.openForWrite("foo").close();
var remainingSpace = mount.getRemainingSpace();
mount.rename("foo", "bar");
assertEquals(remainingSpace, mount.getRemainingSpace(), "Free space has changed after moving");
assertEquals(access.computeRemainingSpace(), access.mount().getRemainingSpace(), "Free space is inconsistent");
}
/**
* Wraps a {@link WritableMount} with additional operations.
*/
interface MountAccess {
/**
* Get the underlying mount.
*
* @return The actual mount.
*/
WritableMount mount();
/**
* Make a path read-only. This may throw a {@link TestAbortedException} if
*
* @param path The mount-relative path.
*/
void makeReadOnly(String path) throws IOException;
/**
* Ensures this mount exists.
*/
void ensuresExist() throws IOException;
/**
* Get the remaining space for this mount.
* <p>
* This should recompute the value where possible, rather than using {@link WritableMount#getRemainingSpace()}:
* its purpose is to ensure the value is accurate!
*
* @return The new
* @throws IOException If the remaining space could not be computed.
*/
long computeRemainingSpace() throws IOException;
}
}

View File

@ -417,6 +417,9 @@ end
--- The stack of "describe"s.
local test_stack = { n = 0 }
--- The stack of setup functions.
local before_each_fns = { n = 0 }
--- Whether we're now running tests, and so cannot run any more.
local tests_locked = false
@ -455,8 +458,14 @@ local function describe(name, body)
local n = test_stack.n + 1
test_stack[n], test_stack.n = name, n
local old_before, new_before = before_each_fns, { n = before_each_fns.n }
for i = 1, old_before.n do new_before[i] = old_before[i] end
before_each_fns = new_before
local ok, err = try(body)
before_each_fns = old_before
-- We count errors as a (failing) test.
if not ok then do_test { error = err, definition = format_loc(debug.getinfo(2, "Sl")) } end
@ -477,7 +486,11 @@ local function it(name, body)
local n = test_stack.n + 1
test_stack[n], test_stack.n, tests_locked = name, n, true
do_test { action = body, definition = format_loc(debug.getinfo(2, "Sl")) }
do_test {
action = body,
before = before_each_fns,
definition = format_loc(debug.getinfo(2, "Sl")),
}
-- Pop the test from the stack
test_stack.n, tests_locked = n - 1, false
@ -498,6 +511,13 @@ local function pending(name)
test_stack.n = n - 1
end
local function before_each(body)
check('it', 1, 'function', body)
local n = before_each_fns.n + 1
before_each_fns[n], before_each_fns.n = body, n
end
local native_co_create, native_loadfile = coroutine.create, loadfile
local line_counts = {}
if cct_test then
@ -568,7 +588,7 @@ do
end
-- When declaring tests, you shouldn't be able to use test methods
set_env { describe = describe, it = it, pending = pending }
set_env { describe = describe, it = it, pending = pending, before_each = before_each }
local suffix = "_spec.lua"
local function run_in(sub_dir)
@ -630,8 +650,13 @@ local function do_run(test)
-- Flush the event queue and ensure we're running with 0 timeout.
os.queueEvent("start_test") os.pullEvent("start_test")
local ok
ok, err = try(test.action)
local ok = true
for i = 1, test.before.n do
if not ok then break end
ok, err = try(test.before[i])
end
if ok then ok, err = try(test.action) end
status = ok and "pass" or (err.fail and "fail" or "error")
pop_state(state)

View File

@ -1,4 +1,8 @@
describe("The fs library", function()
local test_root = "/test-files/fs"
local function test_file(path) return fs.combine(test_root, path) end
before_each(function() fs.delete(test_root) end)
describe("fs.complete", function()
it("validates arguments", function()
fs.complete("", "")
@ -139,7 +143,7 @@ describe("The fs library", function()
end)
it("errors when closing twice", function()
local handle = fs.open("test-files/out.txt", "w")
local handle = fs.open(test_file "out.txt", "w")
handle.close()
expect.error(handle.close):eq("attempt to use a closed file")
end)
@ -216,6 +220,48 @@ describe("The fs library", function()
expect.error(fs.move, "test-files", "rom/move"):eq("Access denied")
expect.error(fs.move, "rom", "test-files"):eq("Access denied")
end)
it("fails if source does not exist", function()
expect.error(fs.move, test_file "src", test_file "dest"):eq("No such file")
end)
it("fails if destination exists", function()
fs.open(test_file "src", "w").close()
fs.open(test_file "dest", "w").close()
expect.error(fs.move, test_file "src", test_file "dest"):eq("File exists")
end)
it("fails to move a directory inside itself", function()
fs.open(test_file "file", "w").close()
expect.error(fs.move, test_root, test_file "child"):eq("Can't move a directory inside itself")
expect.error(fs.move, "", "child"):eq("Can't move a directory inside itself")
end)
it("files can be renamed", function()
fs.open(test_file "src", "w").close()
fs.move(test_file "src", test_file" dest")
expect(fs.exists(test_file "src")):eq(false)
expect(fs.exists(test_file "dest")):eq(true)
end)
it("directories can be renamed", function()
fs.open(test_file "src/some/file", "w").close()
fs.move(test_file "src", test_file" dest")
expect(fs.exists(test_file "src")):eq(false)
expect(fs.exists(test_file "dest")):eq(true)
expect(fs.exists(test_file "dest/some/file")):eq(true)
end)
it("creates directories before renaming", function()
fs.open(test_file "src", "w").close()
fs.move(test_file "src", test_file "dest/file")
expect(fs.exists(test_file "src")):eq(false)
expect(fs.exists(test_file "dest/file")):eq(true)
end)
end)
describe("fs.getCapacity", function()
@ -240,12 +286,11 @@ describe("The fs library", function()
it("returns information about files", function()
local now = os.epoch("utc")
fs.delete("/tmp/basic-file")
local h = fs.open("/tmp/basic-file", "w")
local h = fs.open(test_file "basic-file", "w")
h.write("A reasonably sized string")
h.close()
local attributes = fs.attributes("tmp/basic-file")
local attributes = fs.attributes(test_file "basic-file")
expect(attributes):matches { isDir = false, size = 25, isReadOnly = false }
if attributes.created - now >= 1000 then

View File

@ -29,7 +29,6 @@ public MemoryMount() {
directories.add("");
}
@Override
public void makeDirectory(String path) {
var file = new File(path);
@ -56,6 +55,11 @@ public void delete(String path) {
}
}
@Override
public void rename(String source, String dest) throws IOException {
throw new IOException("Not supported");
}
@Override
public WritableByteChannel openForWrite(final String path) {
return Channels.newChannel(new ByteArrayOutputStream() {
@ -110,6 +114,11 @@ public long getSize(String path) {
throw new RuntimeException("Not implemented");
}
@Override
public long getCapacity() {
return Long.MAX_VALUE;
}
@Override
public SeekableByteChannel openForRead(String path) throws FileOperationException {
var file = files.get(path);