SHRINKWRAP-118: Splitting up ResourceContainer

[alrubinger]

https://jira.jboss.org/jira/browse/SHRINKWRAP-118

This one's a bit more involved than our typical entry-level contributor issues, but we need to start chipping away at pending API changes and work towards getting to a Beta-level.

The issue here is that "ResourceContainer" defines too much; it should work only upon ClassLoader resources.  Currently it overloads the method name "addResource" too heavily.

Nothing too technically involved here, but resolution of this issue will involve:

• Creation of new Container types like "UrlContainer" and "FileContainer"
• Moving methods to the new container types and renaming them accordingly
• Adjusting all JavaDoc (some of which is already outdated)
• Setting the aggregate archive types (ie, JavaArchive, EnterpriseArchive) to extend the new Container types
• Refactoring the tests to match

Let's let German Escobar have a look; we can attach patches for review to the JIRA and discuss them here.  Let me know if this is too ambitious for a first issue.

S,

ALR

[alrubinger]

Ah yes:

ShrinkWrap | Development and Contribution

Above are instructions for getting a dev environment set up.

S,

ALR

[germanescobar]

I've uploaded a patch. I split ResourceContainer in FileContainer and UrlContainer. However, I have some questions regarding this issue:

1. Do we really need an AssetContainer? As far as I understand, everything underneath is an asset, right? Each container has its own methods to add assets directly.

2. Following the previous question, should we create an addXXX(Asset asset, ...) method to FileContainer and UrlContainer like other containers?

3. Are we still going to treat FileContainer and UrlContainer assets as resources? In case we do:

A. Can we call the methods addFileResource and addUrlResource? addUrl(...) sounds odd.

B. Shouldn't we leave ResourceContainer method as they are now? As I see it, you can add a resource using a File, a URL, etc., just like ManifestContainer, EnterpriseContainer, etc. They all overload the addXXX(...) methods heavily too.

[alrubinger]

germanescobar wrote:

I've uploaded a patch. I split ResourceContainer in FileContainer and UrlContainer. However, I have some questions regarding this issue:

Awesome; apologies it's taken me a bit to get back to you.

1. Do we really need an AssetContainer? As far as I understand, everything underneath is an asset, right? Each container has its own methods to add assets directly.

Right, Asset support is defined by "Archive".

2. Following the previous question, should we create an addXXX(Asset asset, ...) method to FileContainer and UrlContainer like other containers?

Nope.

3. Are we still going to treat FileContainer and UrlContainer assets as resources? In case we do:

A. Can we call the methods addFileResource and addUrlResource? addUrl(...) sounds odd.

Sounds like you want to generically apply the word "resource" to mean "some content"?  We have this presently represented by the term "asset", while we've defined "resource" as a "ClassLoader resource".  In your example above, "ResourceContainer" should also become "ClassLoaderResourceContainer" with methods like "addClassLoaderResource" for consistency.  And then that means that every "add*" method would have the word "Resource" appended to the end.

I personally prefer the more succinct "addUrl" style.  In effect you're really adding the content located at the URL's address, but in the context of adding stuff to an archive, isn't that obvious?  Opinions please.

B. Shouldn't we leave ResourceContainer method as they are now? As I see it, you can add a resource using a File, a URL, etc., just like ManifestContainer, EnterpriseContainer, etc. They all overload the addXXX(...) methods heavily too.

You bring up what I call a "design mismatch" which Aslak and I have recently been debating.

Some containers specify input types; the types of things an archive is capable of importing.  For instance, ClassContainer, ResourceContainer, DirectoryContainer.  Some containers are intended to do the opposite; they are in place to define the contract for redirecting output to a known location.

For instance, "addClasses" on "WebArchive" shouldn't be placed relative to the archive root; they go into "WEB-INF/classes".

I consider these two separate concerns.  Container types should define the input contract, and some other thing "Redirector(, or better name)" should intercept some additions and correct the base ArchivePath under which they should be stored, depending upon the archive type being used.

I seek to resolve this mismatch, first by keeping Container types as an input contract, and by adding some "Redirector" thingy under the hood to give the desired function currently working in the testsuites.

Aslak, German, what do you think?

S,

ALR

PS - BTW for your first issue this is turning to be a good debate and your analysis has been spot-on.

[germanescobar]

Sounds like you want to generically apply the word "resource" to mean "some content"?

I was thinking about files that are placed in the classpath but are not classes. Like maven resources.

I consider these two separate concerns.  Container types should define the input contract, and some other thing "Redirector(, or better name)" should intercept some additions and correct the base ArchivePath under which they should be stored, depending upon the archive type being used.

I totally agree! Basically you could add anything to an archive (using Archive.add). However, we are giving some tools to the user to make things easier and "more type safe". So, here is what I think (sorry if I totally missed the point here!!). First, we have a set of known locations:

• ClassPath
• LibraryPath
• WebContentPath
• ModulesPath
• MetaInfPath
• WebInfPath
• etc.

Each location then supports a set of input types:

• File
• Class
• Archive
• ClassLoader resource
• URL
• etc.

For example, ClassPath will have methods like:

// these are usually in all of the known locations
addToClassPath(File) - it could be a directory
addToClassPath(URL)
addToClassPath(String, ClassLoader)
addToClassPath(String) - I think this should be in the form of a URI. We would then redirect to the corresponding method

// these are specific to ClassPath
addClass(Class)
addPackage(Package)

Each archive decides which known locations it supports and it can add some more specific methods if they are not already defined in some known location. For example, addApplicationXml in EarArchive.

AFAIK, Archive currently supports a collection of Assets. However, we would need it to support a collection of paths and assets like in a tree like structure where a node is a path or an asset, in order to implement what I'm saying.

I don't know if this makes sense but I think it gives the user more control over where the assets are being placed in the first place.

PS - If I missed the point here just let me know and and we'll move on ..

[aslak]

If I read you correctly German, this is exactly my point.

The intent of the Container split up was to show that some Archive types does not support a type of objects/paths, ie:

EnterpriseArchive is not a ClassContainer.

The Containers were meant to be a mapping to a Base Path inside the Archive, not where you would fetch things from, ie File / Class loader Resources. In my eyes you can just as well add a File as a 'Resource' inside the Archive as a URL.

I'm still confused about how it is suppose to work if we split up the containers to be where you get things from. I don't see how you don't end up having to specify the full path in that case. Andrew, please explain?

When it comes to ClassContainer I think we should include overloads like addClass(File) addClass(URL) as well, not only Packages/Classes.

And I'm wondering if DirectoryContainer is really a container. A Directory is a Path, any Archive can hold a Path. I think it should be merged with the Archive interface.

[alrubinger]

If the Containers are "a mapping to a base path", then how do you resolve:

1. ClassContainer defines the contract for the mapping
2. JavaArchive is a ClassContainer
3. WebArchive is a ClassContainer
4. Where do Classes go when you call archive.addClass(Class) ?

In JavaArchive, it's the root; in WebArchive it's WEB-INF/classes.  The two then become inconsistent and violate the ClassContainer contract.

What example do you feel I haven't accounted for, where you think the user would need to specify the full path?  Looks like "Containers are entities which hold Assets", and they define the input type.  Where these *go* is a function of the spec Archive type (which is already how it's implemented, BTW.  Look at ContainerBase.addClasses(Class<?> classes...).  It delegates the base location to the impl).

S,

ALR

[aslak]

If you define the Container to be where is comes from.

Take ClassLoaderResource in a WebArchive as an example. How do you specify that you want the Resource to go to:

"/WEB-INF/" or "/WEB-INF/classes" or "/" ?

All valid locations to place something you have fetched from the Classloader.

Where does the URLContainer or FileContainer place the assets?

[germanescobar]

I think we all agree that there are two concepts here that are merged in what is now called a Container. On one hand we have:

• ClassContainer
• LibraryContainer
• EnterpriseContainer
• WebContainer
• ResourceAdapterContainer
• ManifestContainer

On the other hand we have:

• DirectoryContainer
• FileContainer
• UrlContainer
• ResourceContainer - ClassLoader resource

For the first group I agree with Aslak:

The Containers were meant to be a mapping to a Base Path inside the Archive, not where you would fetch things from, ie File / Class loader Resources.

For example, the path for ClassContainer in a JavaArchive is the root. In a WebArchive is WEB-INF/classes. Andrew, why do you say:

The two then become inconsistent and violate the ClassContainer contract

We could say in the ClassContainer contract: The path within the Archive is up to the implementation.

Regarding the second group. I don't think they are "Containers". On the contrary, I think they are the input types that can be handled by the "Containers". What do you think?

[alrubinger]

aslak wrote:

Take ClassLoaderResource in a WebArchive as an example. How do you specify that you want the Resource to go to:

"/WEB-INF/" or "/WEB-INF/classes" or "/" ?

This is exactly why we have a mismatch. 

Note I'm not really proposing anything at the moment, just highlighting the issues to resolve.

S,

ALR

[germanescobar]

@Aslak

I think that what you call "where you get things from" is what Andrew calls input types (I just borrowed the concept). Just to clarify my previous comment.

[alrubinger]

For example, the path for ClassContainer in a JavaArchive is the root. In a WebArchive is WEB-INF/classes. Andrew, why do you say:

The two then become inconsistent and violate the ClassContainer contract

We could say in the ClassContainer contract: The path within the Archive is up to the implementation.

Regarding the second group. I don't think they are "Containers". On the contrary, I think they are the input types that can be handled by the "Containers". What do you think?

I think I need to retype some stuff my a previous post; the new forums have a habit of deleting things.

The gist I've got left out is that the simplest way to resolve the mismatch is to simply stop supporting one construct.  Namely, the one I was highlighting (ResourceContainer JavaDoc should not be the thing we aspire to be, it should be the thing we ditch).

In this proposal above, we say "Containers are entities which store types."  Unlike "Assets", which represent content, "types" are content with context.  A Class is a bunch of bytes meant to go in a target location depending upon the context of the archive (JAR, WAR, etc).  So I think that's consistent with what we're all now talking about.

So to carry on that definition, we say then:

LibraryContainer: "addLibrary(*) methods all put stuff in WEB-INF/lib or equivalent

ClassContainer: addClass(*) > WEB-INF/classes or equivalent

...etc.

So long as all is consistent and documented, we've met the objective.

S,

ALR

[germanescobar]

In this proposal above, we say "Containers are entities which store types."

What if I want to add a messages_en.properties in a WebArchive? They go in the WEB-INF/classes. As Aslak asked before, should I specify the full path?

Wouldn't it be better if we define Containers as a "path within an archive capable of storing assets"? In this case the name "Path" would be more clear than "Container". For example (taken from a comment I posted before):

• ClassPath
• LibraryPath
• WebInfPath
• ModulesPath
• etc.

Each of these paths then supports a set of input types:

• File
• Class (only ClassPath)
• Archive (for example for the ModulesPath)
• ClassLoader resource
• URL
• etc.

Now you could say something like:

WebArchive webArchive = ...;
webArchive.addClass(...)
          .addToClassPath(<a file, url, etc.>)
          .addToWebInfPath(<a custom config file for example>)
          .etc ...

• addClass(...) would be defined in ClassPath (mapped to WEB-INF/classes in a WebArchive)
• addToClassPath(...) would be defined in ClassPath
• addToWebInfPath(...) would be defined in WebInfPath (mapped to WEB-INF in a WebArchive)

[alrubinger]

OK, I've gotta do a 180 here.  I'm seeing Aslak's design intent now.  After some discussion in #jbosstesting on Freenode (hint hint German), we've got:

Action items:

• Move methods in "DirectoryContainer" to "Archive"
• Delete "DirectoryContainer"
• Consider a rename of "*Container" types.  They're not just containers, they define support to add things to specific places.
• Make a Wiki page with a mapping table showing target locations per input type.  eg. addClass in WebArchive goes to "WEB-INF/classes".

For the code portions, this should now be much easier than initially reported.  German was key in helping us get to consensus, so could you edit the SHRINKWRAP-118 description to account for the first two bullets above, then do a patch for it?

S,

ALR

PS - Any confusion in this thread is mine; apologies.  Though we do need to clear up naming and documentation.

[germanescobar]

Ok,

I've already uploaded the patch. Some questions though:

1. I think we need to delete ResourceContainer too. Do you agree? Should I delete it as part of this patch?
2. I think createDirectory is a better name than addDirectory. addDirectory sounds like if you were adding a directory from your machine to the archive.

Think that's all!