From cf3635b42df3a8a0a14c5f12f2b2846155ce4bb1 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Tue, 9 Oct 2018 23:13:11 +0200 Subject: [PATCH] Resource.lastModified() propagates 0 value if target resource exists Includes use of Files.getLastModifiedTime for NIO Paths, preservation of NIO-based resolution on createRelative, deprecation of PathResource, and consistent use of getContentLengthLong over getContentLength. Issue: SPR-17320 --- .../io/AbstractFileResolvingResource.java | 29 +++++-- .../core/io/AbstractResource.java | 9 ++- .../core/io/FileSystemResource.java | 75 ++++++++++++++++--- .../core/io/FileUrlResource.java | 5 +- .../springframework/core/io/PathResource.java | 5 +- .../org/springframework/core/io/Resource.java | 4 +- .../core/io/ResourceTests.java | 48 ++++++++++-- 7 files changed, 139 insertions(+), 36 deletions(-) diff --git a/spring-core/src/main/java/org/springframework/core/io/AbstractFileResolvingResource.java b/spring-core/src/main/java/org/springframework/core/io/AbstractFileResolvingResource.java index c8677e659b..5fae09e9ad 100644 --- a/spring-core/src/main/java/org/springframework/core/io/AbstractFileResolvingResource.java +++ b/spring-core/src/main/java/org/springframework/core/io/AbstractFileResolvingResource.java @@ -65,7 +65,7 @@ public abstract class AbstractFileResolvingResource extends AbstractResource { return false; } } - if (con.getContentLength() >= 0) { + if (con.getContentLengthLong() > 0) { return true; } if (httpCon != null) { @@ -106,7 +106,7 @@ public abstract class AbstractFileResolvingResource extends AbstractResource { return false; } } - int contentLength = con.getContentLength(); + long contentLength = con.getContentLengthLong(); if (contentLength > 0) { return true; } @@ -226,23 +226,35 @@ public abstract class AbstractFileResolvingResource extends AbstractResource { URL url = getURL(); if (ResourceUtils.isFileURL(url)) { // Proceed with file system resolution - return getFile().length(); + File file = getFile(); + long length = file.length(); + if (length == 0L && !file.exists()) { + throw new FileNotFoundException(getDescription() + + " cannot be resolved in the file system for checking its content length"); + } + return length; } else { // Try a URL connection content-length header URLConnection con = url.openConnection(); customizeConnection(con); - return con.getContentLength(); + return con.getContentLengthLong(); } } @Override public long lastModified() throws IOException { URL url = getURL(); + boolean fileCheck = false; if (ResourceUtils.isFileURL(url) || ResourceUtils.isJarURL(url)) { // Proceed with file system resolution + fileCheck = true; try { - return super.lastModified(); + File fileToCheck = getFileForLastModifiedCheck(); + long lastModified = fileToCheck.lastModified(); + if (lastModified > 0L || fileToCheck.exists()) { + return lastModified; + } } catch (FileNotFoundException ex) { // Defensively fall back to URL connection check instead @@ -251,7 +263,12 @@ public abstract class AbstractFileResolvingResource extends AbstractResource { // Try a URL connection last-modified header URLConnection con = url.openConnection(); customizeConnection(con); - return con.getLastModified(); + long lastModified = con.getLastModified(); + if (fileCheck && lastModified == 0 && con.getContentLengthLong() <= 0) { + throw new FileNotFoundException(getDescription() + + " cannot be resolved in the file system for checking its last-modified timestamp"); + } + return lastModified; } /** diff --git a/spring-core/src/main/java/org/springframework/core/io/AbstractResource.java b/spring-core/src/main/java/org/springframework/core/io/AbstractResource.java index 2bb8175cce..9489e48eca 100644 --- a/spring-core/src/main/java/org/springframework/core/io/AbstractResource.java +++ b/spring-core/src/main/java/org/springframework/core/io/AbstractResource.java @@ -146,7 +146,7 @@ public abstract class AbstractResource implements Resource { InputStream is = getInputStream(); try { long size = 0; - byte[] buf = new byte[255]; + byte[] buf = new byte[256]; int read; while ((read = is.read(buf)) != -1) { size += read; @@ -169,10 +169,11 @@ public abstract class AbstractResource implements Resource { */ @Override public long lastModified() throws IOException { - long lastModified = getFileForLastModifiedCheck().lastModified(); - if (lastModified == 0L) { + File fileToCheck = getFileForLastModifiedCheck(); + long lastModified = fileToCheck.lastModified(); + if (lastModified == 0L && !fileToCheck.exists()) { throw new FileNotFoundException(getDescription() + - " cannot be resolved in the file system for resolving its last-modified timestamp"); + " cannot be resolved in the file system for checking its last-modified timestamp"); } return lastModified; } diff --git a/spring-core/src/main/java/org/springframework/core/io/FileSystemResource.java b/spring-core/src/main/java/org/springframework/core/io/FileSystemResource.java index 0a59eaba45..30ba44033d 100644 --- a/spring-core/src/main/java/org/springframework/core/io/FileSystemResource.java +++ b/spring-core/src/main/java/org/springframework/core/io/FileSystemResource.java @@ -26,6 +26,7 @@ import java.net.URL; import java.nio.channels.FileChannel; import java.nio.channels.ReadableByteChannel; import java.nio.channels.WritableByteChannel; +import java.nio.file.FileSystem; import java.nio.file.Files; import java.nio.file.NoSuchFileException; import java.nio.file.Path; @@ -86,10 +87,10 @@ public class FileSystemResource extends AbstractResource implements WritableReso *

Note: When building relative resources via {@link #createRelative}, * the relative path will apply at the same directory level: * e.g. new File("C:/dir1"), relative path "dir2" -> "C:/dir2"! - * If you prefer to have relative paths built underneath the given root - * directory, use the {@link #FileSystemResource(String) constructor with a file path} - * to append a trailing slash to the root path: "C:/dir1/", which - * indicates this directory as root for all relative paths. + * If you prefer to have relative paths built underneath the given root directory, + * use the {@link #FileSystemResource(String) constructor with a file path} + * to append a trailing slash to the root path: "C:/dir1/", which indicates + * this directory as root for all relative paths. * @param file a File handle * @see #FileSystemResource(Path) * @see #getFile() @@ -102,20 +103,38 @@ public class FileSystemResource extends AbstractResource implements WritableReso } /** - * Create a new {@code FileSystemResource} from a {@link Path} handle. + * Create a new {@code FileSystemResource} from a {@link Path} handle, + * performing all file system interactions via NIO.2 instead of {@link File}. *

In contrast to {@link PathResource}, this variant strictly follows the * general {@link FileSystemResource} conventions, in particular in terms of * path cleaning and {@link #createRelative(String)} handling. * @param filePath a Path handle to a file * @since 5.1 * @see #FileSystemResource(File) - * @see PathResource */ public FileSystemResource(Path filePath) { Assert.notNull(filePath, "Path must not be null"); - this.filePath = filePath; - this.file = null; this.path = StringUtils.cleanPath(filePath.toString()); + this.file = null; + this.filePath = filePath; + } + + /** + * Create a new {@code FileSystemResource} from a {@link FileSystem} handle, + * locating the specified path. + *

This is an alternative to {@link #FileSystemResource(String)}, + * performing all file system interactions via NIO.2 instead of {@link File}. + * @param fileSystem the FileSystem to locate the path within + * @param path a file path + * @since 5.1.1 + * @see #FileSystemResource(File) + */ + public FileSystemResource(FileSystem fileSystem, String path) { + Assert.notNull(fileSystem, "FileSystem must not be null"); + Assert.notNull(path, "Path must not be null"); + this.path = StringUtils.cleanPath(path); + this.file = null; + this.filePath = fileSystem.getPath(this.path).normalize(); } @@ -240,11 +259,44 @@ public class FileSystemResource extends AbstractResource implements WritableReso } /** - * This implementation returns the underlying File's length. + * This implementation returns the underlying File/Path length. */ @Override public long contentLength() throws IOException { - return (this.file != null ? this.file.length() : Files.size(this.filePath)); + if (this.file != null) { + long length = this.file.length(); + if (length == 0L && !this.file.exists()) { + throw new FileNotFoundException(getDescription() + + " cannot be resolved in the file system for checking its content length"); + } + return length; + } + else { + try { + return Files.size(this.filePath); + } + catch (NoSuchFileException ex) { + throw new FileNotFoundException(ex.getMessage()); + } + } + } + + /** + * This implementation returns the underlying File/Path last-modified time. + */ + @Override + public long lastModified() throws IOException { + if (this.file != null) { + return super.lastModified(); + } + else { + try { + return Files.getLastModifiedTime(this.filePath).toMillis(); + } + catch (NoSuchFileException ex) { + throw new FileNotFoundException(ex.getMessage()); + } + } } /** @@ -255,7 +307,8 @@ public class FileSystemResource extends AbstractResource implements WritableReso @Override public Resource createRelative(String relativePath) { String pathToUse = StringUtils.applyRelativePath(this.path, relativePath); - return new FileSystemResource(pathToUse); + return (this.file != null ? new FileSystemResource(pathToUse) : + new FileSystemResource(this.filePath.getFileSystem(), pathToUse)); } /** diff --git a/spring-core/src/main/java/org/springframework/core/io/FileUrlResource.java b/spring-core/src/main/java/org/springframework/core/io/FileUrlResource.java index 7c725f9fe3..d0013c8713 100644 --- a/spring-core/src/main/java/org/springframework/core/io/FileUrlResource.java +++ b/spring-core/src/main/java/org/springframework/core/io/FileUrlResource.java @@ -37,9 +37,8 @@ import org.springframework.util.ResourceUtils; *

This is the class resolved by {@link DefaultResourceLoader} for a "file:..." * URL location, allowing a downcast to {@link WritableResource} for it. * - *

Alternatively, for direct construction from a {@link java.io.File} handle, - * consider using {@link FileSystemResource}. For an NIO {@link java.nio.file.Path}, - * consider using {@link PathResource} instead. + *

Alternatively, for direct construction from a {@link java.io.File} handle + * or NIO {@link java.nio.file.Path}, consider using {@link FileSystemResource}. * * @author Juergen Hoeller * @since 5.0.2 diff --git a/spring-core/src/main/java/org/springframework/core/io/PathResource.java b/spring-core/src/main/java/org/springframework/core/io/PathResource.java index 2bedadec6b..61f5f1ebc8 100644 --- a/spring-core/src/main/java/org/springframework/core/io/PathResource.java +++ b/spring-core/src/main/java/org/springframework/core/io/PathResource.java @@ -48,10 +48,11 @@ import org.springframework.util.Assert; * @author Philippe Marschall * @author Juergen Hoeller * @since 4.0 - * @see FileSystemResource#FileSystemResource(Path) * @see java.nio.file.Path * @see java.nio.file.Files + * @deprecated as of 5.1.1, in favor of {@link FileSystemResource#FileSystemResource(Path)} */ +@Deprecated public class PathResource extends AbstractResource implements WritableResource { private final Path path; @@ -105,7 +106,7 @@ public class PathResource extends AbstractResource implements WritableResource { /** * This implementation returns whether the underlying file exists. - * @see org.springframework.core.io.PathResource#exists() + * @see java.nio.file.Files#exists(Path, java.nio.file.LinkOption...) */ @Override public boolean exists() { diff --git a/spring-core/src/main/java/org/springframework/core/io/Resource.java b/spring-core/src/main/java/org/springframework/core/io/Resource.java index 896f4bcbc3..cf3e646af9 100644 --- a/spring-core/src/main/java/org/springframework/core/io/Resource.java +++ b/spring-core/src/main/java/org/springframework/core/io/Resource.java @@ -43,9 +43,9 @@ import org.springframework.lang.Nullable; * @see WritableResource * @see ContextResource * @see UrlResource - * @see ClassPathResource + * @see FileUrlResource * @see FileSystemResource - * @see PathResource + * @see ClassPathResource * @see ByteArrayResource * @see InputStreamResource */ diff --git a/spring-core/src/test/java/org/springframework/core/io/ResourceTests.java b/spring-core/src/test/java/org/springframework/core/io/ResourceTests.java index 3a019821fe..3e9ee663b9 100644 --- a/spring-core/src/test/java/org/springframework/core/io/ResourceTests.java +++ b/spring-core/src/test/java/org/springframework/core/io/ResourceTests.java @@ -123,11 +123,10 @@ public class ResourceTests { @Test public void testFileSystemResource() throws IOException { - Resource resource = new FileSystemResource(getClass().getResource("Resource.class").getFile()); + String file = getClass().getResource("Resource.class").getFile(); + Resource resource = new FileSystemResource(file); doTestResource(resource); - assertEquals(new FileSystemResource(getClass().getResource("Resource.class").getFile()), resource); - Resource resource2 = new FileSystemResource("core/io/Resource.class"); - assertEquals(resource2, new FileSystemResource("core/../core/io/./Resource.class")); + assertEquals(new FileSystemResource(file), resource); } @Test @@ -136,8 +135,12 @@ public class ResourceTests { Resource resource = new FileSystemResource(filePath); doTestResource(resource); assertEquals(new FileSystemResource(filePath), resource); - Resource resource2 = new FileSystemResource("core/io/Resource.class"); - assertEquals(resource2, new FileSystemResource("core/../core/io/./Resource.class")); + } + + @Test + public void testFileSystemResourceWithPlainPath() { + Resource resource = new FileSystemResource("core/io/Resource.class"); + assertEquals(resource, new FileSystemResource("core/../core/io/./Resource.class")); } @Test @@ -157,23 +160,52 @@ public class ResourceTests { private void doTestResource(Resource resource) throws IOException { assertEquals("Resource.class", resource.getFilename()); assertTrue(resource.getURL().getFile().endsWith("Resource.class")); + assertTrue(resource.exists()); + assertTrue(resource.isReadable()); + assertTrue(resource.contentLength() > 0); + assertTrue(resource.lastModified() > 0); Resource relative1 = resource.createRelative("ClassPathResource.class"); assertEquals("ClassPathResource.class", relative1.getFilename()); assertTrue(relative1.getURL().getFile().endsWith("ClassPathResource.class")); assertTrue(relative1.exists()); + assertTrue(relative1.isReadable()); + assertTrue(relative1.contentLength() > 0); + assertTrue(relative1.lastModified() > 0); Resource relative2 = resource.createRelative("support/ResourcePatternResolver.class"); assertEquals("ResourcePatternResolver.class", relative2.getFilename()); assertTrue(relative2.getURL().getFile().endsWith("ResourcePatternResolver.class")); assertTrue(relative2.exists()); + assertTrue(relative2.isReadable()); + assertTrue(relative2.contentLength() > 0); + assertTrue(relative2.lastModified() > 0); - /* Resource relative3 = resource.createRelative("../SpringVersion.class"); assertEquals("SpringVersion.class", relative3.getFilename()); assertTrue(relative3.getURL().getFile().endsWith("SpringVersion.class")); assertTrue(relative3.exists()); - */ + assertTrue(relative3.isReadable()); + assertTrue(relative3.contentLength() > 0); + assertTrue(relative3.lastModified() > 0); + + Resource relative4 = resource.createRelative("X.class"); + assertFalse(relative4.exists()); + assertFalse(relative4.isReadable()); + try { + relative4.contentLength(); + fail("Should have thrown FileNotFoundException"); + } + catch (FileNotFoundException ex) { + // expected + } + try { + relative4.lastModified(); + fail("Should have thrown FileNotFoundException"); + } + catch (FileNotFoundException ex) { + // expected + } } @Test