Copilot commented on code in PR #2120:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2120#discussion_r2853135774
##########
libminifi/src/core/ForwardingContentSession.cpp:
##########
@@ -51,6 +51,11 @@ std::shared_ptr<io::BaseStream>
ForwardingContentSession::append(const std::shar
return repository_->write(*resource_id, true);
}
+void ForwardingContentSession::remove(const std::shared_ptr<ResourceClaim>&
resource_id) {
Review Comment:
The remove() method only erases the claim from the internal tracking sets
but doesn't delete the actual content from the repository. Since
ForwardingContentSession writes directly to the repository (line 43), when a
write is cancelled, the content file will remain on disk as an orphan. Add a
call to repository_->remove(*resource_id) to properly clean up the physical
file when remove() is called.
```suggestion
void ForwardingContentSession::remove(const std::shared_ptr<ResourceClaim>&
resource_id) {
if (repository_ && resource_id) {
repository_->remove(*resource_id);
}
```
##########
libminifi/test/libtest/unit/ContentRepositoryDependentTests.h:
##########
@@ -171,4 +171,64 @@ void
testReadFromZeroLengthFlowFile(std::shared_ptr<core::ContentRepository> con
REQUIRE_NOTHROW(process_session.readBuffer(flow_file));
REQUIRE_NOTHROW(process_session.read(flow_file, ReadUntilItCan{}));
}
+
+inline void testErrWrite(std::shared_ptr<core::ContentRepository>
content_repo) {
+ const auto fixture = Fixture(std::move(content_repo));
+ core::ProcessSession& process_session = fixture.processSession();
+ const auto flow_file = process_session.create();
+ fixture.writeToFlowFile(flow_file, "original_content");
+ fixture.transferAndCommit(flow_file);
+
+ REQUIRE_THROWS(
+ process_session.write(flow_file, [](const
std::shared_ptr<minifi::io::OutputStream>& output_stream) {
+ output_stream->write("new_content");
+ return MinifiIoStatus::MINIFI_IO_ERROR;
+ }));
+
+ CHECK(flow_file->getSize() == 16);
+ ReadUntilItCan read_until_it_can_callback;
+ const auto read_result = process_session.readBuffer(flow_file);
+ process_session.read(flow_file, std::ref(read_until_it_can_callback));
+ CHECK(to_string(read_result) == "original_content");
+}
+
+inline void testOkWrite(std::shared_ptr<core::ContentRepository> content_repo)
{
+ const auto fixture = Fixture(std::move(content_repo));
+ core::ProcessSession& process_session = fixture.processSession();
+ const auto flow_file = process_session.create();
+ fixture.writeToFlowFile(flow_file, "original_content");
+ fixture.transferAndCommit(flow_file);
+
+ CHECK(flow_file->getSize() == 16);
+
+ process_session.write(flow_file, [](const
std::shared_ptr<minifi::io::OutputStream>& output_stream) {
+ constexpr std::string str = "new_content";
Review Comment:
Using constexpr with std::string is unnecessary here. The string is not used
for compile-time computation and is only needed at runtime within the lambda.
Consider using a simple string literal, const char*, or std::string_view
instead of constexpr std::string for clarity and to avoid confusion about the
intended usage.
--
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]
