commit 69290f9695649fad58aab5c48b2132a0a1cd1072
parent b1538828de7ef3af1f3d4ba9659ed3f414df8d85
Author: JohnMcPMS <johnmcp@microsoft.com>
Date: Mon, 11 Jan 2021 16:03:19 -0800
Force output paths to use u8string with filesystem::path (#694)
## Issue
When paths with non-ASCII characters are used, logging them is resulting in an exception due to not being able to convert them to ASCII (the default behavior when forced to convert to a narrow char).
## Change
To better handle all situations of output of a path, both the logging and reporting infrastructure have been enlightened to force the use of `u8String()` when they see one.
Diffstat:
6 files changed, 70 insertions(+), 14 deletions(-)
diff --git a/src/AppInstallerCLICore/ChannelStreams.cpp b/src/AppInstallerCLICore/ChannelStreams.cpp
@@ -85,6 +85,13 @@ namespace AppInstaller::CLI::Execution
return *this;
}
+ OutputStream& OutputStream::operator<<(const std::filesystem::path& path)
+ {
+ ApplyFormat();
+ m_out << path.u8string();
+ return *this;
+ }
+
NoVTStream::NoVTStream(std::ostream& out, bool enabled) :
m_out(out, enabled, false) {}
diff --git a/src/AppInstallerCLICore/ChannelStreams.h b/src/AppInstallerCLICore/ChannelStreams.h
@@ -100,6 +100,7 @@ namespace AppInstaller::CLI::Execution
OutputStream& operator<<(std::ostream& (__cdecl* f)(std::ostream&));
OutputStream& operator<<(const VirtualTerminal::Sequence& sequence);
OutputStream& operator<<(const VirtualTerminal::ConstructedSequence& sequence);
+ OutputStream& operator<<(const std::filesystem::path& path);
private:
// Applies the format for the stream.
diff --git a/src/AppInstallerCLIE2ETests/SourceCommand.cs b/src/AppInstallerCLIE2ETests/SourceCommand.cs
@@ -29,7 +29,7 @@ namespace AppInstallerCLIE2ETests
public void SourceAddWithInvalidURL()
{
// Add source with invalid url should fail
- var result = TestCommon.RunAICLICommand("source add", "AnotherSource https://microsoft.com");
+ var result = TestCommon.RunAICLICommand("source add", $"AnotherSource {Constants.TestSourceUrl}/Invalid/Directory/Dont/Add/Me");
Assert.AreEqual(Constants.ErrorCode.HTTP_E_STATUS_NOT_FOUND, result.ExitCode);
Assert.True(result.StdOut.Contains("An unexpected error occurred while executing the command"));
}
@@ -50,7 +50,7 @@ namespace AppInstallerCLIE2ETests
// List with no args should list all available sources
var result = TestCommon.RunAICLICommand("source list", "");
Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode);
- Assert.True(result.StdOut.Contains("https://localhost:5001/TestKit"));
+ Assert.True(result.StdOut.Contains(Constants.TestSourceUrl));
}
[Test]
@@ -58,8 +58,8 @@ namespace AppInstallerCLIE2ETests
{
var result = TestCommon.RunAICLICommand("source list", $"-n {Constants.TestSourceName}");
Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode);
- Assert.True(result.StdOut.Contains("TestSource"));
- Assert.True(result.StdOut.Contains("https://localhost:5001/TestKit"));
+ Assert.True(result.StdOut.Contains(Constants.TestSourceName));
+ Assert.True(result.StdOut.Contains(Constants.TestSourceUrl));
Assert.True(result.StdOut.Contains("Microsoft.PreIndexed.Package"));
Assert.True(result.StdOut.Contains("Updated"));
}
@@ -110,8 +110,8 @@ namespace AppInstallerCLIE2ETests
{
var result = TestCommon.RunAICLICommand("source reset", "");
Assert.True(result.StdOut.Contains("The following sources will be reset if the --force option is given:"));
- Assert.True(result.StdOut.Contains("TestSource"));
- Assert.True(result.StdOut.Contains("https://localhost:5001/TestKit"));
+ Assert.True(result.StdOut.Contains(Constants.TestSourceName));
+ Assert.True(result.StdOut.Contains(Constants.TestSourceUrl));
}
[Test]
@@ -126,8 +126,8 @@ namespace AppInstallerCLIE2ETests
result = TestCommon.RunAICLICommand("source list", "");
Assert.True(result.StdOut.Contains("winget"));
Assert.True(result.StdOut.Contains("https://winget.azureedge.net/cache"));
- Assert.False(result.StdOut.Contains($"{Constants.TestSourceName}"));
- Assert.False(result.StdOut.Contains($"{Constants.TestSourceUrl}"));
+ Assert.False(result.StdOut.Contains(Constants.TestSourceName));
+ Assert.False(result.StdOut.Contains(Constants.TestSourceUrl));
ResetTestSource();
}
}
diff --git a/src/AppInstallerCLITests/Strings.cpp b/src/AppInstallerCLITests/Strings.cpp
@@ -3,6 +3,7 @@
#include "pch.h"
#include "TestCommon.h"
#include <AppInstallerStrings.h>
+#include <ExecutionReporter.h>
using namespace std::string_view_literals;
using namespace AppInstaller::Utility;
@@ -147,3 +148,19 @@ TEST_CASE("ExpandEnvironmentVariables", "[strings]")
REQUIRE(ExpandEnvironmentVariables(L"%TEMP%") == tempPath);
}
+
+TEST_CASE("PathOutput", "[strings]")
+{
+ std::string original = "\xe6\xb5\x8b\xe8\xaf\x95";
+ std::filesystem::path path = ConvertToUTF16(original);
+ AICLI_LOG(Test, Info, << path);
+
+ std::istringstream in;
+ std::ostringstream out;
+ AppInstaller::CLI::Execution::Reporter reporter{ out, in };
+
+ reporter.Info() << path;
+
+ std::string output = out.str();
+ REQUIRE(output.substr(output.size() - original.size()) == original);
+}
diff --git a/src/AppInstallerCommonCore/Public/AppInstallerLogging.h b/src/AppInstallerCommonCore/Public/AppInstallerLogging.h
@@ -17,7 +17,7 @@
auto& _aicli_log_log = AppInstaller::Logging::Log(); \
if (_aicli_log_log.IsEnabled(_aicli_log_channel, _aicli_log_level)) \
{ \
- std::stringstream _aicli_log_strstr; \
+ AppInstaller::Logging::LoggingStream _aicli_log_strstr; \
_aicli_log_strstr _outstream_; \
_aicli_log_log.Write(_aicli_log_channel, _aicli_log_level, _aicli_log_strstr.str()); \
} \
@@ -140,6 +140,37 @@ namespace AppInstaller::Logging
// Calls the various stream format functions to produce an 8 character hexadecimal output.
std::ostream& SetHRFormat(std::ostream& out);
+
+ // This type allows us to override the default behavior of output operators for logging.
+ struct LoggingStream
+ {
+ // Force use of the UTF-8 string from a file path.
+ // This should not be necessary when we move to C++20 and convert to using u8string.
+ friend AppInstaller::Logging::LoggingStream& operator<<(AppInstaller::Logging::LoggingStream& out, std::filesystem::path& path)
+ {
+ out.m_out << path.u8string();
+ return out;
+ }
+
+ friend AppInstaller::Logging::LoggingStream& operator<<(AppInstaller::Logging::LoggingStream& out, const std::filesystem::path& path)
+ {
+ out.m_out << path.u8string();
+ return out;
+ }
+
+ // Everything else.
+ template <typename T>
+ friend AppInstaller::Logging::LoggingStream& operator<<(AppInstaller::Logging::LoggingStream& out, T&& t)
+ {
+ out.m_out << std::forward<T>(t);
+ return out;
+ }
+
+ std::string str() const { return m_out.str(); }
+
+ private:
+ std::stringstream m_out;
+ };
}
// Enable output of system_clock time_points.
diff --git a/src/AppInstallerCommonCore/Public/winget/LocIndependent.h b/src/AppInstallerCommonCore/Public/winget/LocIndependent.h
@@ -53,12 +53,12 @@ namespace AppInstaller::Utility
bool operator<(const LocIndString& other) const { return m_value < other.m_value; }
+ friend std::ostream& operator<<(std::ostream& out, const AppInstaller::Utility::LocIndString& lis)
+ {
+ return (out << lis.get());
+ }
+
private:
std::string m_value;
};
}
-
-inline std::ostream& operator<<(std::ostream& out, const AppInstaller::Utility::LocIndString& lis)
-{
- return (out << lis.get());
-}
