Copilot commented on code in PR #247:
URL:
https://github.com/apache/maven-shade-plugin/pull/247#discussion_r3673683621
##########
src/main/java/org/apache/maven/plugins/shade/resource/AppendingTransformer.java:
##########
@@ -46,8 +46,11 @@ public boolean canTransformResource(String r) {
@Override
public void processResource(String resource, InputStream is,
List<Relocator> relocators, long time)
throws IOException {
+ if (data.size() > 0) {
+ // Append the EOL before the new content to ensure the EOL is not
at the end of the file.
+ data.write('\n');
+ }
IOUtil.copy(is, data);
Review Comment:
Using `data.size() > 0` to decide whether to insert the separator breaks the
case where a previously processed resource is empty (0 bytes). In that
scenario, the next resource will be appended without any separator, changing
behavior compared to the previous implementation. Consider tracking whether
`processResource` has been called at least once (e.g., a `boolean hasAppended`)
instead of relying on `data.size()`.
##########
src/test/java/org/apache/maven/plugins/shade/resource/AppendingTransformerTest.java:
##########
@@ -56,4 +64,37 @@ public void testCanTransformResource() {
assertTrue(transformer.canTransformResource("ABCDEFGHIJKLMNOPQRSTUVWXYZ"));
assertFalse(transformer.canTransformResource("META-INF/MANIFEST.MF"));
}
+
+ @Test
+ public void testProcessResource() throws IOException {
+ transformer.resource = "test-resource";
+ String firstLine = "first line";
+ String secondLine = "second line";
+ InputStream firstIs = new ByteArrayInputStream(firstLine.getBytes());
+ InputStream secondIs = new ByteArrayInputStream(secondLine.getBytes());
+
+ transformer.processResource("", firstIs, Collections.emptyList());
+ transformer.processResource("", secondIs, Collections.emptyList());
+
+ final ByteArrayOutputStream out = new ByteArrayOutputStream();
+ try (final JarOutputStream jarOutputStream = new JarOutputStream(out))
{
+ transformer.modifyOutputStream(jarOutputStream);
+ }
+
+ try (final JarInputStream jis = new JarInputStream(new
ByteArrayInputStream(out.toByteArray()))) {
+ assertEquals("test-resource", jis.getNextJarEntry().getName());
+ String result = read(jis);
+ assertEquals(firstLine + "\n" + secondLine, result);
+ }
+ }
+
+ private String read(final JarInputStream jar) throws IOException {
+ final StringBuilder builder = new StringBuilder();
+ final byte[] buffer = new byte[512];
+ int read;
+ while ((read = jar.read(buffer)) >= 0) {
+ builder.append(new String(buffer, 0, read));
+ }
Review Comment:
Looping on `>= 0` can hang if `read(...)` ever returns `0` (which some
`InputStream` implementations can do), because the buffer contents won’t
advance and the loop won’t terminate. Prefer looping on `> 0`, or handle `0` by
continuing, or use a utility that reads until `-1` safely.
##########
src/main/java/org/apache/maven/plugins/shade/resource/AppendingTransformer.java:
##########
@@ -46,8 +46,11 @@ public boolean canTransformResource(String r) {
@Override
public void processResource(String resource, InputStream is,
List<Relocator> relocators, long time)
throws IOException {
+ if (data.size() > 0) {
+ // Append the EOL before the new content to ensure the EOL is not
at the end of the file.
+ data.write('\n');
+ }
IOUtil.copy(is, data);
Review Comment:
This always inserts a `\\n` between resources even if the previous resource
content already ends with `\\n`, which can introduce an unintended blank line
between merged resources. If the goal is exactly one separator between entries
(but none at EOF), consider tracking whether the previous copy ended with a
newline and only writing `\\n` when needed (e.g., maintain a
`lastByteWasNewline` flag while copying).
##########
src/main/java/org/apache/maven/plugins/shade/resource/AppendingTransformer.java:
##########
@@ -46,8 +46,11 @@ public boolean canTransformResource(String r) {
@Override
public void processResource(String resource, InputStream is,
List<Relocator> relocators, long time)
throws IOException {
+ if (data.size() > 0) {
+ // Append the EOL before the new content to ensure the EOL is not
at the end of the file.
+ data.write('\n');
+ }
IOUtil.copy(is, data);
Review Comment:
The new separator behavior introduces edge cases that aren’t covered by the
added test: (1) when an input resource already ends with `\\n` (to ensure you
don’t get double separators), and (2) when an earlier processed resource is
empty (to ensure separators still behave correctly). Adding tests for these
cases would prevent regressions.
##########
src/test/java/org/apache/maven/plugins/shade/resource/AppendingTransformerTest.java:
##########
@@ -56,4 +64,37 @@ public void testCanTransformResource() {
assertTrue(transformer.canTransformResource("ABCDEFGHIJKLMNOPQRSTUVWXYZ"));
assertFalse(transformer.canTransformResource("META-INF/MANIFEST.MF"));
}
+
+ @Test
+ public void testProcessResource() throws IOException {
+ transformer.resource = "test-resource";
+ String firstLine = "first line";
+ String secondLine = "second line";
+ InputStream firstIs = new ByteArrayInputStream(firstLine.getBytes());
+ InputStream secondIs = new ByteArrayInputStream(secondLine.getBytes());
Review Comment:
The test uses the platform default charset via `String#getBytes()`, which
can make the test non-portable. Use an explicit charset (e.g.,
`StandardCharsets.UTF_8`) for both `getBytes(...)` and decoding in `read(...)`
to keep behavior consistent across environments.
--
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]
