[MAGNOLIA-6326] Don't fail when a resource exists as a folder in an origin and a file in another Created: 28/Jul/15  Updated: 07/Aug/15  Resolved: 04/Aug/15

Status: Closed
Project: Magnolia
Component/s: resource-loader
Affects Version/s: None
Fix Version/s: 5.4.1

Type: Improvement Priority: Critical
Reporter: Magnolia International Assignee: Jaroslav Simak
Resolution: Fixed Votes: 0
Labels: backlog541, support
Remaining Estimate: 0d
Time Spent: 0.25d
Original Estimate: Not Specified

Issue Links:
causality
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Release notes required:
Yes
Date of First Response:
Epic Link: Config by file / code
Sprint: Sprint 4 (Kromeriz)
Story Points: 5

 Description   

Unfortunately, the following situation happens quite often: java.lang.IllegalStateException: Given resources are not all directory/file: [JcrResource{path=/foo/bar}, ClasspathResource{path=/foo/bar}]

This is happening in info.magnolia.resourceloader.layered.LayeredResourceOrigin#newLayeredResource. We originally thought this would be a really unlikely and extreme case, so we just added a protective if block. It looks like this is happening to a few folks, especially with e.g themes, where for some reason we still strip the extension when importing nodes into JCR.

TODO:

  • improve error message (which of these resources is a folder, which one is a file)
  • log it, ignore not-matching resources (e.g simply not add them into the LayeredResource)
  • fix e.g ThemeVersionHandler so it does NOT strip extensions (and check for other uses of InstallBinaryResourceTask and InstallTextResourceTask
  • generally discourage the use of InstallBinaryResourceTask and InstallTextResourceTask since they are not all that vital anymore (since resources DON'T have to be copied to JCR to be served anymore, they can be served straight from filesystem or classpath since 5.4)

Workarounds:

  • rename resources such that there is no filename-without-extension that conflicts with folders
  • don't use ThemeVersionHandler, or use a patched version of it (which sets stripExtension to false)
  • don't InstallBinaryResourceTask or InstallTextResourceTask, or make sure stripExtension is false)


 Comments   
Comment by Magnolia International [ 06/Aug/15 ]

Committed a few minor changes – have a look, take or reject.

  • IMO clearer indication in toString if resource is a file or a directory. Also added origin name in there (the classname of the resource isn't enough, e.g with classpath and legacy-classpath. Ideally, we could print out the complete url/path or whatever (e.g file://foo/bar.jar:foo/file.txt for a classpath resource), but we don't have that info for all resources (i.e for ClasspathResource we currently don't keep the url, just the path)
  • Instead of introducing an extra variable to workaround the fact that 'resources' was final in newLayeredResource(), simply make it non-final (debatable)
  • Be careful with Guava collections: they're mostly "views" on the given collection - in most cases, you'll want to copy that. Here you were doing it (new ArrayList()), but for all cases. I changed that (only do copy if needed), and use Lists.newArrayList() to be consistent.

In tests:

  • no need to check for collection size if you also use Matchers.contains(), which also ensures the size is as expected

I was on the fence about the filtering – since we already apply the function once on the collection anyway (with the if (all()) statement), we might as well just filter right away. (We don't because we want to log.warn, although we could do that in predicate). OTOH, it's probably best to avoid filter/copy of the collection for 99% of the cases, and leave the double-execution of the predicate for the very rare cases that this issue fixes.

Otherwise

Generated at Mon Feb 12 04:13:27 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.