Created new liquibase.resource.Resource interface

[nvoxland]

Impact

• Bug fix (non-breaking change which fixes expected existing functionality)
• Enhancement/New feature (adds functionality without impacting existing logic)
• Breaking change (fix or feature that would cause existing functionality to change)

Description

The current ResourceAccessor / PathHandler interfaces are purely around reading files and therefore only expose "input stream" implementations. But, there are times we want to be able to write to resources as well.

The files to write to can be things that are "absolutely" referenced, like the liquibase.properties file given an absolute path as well as existing files found in the search path.

This is introduces write support by adding a new liquibase.resource.Resource object which can be returned by ResourceAccessors and PathHandlers.

The new Resource objects wrap a specific resource that has been found, and have openInputStream and openOutputStream methods on them.

API Changes

Resource

The new Resource interface is kept purposefully simple. And as a new interface it doesn't impact anything existing.

It has a getPath() call to get the ResourceAccessor-valid path as well as a getURI() to return an "absolute" description of the file. The getURI() should be used to determine equality since multiple Resources can have the same path within a ResourceAccessor.

Beyond those description calls, there is openInputStream/openOutputStream for reading/writing as well as an exists() and isWritable() calls. I thought about not including the exists and isWritable in favor of just throwing exceptions from openInputStream and openOutputStream, but I could imagine enough times when we wanted to know that without necessarily starting to read from the file so added them. I didn't include an isReadable figuring exists is close enough that the extra thing to implement was not useful enough.

I looked at org.springframework.core.io.Resource for a lot of the inspiration for this class but kept it simpler than that interface. For example, I didn't include isOpen, isReadable, isFile, contentLength, lastModified and createRelative under the theory that we shouldn't need those and so didn't want to put it on implementations to deal with them.

I created implementations of Resource we need so far: PathResource which works for files and also things within jar files. Also URIResource and SpringResource.

We really need a MavenResource as well because that will allow us to have openInputStream return a stream from target/classes and openOutputStream return a stream from src/main/resources (or maybe a combined 'write to both' one). Being able to support "split" places to read/write like that is part of the design goal behind Resource, but I didn't get that yet.

ResourceAccessor

The changes to ResourceAccessor shouldn't impact anyone calling resource accessors, but it will be an api breaking change for anyone implementing their own ResourceAccessors. This is something we'll need to include in the release notes, but this should be a limited enough set of people that I think it will be OK. I looked around a bit on github, and most of the uses I found are in tests. The ResourceAccessors we ship cover most of the ways people want to use them. are The alternative to being a breaking change is difficult and imposing complexity on everyone using the APIs so this seemed far better to me.

With the new Resource concept, the existing ResourceAccessor.openStream and ResourceAccessor.openStream methods are no longer needed and have been marked as deprecated. Also the ResourceAccessor.list() method which just returns string paths which have to be re-looked-up via openStream(s) so that method is also now listed as deprecated. These methods have been re-implemented, but should work about the same which is why anyone calling these methods should not be impacted. The only expected behavior change is that search can no longer include directories in the returned set, but that seems like an odd use case nobody should have to need. Our code wasn't using that feature and it's likely it didn't even work before (which is why I took it out)

In place of those deprecated methods, we now have ResourceAccessor.search(String path, boolean recursive) and ResourceAccessor.getAll(String path). Both those now only take a simple path argument vs relativeTo and path and instead there is a separate ResourceAccesor.resolve(String, String)

Compared to list, search has been simplified by no longer making it have to figure out the base directory. The javadoc is also more explicit that the path needs to be a "base directory" vs. a file. It's also been simplified to not have to worry about handling files vs. directories, particuarly because we found that some ResourceAccessors (especially ClassLoaderResouceAccessor) doesn't really have a concept of "directories" so I took that out.

Compared to search, the new getAll() method is what you use to get a specific file. But, multiple files can be in the same path for some resource accessors so it's getAll(). There is a convencience get() method on the interface which calls getAll and fails with a standard exception if there is more than one. There is also a convenience getExisting() method on the interface which calls get() and throws an exception if the resource does not exist. get vs getAll is the new version of openStream vs. openStreams.

Both search and getAll return Lists vs. SortedSets like the old methods do, because we actually want the ResourceAccessor to be in control of the order things come back in, not a Comparator. For example, ClassLoaderResourceAccessor should return files in the order they were in the classloader, not alphabetical order. Our new "duplicate file mode" feature makes this change needed.

I did the new convenience methods and re-implementations of deprecated methods as default methods on the interface vs. in AbstractResourceAccessor. That seems like the better place to put them to be more global and so AbstractResourceAccessor is pretty boring now. But thought it was good to keep that abstract class in there still for API compatibility and future extensibitlity.

So you see how the breaking change we're stuck with is that ResourceAccessor implementations need to implement the new getAll and search methods. Introducing a new interface instead would keep those from breaking compliation, but we'd need to scatter "if resourceAccessor instanceof NewResourceAccessor" checks throughout the code and really those other implementations would be missing functionality without still implementing that new interface so they're really still broken. So, I just went all-in and added them as new methods and then re-implemented our existing ResourceAccessors to use those new methods and re-implemented the deprecated methods as calls to the new methods.

PathHandler

The PathHandler interface is like ResourceAccessor in that it had a way to get an inputStream and really should be returning a Resource instead. Since this is a new API I also broke that one by replacing the old method with a new one and in that case didn't even bother keeping a deprecated version since it's so new.

Updated Code

To keep away the "uses deprecated methods" warning and to ensure the new methods are usable by developers, I fixed all our existing code to use the new search/get methods vs. the old deprecated ones. The new and existing tests keep failing on various things both locally and on the build server which helped me to work through cases not yet well enough handled on the new code. Once they are passing, I'll be fairly confident that the changes don't break anything, but it is enough moving around of existing logic that it is possible that non-tested edge cases and further-afield stuff like odd spring environmental setups may find bugs that will have to be fixed quickly in future releases.

Things to be aware of

• Currently a breaking API change for ResourceAccessor implementations due to the additional method to implement, but not for anyone using a ResourceAccessor
• Currently a breaking API change for PathHandler due to a replacing of a method

Things to worry about

• Large refactoring of the ResourceAccessor layer, and there may be use cases we're not testing

[github-actions]

Unit Test Results

  1 408 files   -   3 248    1 408 suites   - 3 248   4m 29s ⏱️ - 39m 20s
  4 608 tests  -        20    4 240 ✔️  -      173  364 💤 +   149    4 ❌ +  4 
15 288 runs   - 39 420  14 878 ✔️  - 34 810  398 💤  - 4 622  12 ❌ +12 

For more details on these failures, see this check.

Results for commit 897a9e4. ± Comparison against base commit 031928b.

♻️ This comment has been updated with latest results.

[nvoxland]

Pushing up this first commit to start the discussion...

The changes so far are API breaking as I'm looking at how it would plug together "ideally". Depending on how it ends up, we can either refactor the changes to be less ideal but API compatible, OR leave it as a breaking API change for custom ResourceAccessor implementations under an assumption that there isn't a ton of those and it can be explained. Which route to go is TBD based on discussion.

I added a new List<Resource> find(String relativeTo, String path, boolean recursive, boolean includeFiles, boolean includeDirectories) throws IOException method to the ResourceAccessor API.

The returned Resource has open methods for input and output streams. Different ResourceAccessors can return different implementations, and I created FileResource and ZipEntryResource as examples (along with a temporary UnknownResource placeholder). I looked at the spring "Resource" and java File and Path classes as inspiration. The spring Resource has separate Resource vs. WritableResource interfaces, as well as methods like getContentLength() and exists(). So far I was feeling like we won't ever need those to want to create separate interfaces. Even exists() I left off figuring that the Resource objects only come from the "find" methods which should not return a Resource if it doesn't exist.

I ended making the existing list() method just call out to the new find() method and updated the AbstractResourceAccessor to work that way. That is a breaking use of AbstractResourceAccessor so could be changed around if we'd like. But, I think that openStream and openStreams can also just be implemented by calls to the new find() method and using the open methods on the Resources returned by find. I haven't tried to re-implement those yet, but should be straightfoward? If we don't want to break AbstractResourceAccessor, I could make a new "BetterAbstractResourceAccessor" or whatever that works the new way and adapt the old AbstractResourceAccessor differently to better preserve API compatibility. Keeping it simple for now to start getting feedback.

ResourceAccessor implementations can return whatever Resource implementations they want. For example, the MavenResourceAccessor should return MavenResource objects where the getInputStream() returns the content from target/classes and the getOutputStream() write to src/main/resources

I thought about adding arguments to getOutputStream() to control things like appending vs. overwriting and lock modes, like Files.newOutputStream(Path path, OpenOption... options) but think that those options are too file specific to have in the generic API. Better to add methods to Resource to configure it if we ever need that, like setOutputMode() or whatever. We can do that without being a big change down the road.

Usage of the changes would be:

• Code that wants to write to a particular resource would need to call resourceAccessor.find(....) to get the resource. They can read from that as they want and/or open an output stream to it
• The existing openStream(s) methods would be marked as deprecated in favor of find().openInputStream()
  • Maybe 40 calls to these methods, so good to update our code while we are at it

[rdhzl]

Quick question, if I may - I've been leveraging FileSystemResourceAccessor like so:

Liquibase liquibase = new liquibase.Liquibase("db.changelog-master.xml", new FileSystemResourceAccessor(migrationsDir), database);

What's the correct replacement this accessor for this now that the class has been deleted? Thanks for your assistance!

[StevenMassaro]

Quick question, if I may - I've been leveraging FileSystemResourceAccessor like so:

Liquibase liquibase = new liquibase.Liquibase("db.changelog-master.xml", new FileSystemResourceAccessor(migrationsDir), database);

What's the correct replacement this accessor for this now that the class has been deleted? Thanks for your assistance!

@rdhzl You should be able to use the DirectoryResourceAccessor like this:

Liquibase liquibase = new liquibase.Liquibase("db.changelog-master.xml", new DirectoryResourceAccessor(migrationsDir), database);