-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
[JEP-202] Extending VirtualFile #3302
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
3f01a77
8e603a5
e773bdb
4911c5c
e3e6d57
2e070ca
9735a34
a9430d3
e0fab1a
9ccffba
7131067
9b9f81d
23f12fd
63885e6
64ee6fd
c7372cc
fca93db
d32b9b1
beea2aa
7223632
d3f5346
2e1af78
90fd89f
9b75cd2
8e60538
2090468
a345051
547fd6f
a54d49b
d25c0b2
8f5d1d3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,9 +29,11 @@ | |
| import java.io.InputStream; | ||
| import java.io.OutputStream; | ||
| import java.io.Serializable; | ||
| import java.net.URL; | ||
| import java.text.Collator; | ||
| import java.util.ArrayList; | ||
| import java.util.Arrays; | ||
| import java.util.Collection; | ||
| import java.util.Collections; | ||
| import java.util.Comparator; | ||
| import java.util.List; | ||
|
|
@@ -51,6 +53,7 @@ | |
| import org.kohsuke.accmod.Restricted; | ||
| import org.kohsuke.accmod.restrictions.NoExternalUse; | ||
| import org.kohsuke.stapler.HttpResponse; | ||
| import org.kohsuke.stapler.HttpResponses; | ||
| import org.kohsuke.stapler.StaplerRequest; | ||
| import org.kohsuke.stapler.StaplerResponse; | ||
|
|
||
|
|
@@ -213,7 +216,7 @@ private void serveFile(StaplerRequest req, StaplerResponse rsp, VirtualFile root | |
| String rest = _rest.toString(); | ||
|
|
||
| // this is the base file/directory | ||
| VirtualFile baseFile = root.child(base); | ||
| VirtualFile baseFile = base.isEmpty() ? root : root.child(base); | ||
|
|
||
| if(baseFile.isDirectory()) { | ||
| if(zip) { | ||
|
|
@@ -295,6 +298,14 @@ private void serveFile(StaplerRequest req, StaplerResponse rsp, VirtualFile root | |
| return; | ||
| } | ||
|
|
||
| URL external = baseFile.toExternalURL(); | ||
| if (external != null) { | ||
| // or this URL could be emitted directly from dir.jelly | ||
| // though we would prefer to delay toExternalURL calls unless and until needed | ||
| rsp.sendRedirect2(external.toExternalForm()); | ||
| return; | ||
| } | ||
|
|
||
| long lastModified = baseFile.lastModified(); | ||
| long length = baseFile.length(); | ||
|
|
||
|
|
@@ -355,7 +366,8 @@ private static String createBackRef(int times) { | |
| private static void zip(OutputStream outputStream, VirtualFile dir, String glob) throws IOException { | ||
| try (ZipOutputStream zos = new ZipOutputStream(outputStream)) { | ||
| zos.setEncoding(System.getProperty("file.encoding")); // TODO JENKINS-20663 make this overridable via query parameter | ||
| for (String n : dir.list(glob.length() == 0 ? "**" : glob)) { | ||
| // TODO consider using run(Callable) here | ||
| for (String n : dir.list(glob.isEmpty() ? "**" : glob, null, /* TODO what is the user expectation? */true)) { | ||
| String relativePath; | ||
| if (glob.length() == 0) { | ||
| // JENKINS-19947: traditional behavior is to prepend the directory name | ||
|
|
@@ -535,10 +547,10 @@ private static List<List<Path>> buildChildPaths(VirtualFile cur, Locale locale) | |
| * @param baseRef String like "../../../" that cancels the 'rest' portion. Can be "./" | ||
| */ | ||
| private static List<List<Path>> patternScan(VirtualFile baseDir, String pattern, String baseRef) throws IOException { | ||
| String[] files = baseDir.list(pattern); | ||
| Collection<String> files = baseDir.list(pattern, null, /* TODO what is the user expectation? */true); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that there was a JEP-200 regression in the similar code recently
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, in |
||
|
|
||
| if (files.length > 0) { | ||
| List<List<Path>> r = new ArrayList<List<Path>>(files.length); | ||
| if (!files.isEmpty()) { | ||
| List<List<Path>> r = new ArrayList<List<Path>>(files.size()); | ||
| for (String match : files) { | ||
| List<Path> file = buildPathList(baseDir, baseDir.child(match), baseRef); | ||
| r.add(file); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -73,6 +73,7 @@ | |
| import java.io.PrintWriter; | ||
| import java.io.RandomAccessFile; | ||
| import java.io.Reader; | ||
| import java.io.Serializable; | ||
| import java.nio.charset.Charset; | ||
| import java.text.DateFormat; | ||
| import java.text.SimpleDateFormat; | ||
|
|
@@ -108,6 +109,7 @@ | |
| import jenkins.model.StandardArtifactManager; | ||
| import jenkins.model.lazy.BuildReference; | ||
| import jenkins.model.lazy.LazyBuildMixIn; | ||
| import jenkins.security.MasterToSlaveCallable; | ||
| import jenkins.util.VirtualFile; | ||
| import jenkins.util.io.OnMaster; | ||
| import net.sf.json.JSONObject; | ||
|
|
@@ -1090,12 +1092,16 @@ public File getArtifactsDir() { | |
| * @return The list can be empty but never null | ||
| */ | ||
| public @Nonnull List<Artifact> getArtifactsUpTo(int artifactsNumber) { | ||
| ArtifactList r = new ArtifactList(); | ||
| SerializableArtifactList sal; | ||
| VirtualFile root = getArtifactManager().root(); | ||
| try { | ||
| addArtifacts(getArtifactManager().root(), "", "", r, null, artifactsNumber); | ||
| sal = root.run(new AddArtifacts(root, artifactsNumber)); | ||
| } catch (IOException x) { | ||
| LOGGER.log(Level.WARNING, null, x); | ||
| sal = new SerializableArtifactList(); | ||
| } | ||
| ArtifactList r = new ArtifactList(); | ||
| r.updateFrom(sal); | ||
| r.computeDisplayName(); | ||
| return r; | ||
| } | ||
|
|
@@ -1109,9 +1115,25 @@ public boolean getHasArtifacts() { | |
| return !getArtifactsUpTo(1).isEmpty(); | ||
| } | ||
|
|
||
| private int addArtifacts(@Nonnull VirtualFile dir, | ||
| private static final class AddArtifacts extends MasterToSlaveCallable<SerializableArtifactList, IOException> { | ||
| private static final long serialVersionUID = 1L; | ||
| private final VirtualFile root; | ||
| private final int artifactsNumber; | ||
| AddArtifacts(VirtualFile root, int artifactsNumber) { | ||
| this.root = root; | ||
| this.artifactsNumber = artifactsNumber; | ||
| } | ||
| @Override | ||
| public SerializableArtifactList call() throws IOException { | ||
| SerializableArtifactList sal = new SerializableArtifactList(); | ||
| addArtifacts(root, "", "", sal, null, artifactsNumber); | ||
| return sal; | ||
| } | ||
| } | ||
|
|
||
| private static int addArtifacts(@Nonnull VirtualFile dir, | ||
| @Nonnull String path, @Nonnull String pathHref, | ||
| @Nonnull ArtifactList r, @Nonnull Artifact parent, int upTo) throws IOException { | ||
| @Nonnull SerializableArtifactList r, @Nonnull SerializableArtifact parent, int upTo) throws IOException { | ||
| VirtualFile[] kids = dir.list(); | ||
| Arrays.sort(kids); | ||
|
|
||
|
|
@@ -1122,26 +1144,26 @@ private int addArtifacts(@Nonnull VirtualFile dir, | |
| String childHref = pathHref + Util.rawEncode(child); | ||
| String length = sub.isFile() ? String.valueOf(sub.length()) : ""; | ||
| boolean collapsed = (kids.length==1 && parent!=null); | ||
| Artifact a; | ||
| SerializableArtifact a; | ||
| if (collapsed) { | ||
| // Collapse single items into parent node where possible: | ||
| a = new Artifact(parent.getFileName() + '/' + child, childPath, | ||
| a = new SerializableArtifact(parent.name + '/' + child, childPath, | ||
| sub.isDirectory() ? null : childHref, length, | ||
| parent.getTreeNodeId()); | ||
| parent.treeNodeId); | ||
| r.tree.put(a, r.tree.remove(parent)); | ||
| } else { | ||
| // Use null href for a directory: | ||
| a = new Artifact(child, childPath, | ||
| a = new SerializableArtifact(child, childPath, | ||
| sub.isDirectory() ? null : childHref, length, | ||
| "n" + ++r.idSeq); | ||
| r.tree.put(a, parent!=null ? parent.getTreeNodeId() : null); | ||
| r.tree.put(a, parent!=null ? parent.treeNodeId : null); | ||
| } | ||
| if (sub.isDirectory()) { | ||
| n += addArtifacts(sub, childPath + '/', childHref + '/', r, a, upTo-n); | ||
| if (n>=upTo) break; | ||
| } else { | ||
| // Don't store collapsed path in ArrayList (for correct data in external API) | ||
| r.add(collapsed ? new Artifact(child, a.relativePath, a.href, length, a.treeNodeId) : a); | ||
| r.add(collapsed ? new SerializableArtifact(child, a.relativePath, a.href, length, a.treeNodeId) : a); | ||
| if (++n>=upTo) break; | ||
| } | ||
| } | ||
|
|
@@ -1159,6 +1181,30 @@ private int addArtifacts(@Nonnull VirtualFile dir, | |
| public static final int TREE_CUTOFF = Integer.parseInt(SystemProperties.getString("hudson.model.Run.ArtifactList.treeCutoff", "40")); | ||
|
|
||
| // ..and then "too many" | ||
|
|
||
| /** {@link Run.Artifact} without the implicit link to {@link Run} */ | ||
| private static final class SerializableArtifact implements Serializable { | ||
| private static final long serialVersionUID = 1L; | ||
| final String name; | ||
| final String relativePath; | ||
| final String href; | ||
| final String length; | ||
| final String treeNodeId; | ||
| SerializableArtifact(String name, String relativePath, String href, String length, String treeNodeId) { | ||
| this.name = name; | ||
| this.relativePath = relativePath; | ||
| this.href = href; | ||
| this.length = length; | ||
| this.treeNodeId = treeNodeId; | ||
| } | ||
| } | ||
|
|
||
| /** {@link Run.ArtifactList} without the implicit link to {@link Run} */ | ||
| private static final class SerializableArtifactList extends ArrayList<SerializableArtifact> { | ||
| private static final long serialVersionUID = 1L; | ||
| private LinkedHashMap<SerializableArtifact, String> tree = new LinkedHashMap<>(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even for a private class not looks a bit dangerous since you do not override modification methods to put data to both containers (array and tree)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code was already a mess and I am trying to touch it as little as possible. The problem here was that |
||
| private int idSeq = 0; | ||
| } | ||
|
|
||
| public final class ArtifactList extends ArrayList<Artifact> { | ||
| private static final long serialVersionUID = 1L; | ||
|
|
@@ -1167,7 +1213,24 @@ public final class ArtifactList extends ArrayList<Artifact> { | |
| * Contains Artifact objects for directories and files (the ArrayList contains only files). | ||
| */ | ||
| private LinkedHashMap<Artifact,String> tree = new LinkedHashMap<Artifact,String>(); | ||
| private int idSeq = 0; | ||
|
|
||
| void updateFrom(SerializableArtifactList clone) { | ||
| Map<String, Artifact> artifacts = new HashMap<>(); // need to share objects between tree and list, since computeDisplayName mutates displayPath | ||
| for (SerializableArtifact sa : clone) { | ||
| Artifact a = new Artifact(sa); | ||
| artifacts.put(a.relativePath, a); | ||
| add(a); | ||
| } | ||
| tree = new LinkedHashMap<>(); | ||
| for (Map.Entry<SerializableArtifact, String> entry : clone.tree.entrySet()) { | ||
| SerializableArtifact sa = entry.getKey(); | ||
| Artifact a = artifacts.get(sa.relativePath); | ||
| if (a == null) { | ||
| a = new Artifact(sa); | ||
| } | ||
| tree.put(a, entry.getValue()); | ||
| } | ||
| } | ||
|
|
||
| public Map<Artifact,String> getTree() { | ||
| return tree; | ||
|
|
@@ -1282,6 +1345,10 @@ public class Artifact { | |
| */ | ||
| private String length; | ||
|
|
||
| Artifact(SerializableArtifact clone) { | ||
| this(clone.name, clone.relativePath, clone.href, clone.length, clone.treeNodeId); | ||
| } | ||
|
|
||
| /*package for test*/ Artifact(String name, String relativePath, String href, String len, String treeNodeId) { | ||
| this.name = name; | ||
| this.relativePath = relativePath; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,6 @@ | |
| import java.io.File; | ||
| import java.io.FileFilter; | ||
| import java.io.IOException; | ||
| import java.io.InterruptedIOException; | ||
| import java.io.Serializable; | ||
|
|
||
| import static hudson.Util.fixEmpty; | ||
|
|
@@ -31,19 +30,15 @@ public abstract class DirScanner implements Serializable { | |
| */ | ||
| protected final void scanSingle(File f, String relative, FileVisitor visitor) throws IOException { | ||
| if (visitor.understandsSymlink()) { | ||
| String target; | ||
| try { | ||
| String target; | ||
| try { | ||
| target = Util.resolveSymlink(f); | ||
| } catch (IOException x) { // JENKINS-13202 | ||
| target = null; | ||
| } | ||
| if (target != null) { | ||
| visitor.visitSymlink(f, target, relative); | ||
| return; | ||
| } | ||
| } catch (InterruptedException e) { | ||
| throw (IOException) new InterruptedIOException().initCause(e); | ||
| target = Util.resolveSymlink(f); | ||
| } catch (IOException x) { // JENKINS-13202 | ||
| target = null; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add logging while you are around?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Preferred to avoid making unrelated changes.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed |
||
| } | ||
| if (target != null) { | ||
| visitor.visitSymlink(f, target, relative); | ||
| return; | ||
| } | ||
| } | ||
| visitor.visit(f, relative); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Binary compatible simplification—the body never actually threw that exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not source compatible IIRC (javac fails when there is a catch block for impossible exception), may impact PCT. There are usages of it externally, e.g. in the Support Core plugin: https://github.com/search?p=1&q=org%3Ajenkinsci+resolveSymlink&type=Code
I would propose to detach it to a separate PR if you feel it's important enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oleg-nenashev Jesse already disputed this in #3340 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, PCT should be unaffected, though it could impede
Why the Java language treats an exception which cannot be thrown as a fatal error rather than a warning (ditto unreachable statements etc.), I do not know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The simple workaround for the
buildPlugincase would be to justcatch (ExceptionI guess.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be better to fix it tho