Zd#144472

[spbsoluble]

No description provided.

[Copilot]

Pull Request Overview

This PR addresses a bug where invalid FileTransferProtocol values could result in empty certificate store inventory. The fix implements a fallback mechanism that defaults to 'Both' when invalid or problematic values are detected, and adds comprehensive logging and warning capabilities to track these scenarios.

• Implements a new PropertyUtilities class to detect flag combination parsing issues in enum values
• Adds fallback logic to default FileTransferProtocol to 'Both' when invalid values are detected
• Introduces a warnings system to capture and report configuration issues through job results

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
SSHHandler.cs	Adds retry logic and warnings collection for failed download attempts
IRemoteHandler.cs	Adds Warnings property to interface definition
BaseRemoteHandler.cs	Implements Warnings property with empty array initialization
RemoteFileJobTypeBase.cs	Enhanced logging and flag combination detection for FileTransferProtocol
RemoteCertificateStore.cs	Adds debug logging for certificate addition parameters
PropertyUtilities.cs	New utility class for detecting enum flag combination parsing issues
InventoryBase.cs	Integrates warnings into job results with Warning status
PEMCertificateStoreSerializer.cs	Adds debug logging for custom properties
ApplicationSettings.cs	Implements flag combination detection and fallback logic
Test files	Unit tests for new PropertyUtilities functionality
Documentation	Updates README and CHANGELOG with Store Password descriptions and bug fix notes

[Copilot]

The indentation is inconsistent with the surrounding code. This line should be aligned with the preceding lines in the same block.

Suggested change

	Warnings = Warnings.Length == 0 ? new[] { warningMsg } : Warnings.Append(warningMsg).ToArray();
	Warnings = Warnings.Length == 0 ? new[] { warningMsg } : Warnings.Append(warningMsg).ToArray();

[Copilot]

This condition will trigger a recursive call if neither SCP nor SFTP protocols are attempted, but there's no guard against infinite recursion. If the FileTransferProtocol enum remains in an invalid state after being set to 'Both', this could cause a stack overflow.

Suggested change

	FileTransferProtocol = ApplicationSettings.FileTransferProtocolEnum.Both;
	var warningMsg = "No download attempted. Setting FileTransferProtocol to 'Both' and retrying download.";
	_logger.LogWarning(warningMsg);
	// append to Warnings global array
	Warnings = Warnings.Length == 0 ? new[] { warningMsg } : Warnings.Append(warningMsg).ToArray();
	return DownloadCertificateFile(path);
	if (retryCount >= 3)
	{
	var errorMsg = "Retry limit reached. Unable to download the file.";
	_logger.LogError(errorMsg);
	throw new RemoteFileException(errorMsg);
	}
	FileTransferProtocol = ApplicationSettings.FileTransferProtocolEnum.Both;
	var warningMsg = "No download attempted. Setting FileTransferProtocol to 'Both' and retrying download.";
	_logger.LogWarning(warningMsg);
	// append to Warnings global array
	Warnings = Warnings.Length == 0 ? new[] { warningMsg } : Warnings.Append(warningMsg).ToArray();
	return DownloadCertificateFile(path, retryCount + 1);

[Copilot]

[nitpick] Use default instead of default(T) for better readability and consistency with modern C# conventions.

Suggested change

	result = default(T);
	result = default;

[Copilot]

String concatenation in a loop using Aggregate creates multiple intermediate string objects. Consider using string.Join(Environment.NewLine, certificateStore.RemoteHandler.Warnings) for better performance.

Suggested change

	jobMsg = certificateStore.RemoteHandler.Warnings.Aggregate(jobMsg, (current, warning) => current + (warning + Environment.NewLine));
	jobMsg = string.Join(Environment.NewLine, certificateStore.RemoteHandler.Warnings);

[Copilot]

[nitpick] The variable name protocolValue could be more descriptive. Consider renaming it to configProtocolValue to clearly indicate it comes from configuration.

Suggested change

	string? protocolValue = configuration["FileTransferProtocol"];
	if (!Enum.TryParse(configuration["FileTransferProtocol"], out FileTransferProtocolEnum protocol))
	throw new RemoteFileException($"Invalid optional config.json FileTransferProtocol option of {configuration["FileTransferProtocol"]}. If present, must be one of these values: {protocolNames}.");
	if (!PropertyUtilities.TryEnumParse(protocolValue, out bool isFlagCombination, out FileTransferProtocolEnum protocol))
	throw new RemoteFileException($"Invalid optional config.json FileTransferProtocol option of {protocolValue}. If present, must be one of these values: {protocolNames}.");
	// Issue: If received a comma-delimited list ("SCP,SFTP,Both"), it's treating it as a flag combination (i.e. mapping it to 0+1+2=3)
	// If this happens, we want to default it to Both so it's resolved as a valid mapping.
	if (isFlagCombination)
	{
	logger.LogWarning($"FileTransferProtocol config value {protocolValue} mapped to a flag combination. Setting FileTransferProtocol explicitly to Both.");
	string? configProtocolValue = configuration["FileTransferProtocol"];
	if (!PropertyUtilities.TryEnumParse(configProtocolValue, out bool isFlagCombination, out FileTransferProtocolEnum protocol))
	throw new RemoteFileException($"Invalid optional config.json FileTransferProtocol option of {configProtocolValue}. If present, must be one of these values: {protocolNames}.");
	// Issue: If received a comma-delimited list ("SCP,SFTP,Both"), it's treating it as a flag combination (i.e. mapping it to 0+1+2=3)
	// If this happens, we want to default it to Both so it's resolved as a valid mapping.
	if (isFlagCombination)
	{
	logger.LogWarning($"FileTransferProtocol config value {configProtocolValue} mapped to a flag combination. Setting FileTransferProtocol explicitly to Both.");