SHRINKWRAP-289 - Recursive verbose formatter #32

Open
wants to merge 6 commits into
from

Conversation

Projects
None yet
3 participants
@gpoul

gpoul commented Jul 16, 2011

Tested this changeset with ShinkWrap 1.0.0-beta4.

I'm sure it will require additional changes, but let me know what you think.

@ALRubinger

This comment has been minimized.

Show comment
Hide comment
@ALRubinger

ALRubinger Jul 16, 2011

Why is this isEmpty() check necessary?

Why is this isEmpty() check necessary?

This comment has been minimized.

Show comment
Hide comment
@gpoul

gpoul Jul 16, 2011

Owner

there seems to be an edge case in the JVM-provided ZipOutputStream implementation that causes the underlying JdkZipExporterDelegate to bail on zero-length content archives.

https://github.com/gpoul/shrinkwrap/blob/master/impl-base/src/main/java/org/jboss/shrinkwrap/impl/base/exporter/zip/JdkZipExporterDelegate.java#L71

Owner

gpoul replied Jul 16, 2011

there seems to be an edge case in the JVM-provided ZipOutputStream implementation that causes the underlying JdkZipExporterDelegate to bail on zero-length content archives.

https://github.com/gpoul/shrinkwrap/blob/master/impl-base/src/main/java/org/jboss/shrinkwrap/impl/base/exporter/zip/JdkZipExporterDelegate.java#L71

This comment has been minimized.

Show comment
Hide comment
@ALRubinger

ALRubinger Jul 16, 2011

Ah, yes. I've had fun with that before; glad I noted it as such. My memory is just about the worst. :)

Ah, yes. I've had fun with that before; glad I noted it as such. My memory is just about the worst. :)

This comment has been minimized.

Show comment
Hide comment
@ALRubinger

ALRubinger Jul 16, 2011

Even still, I think we have a gap here. Nowhere in the API docs does it note that "null" is an acceptable return value:

http://docs.jboss.org/shrinkwrap/1.0.0-beta-4/org/jboss/shrinkwrap/api/asset/Asset.html

How about instead: new ByteArrayInputStream(new byte[]{}); ?

Even still, I think we have a gap here. Nowhere in the API docs does it note that "null" is an acceptable return value:

http://docs.jboss.org/shrinkwrap/1.0.0-beta-4/org/jboss/shrinkwrap/api/asset/Asset.html

How about instead: new ByteArrayInputStream(new byte[]{}); ?

This comment has been minimized.

Show comment
Hide comment
@ALRubinger

ALRubinger Jul 16, 2011

Or even better...whatever content represents a valid this.exporter will no content? We could get around the JDK ZIP limitation by using another tool to generate an empty ZIP archive, putting that in src/main/resources, and return a new InputStream with that as the byte content.

Or even better...whatever content represents a valid this.exporter will no content? We could get around the JDK ZIP limitation by using another tool to generate an empty ZIP archive, putting that in src/main/resources, and return a new InputStream with that as the byte content.

This comment has been minimized.

Show comment
Hide comment
@gpoul

gpoul Jul 16, 2011

Owner

I was thinking that "If this returns null, this denotes that the Asset is to be viewed as a logical path (placeholder/directory) only with no backing content." would cover this case just fine... (?)

Owner

gpoul replied Jul 16, 2011

I was thinking that "If this returns null, this denotes that the Asset is to be viewed as a logical path (placeholder/directory) only with no backing content." would cover this case just fine... (?)

This comment has been minimized.

Show comment
Hide comment
@ALRubinger

ALRubinger Jul 16, 2011

Hehe, but this isn't a logical path. It's an empty archive. Directories in ShrinkWrap are denoted by Node with getAsset==null. http://docs.jboss.org/shrinkwrap/1.0.0-beta-4/org/jboss/shrinkwrap/api/Node.html#getAsset()

Hehe, but this isn't a logical path. It's an empty archive. Directories in ShrinkWrap are denoted by Node with getAsset==null. http://docs.jboss.org/shrinkwrap/1.0.0-beta-4/org/jboss/shrinkwrap/api/Node.html#getAsset()

This comment has been minimized.

Show comment
Hide comment
Owner

gpoul replied Jul 16, 2011

@ALRubinger

This comment has been minimized.

Show comment
Hide comment
@ALRubinger

ALRubinger Jul 16, 2011

I suspect this filename pattern matching isn't sufficient/fully-accurate as a check to figure out if this is a sub-archive. What about instead getting the Asset contents for the lcNodePath and doing type checking for (pathTarget instanceof ArchiveAsset) instead?

I suspect this filename pattern matching isn't sufficient/fully-accurate as a check to figure out if this is a sub-archive. What about instead getting the Asset contents for the lcNodePath and doing type checking for (pathTarget instanceof ArchiveAsset) instead?

This comment has been minimized.

Show comment
Hide comment

This comment has been minimized.

Show comment
Hide comment
@ALRubinger

ALRubinger Jul 16, 2011

To Aslak's point: Yes. This is a new addition to the SW API.

To Aslak's point: Yes. This is a new addition to the SW API.

This comment has been minimized.

Show comment
Hide comment
@gpoul

gpoul Jul 16, 2011

Owner

The ArchiveAsset check won't really work given that some of those subarchives are of type ByteArrayAsset. :-/

Owner

gpoul replied Jul 16, 2011

The ArchiveAsset check won't really work given that some of those subarchives are of type ByteArrayAsset. :-/

This comment has been minimized.

Show comment
Hide comment
@gpoul

gpoul Jul 17, 2011

Owner

I thought some more about this and short of basically detecting in the ByteArrayAsset whether this is a sub-archive I'm not sure how to do this. Trying every file to see whether it is an archive just doesn't feel right.

What do you think about combining a list of extensions and a check for ArchiveAsset? Does that make any sense?

Owner

gpoul replied Jul 17, 2011

I thought some more about this and short of basically detecting in the ByteArrayAsset whether this is a sub-archive I'm not sure how to do this. Trying every file to see whether it is an archive just doesn't feel right.

What do you think about combining a list of extensions and a check for ArchiveAsset? Does that make any sense?

This comment has been minimized.

Show comment
Hide comment
@ALRubinger

ALRubinger Jul 18, 2011

Recommend that we don't bother inspecting ByteArrayAssets for content. They were just added as data, thus not "mounted" and unknown. IMO we should avoid guessing or lengthy import detection checks. So I'd b happy with just archive.getAsType(GenericArchive.class, node.getPath()) or an instanceof ArchiveAsset check.

Recommend that we don't bother inspecting ByteArrayAssets for content. They were just added as data, thus not "mounted" and unknown. IMO we should avoid guessing or lengthy import detection checks. So I'd b happy with just archive.getAsType(GenericArchive.class, node.getPath()) or an instanceof ArchiveAsset check.

This comment has been minimized.

Show comment
Hide comment
@gpoul

gpoul Jul 18, 2011

Owner

I'm not sure what you mean with the archive.getAsType(...) as that would basically mean "trying" to get the underlying path as an archive on every path, right?

What do you think about using a list of extensions combined with a check for instanceof ArchiveAsset? (if that's not what you meant with your last comment)

Owner

gpoul replied Jul 18, 2011

I'm not sure what you mean with the archive.getAsType(...) as that would basically mean "trying" to get the underlying path as an archive on every path, right?

What do you think about using a list of extensions combined with a check for instanceof ArchiveAsset? (if that's not what you meant with your last comment)

This comment has been minimized.

Show comment
Hide comment
@aslakknutsen

aslakknutsen Jul 19, 2011

getAsType handled the Import part under the hood, based on the default configured importer for that type of Archive, GenericArchive is Zip i assume.. but you're right, you'll still need a extension check to avoid trying everything..

and if the goal is only to print things that are already nested Archives, then that won't really help.

if(node.getAsset() instanceOf ArchiveAsset)
{
GenericArchive nested = archive.getAsType(GenericArchive.class, path)
}

getAsType handled the Import part under the hood, based on the default configured importer for that type of Archive, GenericArchive is Zip i assume.. but you're right, you'll still need a extension check to avoid trying everything..

and if the goal is only to print things that are already nested Archives, then that won't really help.

if(node.getAsset() instanceOf ArchiveAsset)
{
GenericArchive nested = archive.getAsType(GenericArchive.class, path)
}

This comment has been minimized.

Show comment
Hide comment
@gpoul

gpoul Jul 19, 2011

Owner

So, would this make sense to you?

         if (nodeAsset.getClass().getName().equals("org.jboss.shrinkwrap.impl.base.asset.ArchiveAsset") || 
               lcNodePath.endsWith(".jar") ||
               lcNodePath.endsWith(".war") ||
               lcNodePath.endsWith(".rar") ||
               lcNodePath.endsWith(".sar")) {
            InputStream nodeInputStream = nodeAsset.openStream();
            // If a valid InputStream is returned, list its contents
            if (nodeInputStream != null) {
               GenericArchive nodeArchive = ShrinkWrap.create(GenericArchive.class).as(ZipImporter.class).importFrom(nodeInputStream).as(GenericArchive.class);
               format(sb, nodeArchive.get(ROOT), subArchiveContext + nodePath);
               // remove the last newline
               sb.deleteCharAt(sb.length() - 1);
               // InputStream is not closed on purpose, as that might fail a subsequent export
            } else {
               // If there is no valid InputStream, only output the path
               sb.append(subArchiveContext);
               sb.append(nodePath);
            }
         } else {
            // If this is not a sub-archive, print the node path
            sb.append(subArchiveContext);
            sb.append(nodePath);
         }

Notice that ArchiveAsset is in impl-base and this code is in shrinkwrap-api, so the circular dependency checker hated me when I tried to resolve the dependency for ArchiveAsset, so this is the best I could do for now to illustrate what I'd do, although it doesn't look right.

Owner

gpoul replied Jul 19, 2011

So, would this make sense to you?

         if (nodeAsset.getClass().getName().equals("org.jboss.shrinkwrap.impl.base.asset.ArchiveAsset") || 
               lcNodePath.endsWith(".jar") ||
               lcNodePath.endsWith(".war") ||
               lcNodePath.endsWith(".rar") ||
               lcNodePath.endsWith(".sar")) {
            InputStream nodeInputStream = nodeAsset.openStream();
            // If a valid InputStream is returned, list its contents
            if (nodeInputStream != null) {
               GenericArchive nodeArchive = ShrinkWrap.create(GenericArchive.class).as(ZipImporter.class).importFrom(nodeInputStream).as(GenericArchive.class);
               format(sb, nodeArchive.get(ROOT), subArchiveContext + nodePath);
               // remove the last newline
               sb.deleteCharAt(sb.length() - 1);
               // InputStream is not closed on purpose, as that might fail a subsequent export
            } else {
               // If there is no valid InputStream, only output the path
               sb.append(subArchiveContext);
               sb.append(nodePath);
            }
         } else {
            // If this is not a sub-archive, print the node path
            sb.append(subArchiveContext);
            sb.append(nodePath);
         }

Notice that ArchiveAsset is in impl-base and this code is in shrinkwrap-api, so the circular dependency checker hated me when I tried to resolve the dependency for ArchiveAsset, so this is the best I could do for now to illustrate what I'd do, although it doesn't look right.

@ALRubinger

This comment has been minimized.

Show comment
Hide comment
@ALRubinger

ALRubinger Jul 16, 2011

Nice stuff; welcome to contribution to ShrinkWrap. :) Probably a test case is in order too, to ensure our expected output?

Nice stuff; welcome to contribution to ShrinkWrap. :) Probably a test case is in order too, to ensure our expected output?

@ALRubinger

This comment has been minimized.

Show comment
Hide comment
@ALRubinger

ALRubinger Jul 16, 2011

Hmm, when flushing out this to disk, "file" command here doesn't recognize as ZIP.

This one does, though:

byte[] emptyZip = new byte[]
{0x50, 0x4b, 0x03, 0x04, 0x14, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 94 + 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
0x0, 0x0};

Hmm, when flushing out this to disk, "file" command here doesn't recognize as ZIP.

This one does, though:

byte[] emptyZip = new byte[]
{0x50, 0x4b, 0x03, 0x04, 0x14, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 94 + 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
0x0, 0x0};

This comment has been minimized.

Show comment
Hide comment
@ALRubinger

ALRubinger Jul 16, 2011

Even better: byte[] emptyZip = new byte[]
{0x50, 0x4b, 0x03, 0x04, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
0x0, 0x0};

Even better: byte[] emptyZip = new byte[]
{0x50, 0x4b, 0x03, 0x04, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
0x0, 0x0};

This comment has been minimized.

Show comment
Hide comment
@ALRubinger

ALRubinger Jul 16, 2011

And even still, the above commit assumes that this.exporter is using the JDK ZIP exporter. Maybe instead return this array if (this.exporter instanceof ZipExporter), else let other exporter impls stream out the empty contents (so we don't write an empty ZIP when it should perhaps be an empty TAR.GZ).

And even still, the above commit assumes that this.exporter is using the JDK ZIP exporter. Maybe instead return this array if (this.exporter instanceof ZipExporter), else let other exporter impls stream out the empty contents (so we don't write an empty ZIP when it should perhaps be an empty TAR.GZ).

This comment has been minimized.

Show comment
Hide comment
@gpoul

gpoul Jul 17, 2011

Owner

Unfortunately the "even better" emptyZip above doesn't work for me. Neither the Windows ZIP-routines nor 7-zip accept this as a valid empty zip.

Owner

gpoul replied Jul 17, 2011

Unfortunately the "even better" emptyZip above doesn't work for me. Neither the Windows ZIP-routines nor 7-zip accept this as a valid empty zip.

This comment has been minimized.

Show comment
Hide comment
@ALRubinger

ALRubinger Jul 18, 2011

How about the first one? And your thoughts on my proposal for the this.exporter logic?

How about the first one? And your thoughts on my proposal for the this.exporter logic?

This comment has been minimized.

Show comment
Hide comment
@gpoul

gpoul Jul 18, 2011

Owner

this.exporter logic is already in the pull request in a slightly modified form. - the first ZIP also doesn't work. It has the same failure behavior as the second one.

Owner

gpoul replied Jul 18, 2011

this.exporter logic is already in the pull request in a slightly modified form. - the first ZIP also doesn't work. It has the same failure behavior as the second one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment