JBossDeveloper

aslak wrote:

 

Formatter is API while Node is SPI, so strictly speaking it shouldn't know about it.

 

The Formatters can use Archive.getContent(), as they do today?

Well, they can. But we will have the same problems we have today. Take a look at FullFormatter or JdkZipExporterDelegate. It is not a problem of the implementations, is a problem with the API. Sometimes you want plain access to the archive contents, but other times you need transversal access. We should provide both to the clients.

What we can do here is leave the API as it is now and let time decide. It would be pointless to add the NodeArchive implementation if no one can benefit from it. I would like to upload the code somewhere just for future reference though. But I really think is a problem that should be address from the API itself. 

I've have promoted Node to API with the following def:

public interface Node
{
   /**
    * @return The {@link Asset} this node holds, null if it is a directory
    */
   Asset getAsset();

   /**
    * @return The child nodes of this node or, an empty set if it has no 
    *           children or holds an asset. This method will never return null.
    */
   Set<Node> getChildren();

   /**
    * @return The path where this node is placed within the {@link Archive}
    */
   ArchivePath getPath();

}

I've also changed the following 4 methods of the Archive interface so they return nodes:

Node get(ArchivePath path) throws IllegalArgumentException;

Node get(String path) throws IllegalArgumentException;

Map<ArchivePath, Node> getContent();

Map<ArchivePath, Node> getContent(Filter<ArchivePath> filter);

I changed implementations and tests accordingly. I had to change some tests because they expected one node but now every directory is a node so you should expect more entries. All tests pass except:

• ZipExporterTestCase.testExportThrowsArchiveExceptionOnAssetWriteFailure() that hangs and haven´t figure out why (some deadlock scenario). It is commented.
• The formatter tests. I haven´t fixed them accordingly. They also expect less entries. They are commented.
• The Glassfish extension tests.

I fixed the MemoryMapArchiveBase but I haven´t added NodeArchiveBase there yet. I also fixed JdkZipExporterDelegate and FullFormatter which are much more cleaner now.

Implementation details

When an Archive is created, it adds a Node with path "/". You can then get the root node using get("/") or get(""). So, now you can either retrieve the contents in a flat presentation with Archive.getContent() method or in a full or partial tree presentation with get("...").

The convention I'm using is that every entry starts with "/" and ends without a trailing "/". On export, the preceding "/" is removed.

I think that's it. I know we talked about not changing the Archive API but I just want to show you this code so you can tell me what you think. In my opinion, it is much more cleaner and flexible.I'm attaching the patch here until we decide whether we want to make this change or not.

German.

Don't worry, get better first!

Besides, I've continued to work on this and I'm almost done. All the tests pass and it throws an Exception if an Illegal Path is introduced.

I've also fixed SHRINKWRAP-133 (the ZIP deadlock). I just add a dummy entry if two conditions are satisfied:

1. An exception was thrown.
2. The ZIP file is empty.

I don´t know if it is too hacky.

I'll upload the path again soon with everything.

Ok, attached is my final proposal to this subject. I really think it's the right path to follow.

List of changes

1. A new Node interface (see previous posts).
2. Changes to the API and Impl to support the new Node interface.
3. DirectoryAsset was deleted.
4. Changes to the ZipImporter implementation classes to take advantage of the new transversal access to the archive.
5. Changes to the FullFormatter to take advantage of the new transversal access to the archive.
6. Minor changes to the tests to support the Node interface. They all pass now.
7. Changes to the glassfish extension to support the Node interface. Tests passing as well.
8. Propossed solution to SHRINKWRAP-133.
9. A new exception IllegalArchivePathException was created and it's thrown on an invalid path access. We are still discussing the name. More information: Illegal path

Benefits of the solution

You now have transversal access to the archive. The Archive.get("/") method will return the root Node. Then you can recursively call Node.getChildren() to navigate the whole tree. Plain access is still supported using the Archive.getContent() method. If the solution is accepted, we could introduce a new Archive.getRootNode() instead of the Archive.get("/") to obtain the root Node.

You can also have partial access to the tree with Archive.get("/somepath...") which will return a Node.

This problem is solved: Access to directories

TODO

1. When using Archive.addDirectories(String ... paths), it may throw an IllegalArchivePathException and some directories might not be added.
2. We need to remove either VerboseFormatter or FullFormatter. They both do the same. Or make a decision about what each should show.

Should I be committing; I can't remember if you have access?

No, I don't have.

I think "VerboseFormatter" is a better name, and probably should keep the "FullFormatter" mechanics.

Ok, I'll change that.

String literal "/" in FullFormatter and VerboseFormatter could be a staticy finaly thing.

Ok, I'll also change that.

The author email address has a stray "c" at the end?

Yes, my GMail address is german.escobarc@gmail.com. The one without the c was already taken.