Skip to content

Prevent reports from reading partially written Allure attachments #1371

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.

Sign up for GitHub

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

Open
baev wants to merge 1 commit into
base: main
Choose a base branch
from
+210 −2
Open

Prevent reports from reading partially written Allure attachments #1371

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions allure-java-commons/src/main/java/io/qameta/allure/FileSystemResultsWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
import java.io.OutputStream;
import java.nio.channels.Channels;
import java.nio.channels.FileChannel;
import java.nio.file.AtomicMoveNotSupportedException;
import java.nio.file.FileAlreadyExistsException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.StandardCopyOption;
Expand Down Expand Up @@ -120,11 +122,23 @@ private void write(final Path file, final String entityName, final ChannelConsum
consumer.accept(channel);
channel.force(true);
}
Files.move(tempFile, file, StandardCopyOption.REPLACE_EXISTING);
try {
Files.move(
tempFile,
file,
StandardCopyOption.REPLACE_EXISTING,
StandardCopyOption.ATOMIC_MOVE
);
} catch (AtomicMoveNotSupportedException
| FileAlreadyExistsException
| UnsupportedOperationException ignored) {
Files.move(tempFile, file, StandardCopyOption.REPLACE_EXISTING);
}
tempFile = null;
} catch (IOException e) {
deleteIfExists(tempFile);
throw new AllureResultsWriteException(getErrorMessage(entityName), e);
} finally {
deleteIfExists(tempFile);
}
}

Expand Down
194 changes: 194 additions & 0 deletions allure-java-commons/src/test/java/io/qameta/allure/FileSystemResultsWriterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,21 @@
import io.qameta.allure.model.TestResultContainer;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;
import org.mockito.MockedStatic;

import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import java.nio.file.AtomicMoveNotSupportedException;
import java.nio.file.CopyOption;
import java.nio.file.FileAlreadyExistsException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.StandardCopyOption;
import java.util.Arrays;
import java.util.List;
import java.util.UUID;
Expand All @@ -38,6 +46,12 @@
import static io.qameta.allure.test.ThreadLocalEnhancedRandom.current;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.AdditionalMatchers.aryEq;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.CALLS_REAL_METHODS;
import static org.mockito.Mockito.mockStatic;
import static org.mockito.Mockito.never;

public class FileSystemResultsWriterTest {

Expand Down Expand Up @@ -125,6 +139,169 @@ void shouldWriteAttachmentFile(@TempDir final Path folder) throws IOException {
.isEqualTo(content);
}

/**
* Verifies that a supported atomic move publishes an attachment without using the fallback.
*
* @param folder the temporary results directory
*/
@Test
void shouldPublishAttachmentAtomicallyWhenSupported(@TempDir final Path folder) throws IOException {
FileSystemResultsWriter writer = new FileSystemResultsWriter(folder);
final String source = "source-attachment.txt";
final String content = "attachment body";
final Path attachmentFile = folder.resolve(source);

try (MockedStatic<Files> files = mockStatic(Files.class, CALLS_REAL_METHODS)) {
Allure.step(
"Write attachment using an atomic move",
() -> writer.write(source, new ByteArrayInputStream(content.getBytes(StandardCharsets.UTF_8)))
);

Allure.step("Verify the atomic move was used without the fallback", () -> {
files.verify(
() -> Files.move(
any(Path.class),
eq(attachmentFile),
aryEq(
new CopyOption[]{
StandardCopyOption.REPLACE_EXISTING,
StandardCopyOption.ATOMIC_MOVE
}
)
)
);
files.verify(
() -> Files.move(
any(Path.class),
eq(attachmentFile),
aryEq(new CopyOption[]{StandardCopyOption.REPLACE_EXISTING})
),
never()
);
});
}

assertThat(Files.readString(attachmentFile))
.isEqualTo(content);
assertThat(listFiles(folder))
.containsExactly(attachmentFile);
}

/**
* Verifies that an atomic-move failure still replaces an attachment with a complete file using a regular move.
*
* @param failureName the atomic-move failure scenario
* @param failure the atomic-move failure
* @param folder the temporary results directory
*/
@ParameterizedTest(name = "{0}")
@MethodSource("atomicMoveFallbackFailures")
void shouldReplaceAttachmentUsingFallbackWhenAtomicMoveFails(
final String failureName,
final Exception failure,
@TempDir final Path folder)
throws IOException {
FileSystemResultsWriter writer = new FileSystemResultsWriter(folder);
final String source = "source-attachment.txt";
final String content = "attachment body";
final Path attachmentFile = folder.resolve(source);
Files.writeString(attachmentFile, "previous attachment body");

try (MockedStatic<Files> files = mockStatic(Files.class, CALLS_REAL_METHODS)) {
files.when(
() -> Files.move(
any(Path.class),
eq(attachmentFile),
aryEq(
new CopyOption[]{
StandardCopyOption.REPLACE_EXISTING,
StandardCopyOption.ATOMIC_MOVE
}
)
)
)
.thenThrow(failure);

Allure.step(
"Write attachment after the atomic move fails",
() -> writer.write(source, new ByteArrayInputStream(content.getBytes(StandardCharsets.UTF_8)))
);

Allure.step("Verify the atomic move was attempted before the fallback", () -> {
files.verify(
() -> Files.move(
any(Path.class),
eq(attachmentFile),
aryEq(
new CopyOption[]{
StandardCopyOption.REPLACE_EXISTING,
StandardCopyOption.ATOMIC_MOVE
}
)
)
);
files.verify(
() -> Files.move(
any(Path.class),
eq(attachmentFile),
aryEq(new CopyOption[]{StandardCopyOption.REPLACE_EXISTING})
)
);
});
}

assertThat(Files.readString(attachmentFile))
.isEqualTo(content);
assertThat(listFiles(folder))
.containsExactly(attachmentFile);
}

@Test
void shouldDeleteTemporaryFileWhenFallbackMoveFailsUnchecked(@TempDir final Path folder) throws IOException {
FileSystemResultsWriter writer = new FileSystemResultsWriter(folder);
final String source = "source-attachment.txt";
final String content = "attachment body";
final Path attachmentFile = folder.resolve(source);

try (MockedStatic<Files> files = mockStatic(Files.class, CALLS_REAL_METHODS)) {
files.when(
() -> Files.move(
any(Path.class),
eq(attachmentFile),
aryEq(
new CopyOption[]{
StandardCopyOption.REPLACE_EXISTING,
StandardCopyOption.ATOMIC_MOVE
}
)
)
)
.thenThrow(new UnsupportedOperationException("Atomic move is not supported"));
files.when(
() -> Files.move(
any(Path.class),
eq(attachmentFile),
aryEq(new CopyOption[]{StandardCopyOption.REPLACE_EXISTING})
)
)
.thenThrow(new IllegalStateException("Fallback move failed"));

assertThatThrownBy(
() -> writer.write(
source,
new ByteArrayInputStream(content.getBytes(StandardCharsets.UTF_8))
)
)
.isInstanceOf(IllegalStateException.class)
.hasMessage("Fallback move failed");
}

assertThat(attachmentFile)
.doesNotExist();
assertThat(listFiles(folder))
.isEmpty();
}

@Test
void shouldNotCreateFinalAttachmentFileWhenStreamFails(@TempDir final Path folder) throws IOException {
FileSystemResultsWriter writer = new FileSystemResultsWriter(folder);
Expand Down Expand Up @@ -169,6 +346,23 @@ private static List<Path> listFiles(final Path folder) throws IOException {
}
}

private static Stream<Arguments> atomicMoveFallbackFailures() {
return Stream.of(
Arguments.of(
"Atomic move is not supported",
new AtomicMoveNotSupportedException("source", "target", "Not supported")
),
Arguments.of(
"Atomic move option is not supported",
new UnsupportedOperationException("Atomic move is not supported")
),
Arguments.of(
"Atomic move cannot replace an existing target",
new FileAlreadyExistsException("target")
)
);
}

private static final class FailingInputStream extends InputStream {

private final byte[] content;
Expand Down