Jetty 12 - Resource resolve() and newResource() return null on resources that do not exist

[joakime]

Cleaned up version of PR #8597

[gregw]

Generally I'm still not convinced that we really want to scatter null checks all over the code base. I also really don't like the implied suggestion that a non null resource does exist. That might be true soon after resolved, but it might not be true for a resource taken out of a collection some time after the resolution.

So I think regardless of returning null or not, I think we need some static methods that will do the null checking:

    interface Resources
    {
        public static boolean exists(Resource r)
        {
            return r != null && r.exists();
        }
        public static boolean isDirectory(Resource r)
        {
            return r != null && r.isDirectory();
        }
    }

This style mimics the statics in Paths and Files (although I don't mind if they are on Resource rather than a Resources interface).

I'm also wondering if perhaps an Optional would be better than a null return. So resolve would always return a non null resource (even if it is a BadResource), but then we'd have a new method:

    public Optional<Resource> resolveExisting(String subPath)
    {
        Resource resource = resolve(subPath);
        if (resource.exists())
            return Optional.of(resource);
        return Optional.empty();
    }

[gregw]

I like the statics... but a few niggles

[gregw]

A directory can be readable, in that it's contents can be listed.

I can see an interpretation of isReadable that means there is content available, but this is a change in semantics that I don't think we need. It is a valid check to ask a directory if it is readable or not. Perhaps this check could be made in the constructor?

[joakime]

Asking "isReadable()" should be different than "isDirectory()".
Meaning if you are isDirectory() == true then you have no need to call isReadable() anyway.

If you look at usage, the isReadable() checks are typically for places where we expect a non-directory that can be accessed and be read (eg: a file)

[gregw]

I know that we used to check some directories for readability. I don't really see the need for changing this semantic. What does a directory that exists but is not actually readable on the file system look like? If all our directory handling code expects directories to not be readable, then we won't be able to distinguish this case?

[gregw]

perhaps rename to hasContent or hasReadableContent if you want your semantic, to distinguish from the Files method that can apply to a directory.

[joakime]

Perhaps it should be canBeOpenForRead() instead.
As it's not meant for "can we access" or "do we have read permissions".