hansva commented on code in PR #9212:
URL: https://github.com/apache/hop/pull/9212#discussion_r4444295495
##########
plugins/actions/xml/src/main/java/org/apache/hop/workflow/actions/xml/xslt/Xslt.java:
##########
@@ -81,68 +84,68 @@ public class Xslt extends ActionBase implements Cloneable,
IAction {
public static final String CONST_ACTION_XSLT_OUPUT_FILE_EXISTS_2_LABEL =
"ActionXSLT.OuputFileExists2.Label";
- @HopMetadataProperty private String xmlfilename;
-
- @HopMetadataProperty private String xslfilename;
+ @HopMetadataProperty(key = "xmlfilename")
+ private String xmlFilename;
- @HopMetadataProperty private String outputfilename;
+ @HopMetadataProperty(key = "xslfilename")
+ private String xslFilename;
- @HopMetadataProperty public int ifFileExists;
+ @HopMetadataProperty(key = "outputfilename")
+ private String outputFilename;
- private boolean addfiletoresult;
+ @HopMetadataProperty private int ifFileExists;
Review Comment:
**[suggestion]** This setting has the same "not read back" problem as the
fields this PR restores, just from the other side. Before the XML cleanup in
57bd0dcdab (2024-11), `getXml()` / `loadXml()` used the tag `iffileexists`.
Since then the field is serialized as `ifFileExists`, and the bundled sample
and the new test resource use that, so changing the key would break files saved
since. A workflow saved before the cleanup still loses this option and falls
back to the default (1, do nothing).
**Suggestion:** implement `ILegacyXml` on `Xslt` and map the old tag in
`convertLegacyXml`. `XmlMetadataUtil` calls it after the normal
deserialization; `ActionAbort.convertLegacyXml` uses the same pattern:
```java
@Override
public void convertLegacyXml(Node node) {
if (XmlHandler.getTagValue(node, "ifFileExists") == null) {
String legacy = XmlHandler.getTagValue(node, "iffileexists");
if (legacy != null) {
ifFileExists = Const.toInt(legacy, 1);
}
}
}
```
A test that loads an action node with `<iffileexists>2</iffileexists>` and
no `ifFileExists` would cover it, next to the round-trip test in `XsltTest`.
##########
plugins/actions/xml/src/main/java/org/apache/hop/workflow/actions/xml/xslt/Xslt.java:
##########
@@ -353,6 +334,8 @@ private boolean processOneXMLFile(
// did not find wildcard
outputfilename = outputfilename + "_" +
StringUtil.getFormattedDateTimeNow(true);
}
+ // Re-resolve so the transformation writes to the unique name, not
the original file
+ outputfile = HopVfs.getFileObject(outputfilename);
Review Comment:
**[nit]** The `FileObject` for the original output name isn't closed before
it is replaced by the one for the unique name, so only the new one is closed in
the `finally` block. Close the old one first.
##########
plugins/actions/xml/src/main/java/org/apache/hop/workflow/actions/xml/xslt/Xslt.java:
##########
@@ -481,71 +464,56 @@ private boolean processOneXMLFile(
+ BaseMessages.getString(PKG, "ActionXSLT.ErrorXLSTXSL2.Label")
+ e.getMessage());
} finally {
- try {
- if (xmlfile != null) {
- xmlfile.close();
- }
-
- if (xslfile != null) {
- xslfile.close();
- }
- if (outputfile != null) {
- outputfile.close();
- }
- } catch (IOException e) {
- logError("Unable to close file", e);
- }
+ // Close FileObjects first, then layered file systems (zip/jar). On
Windows the archive
+ // stays locked until the FileSystem itself is closed, which breaks
@TempDir cleanup in tests.
+ closeFileAndLayeredSystem(xmlfile);
+ closeFileAndLayeredSystem(xslfile);
+ closeFileAndLayeredSystem(outputfile);
}
return retval;
}
- @Override
- public boolean isEvaluation() {
- return true;
- }
-
- /*
- public void setXmlFilename(String filename) {
- this.xmlfilename = filename;
- }
-
- public String getXmlFilename() {
- return xmlfilename;
+ private void closeFileAndLayeredSystem(FileObject file) {
+ if (file == null) {
+ return;
}
-
- public String getOutputFilename() {
- return outputfilename;
- }
-
- public void setOutputfilename(String outputfilename) {
- this.outputfilename = outputfilename;
+ FileSystem fileSystem = null;
+ try {
+ fileSystem = file.getFileSystem();
+ } catch (Exception ignored) {
+ // keep going and still try to close the FileObject
}
-
- public void setxslFilename(String filename) {
- this.xslfilename = filename;
+ try {
+ file.close();
+ } catch (IOException e) {
+ logError("Unable to close file", e);
}
-
- public String getxslFilename() {
- return xslfilename;
+ if (fileSystem == null) {
+ return;
}
-
- public void setAddFileToResult(boolean addfiletoresultin) {
- this.addfiletoresult = addfiletoresultin;
+ try {
+ String scheme = fileSystem.getRoot().getName().getScheme();
+ if ("zip".equalsIgnoreCase(scheme) || "jar".equalsIgnoreCase(scheme)) {
+ fileSystem.getFileSystemManager().closeFileSystem(fileSystem);
Review Comment:
**[bug]** `closeFileSystem` closes the zip/jar file system for every user of
that archive, not only for this action.
Commons VFS (2.10.0) caches one `ZipFileSystem` per archive, and every
`zip:file:///data/in.zip!/...` path resolved through the same Hop VFS manager
shares it. `AbstractFileProvider.closeFileSystem` removes it from the cache and
calls `fs.close()`, which ends in `ZipFileSystem.doCloseCommunicationLink()` →
`zipFile.close()`. Closing a `java.util.zip.ZipFile` closes every input stream
it returned, in any thread.
Scenario: a workflow with two parallel branches. Branch A runs this XSLT
action on `zip:file:///data/in.zip!/a.xml`, branch B runs a pipeline whose Text
file input streams `zip:file:///data/in.zip!/b.csv`. When A finishes, B's open
stream fails with `IOException: Stream closed` partway through the file. Two
XSLT actions on the same archive in parallel branches race the same way. New
reads after the close reopen the archive lazily, so it shows up as an
intermittent failure that is hard to trace back to this action.
It isn't needed for #9172: it was added so Windows releases the archive lock
for `@TempDir` cleanup in the tests.
**Suggestion:** close only the `FileObject`s here, and close the layered
file system in the test teardown (for example
`HopVfs.getFileSystemManager().closeFileSystem(...)` in an `@AfterEach`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
