Skip to content
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.

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

AmazonSQS.Receive - Implemented internal Result classes #8

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions Frends.AmazonSQS.Receive/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# Changelog

## [1.1.0] - 2024-11-08
### Fixed
- Fixed dotnotation with Result properties by implementing internal classes for the properties.

## [1.0.0] - 2023-08-11
### Added
- Initial implementation
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public async Task SetUp()
SessionToken = string.Empty,
};

_msg = $"Frends.AmazonSQS.Receive.Tests.SendMessage() test.\nDatetime: {DateTime.Now.ToString("o")}";
_msg = $"Frends.AmazonSQS.Receive.Tests.SendMessage() test.\nDatetime: {DateTime.Now:o}";
await SendTestMessage(_msg);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
using System.Collections.Generic;

namespace Frends.AmazonSQS.Receive.Definitions;

/// <summary>
/// AmazonSQS message class.
/// </summary>
public class Message
{
/// <summary>
/// A map of the attributes requested in ReceiveMessage to their respective values. Supported attributes:
/// ApproximateReceiveCount, ApproximateFirstReceiveTimestamp, MessageDeduplicationId, MessageGroupId, SenderId, SentTimestamp, SequenceNumber.
/// ApproximateFirstReceiveTimestamp and SentTimestamp are each returned as an integer representing the epoch time in milliseconds.
/// </summary>
public Dictionary<string, string> Attributes { get; set; }

/// <summary>
/// The message's contents (not URL-encoded).
/// </summary>
public string Body { get; set; }

/// <summary>
/// An MD5 digest of the non-URL-encoded message body string.
/// </summary>
public string MD5OfBody { get; set; }

/// <summary>
/// An MD5 digest of the non-URL-encoded message attribute string.
/// You can use this attribute to verify that Amazon SQS received the message correctly.
/// Amazon SQS URL-decodes the message before creating the MD5 digest. For information about MD5, see RFC1321.
/// </summary>
public string MD5OfMessageAttributes { get; set; }

/// <summary>
/// A unique identifier for the message. A MessageIdis considered unique across all Amazon Web Services accounts for an extended period of time.
/// </summary>
public string MessageId { get; set; }

/// <summary>
/// An identifier associated with the act of receiving the message. A new receipt handle is returned every time you receive a message.
/// When deleting a message, you provide the last received receipt handle to delete the message.
/// </summary>
public string ReceiptHandle { get; set; }

internal Message(Amazon.SQS.Model.Message message)
{
Attributes = message.Attributes;
Body = message.Body;
MD5OfBody = message.MD5OfBody;
MD5OfMessageAttributes = message.MD5OfMessageAttributes;
MessageId = message.MessageId;
ReceiptHandle = message.ReceiptHandle;
}
Comment on lines +45 to +53
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add defensive programming measures in the constructor.

The current implementation has several potential issues:

  1. No null check for the input message parameter
  2. Direct reference assignment of the Attributes dictionary could allow external modifications
  3. No validation of required properties
     internal Message(Amazon.SQS.Model.Message message)
     {
+        if (message == null)
+            throw new ArgumentNullException(nameof(message));
+
-        Attributes = message.Attributes;
+        Attributes = new Dictionary<string, string>(message.Attributes ?? new Dictionary<string, string>());
         Body = message.Body;
         MD5OfBody = message.MD5OfBody;
         MD5OfMessageAttributes = message.MD5OfMessageAttributes;
         MessageId = message.MessageId;
         ReceiptHandle = message.ReceiptHandle;
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
internal Message(Amazon.SQS.Model.Message message)
{
Attributes = message.Attributes;
Body = message.Body;
MD5OfBody = message.MD5OfBody;
MD5OfMessageAttributes = message.MD5OfMessageAttributes;
MessageId = message.MessageId;
ReceiptHandle = message.ReceiptHandle;
}
internal Message(Amazon.SQS.Model.Message message)
{
if (message == null)
throw new ArgumentNullException(nameof(message));
Attributes = new Dictionary<string, string>(message.Attributes ?? new Dictionary<string, string>());
Body = message.Body;
MD5OfBody = message.MD5OfBody;
MD5OfMessageAttributes = message.MD5OfMessageAttributes;
MessageId = message.MessageId;
ReceiptHandle = message.ReceiptHandle;
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
using System.Collections.Generic;

namespace Frends.AmazonSQS.Receive.Definitions;

/// <summary>
/// Response metadata class.
/// </summary>
public class ResponseMetadata
{
/// <summary>
/// Map of response metadata.
/// </summary>
public IDictionary<string, string> Metadata { get; set; }

/// <summary>
/// ID that uniquely identifies a request. Amazon keeps track of request IDs. If you have a question about a request, include the request ID in your correspondence.
/// </summary>
public string RequestId { get; set; }

internal ResponseMetadata(Amazon.Runtime.ResponseMetadata metadata)
{
Metadata = metadata.Metadata;
RequestId = metadata.RequestId;
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
using Amazon.Runtime;
using Amazon.SQS.Model;
using Amazon.SQS.Model;
using System.Collections.Generic;
using System.Linq;
using System.Net;

namespace Frends.AmazonSQS.Receive.Definitions;
Expand Down Expand Up @@ -38,7 +38,7 @@ internal Result(ReceiveMessageResponse response)
{
ContentLength = response.ContentLength;
StatusCode = response.HttpStatusCode;
Messages = response.Messages;
ResponseMetadata = response.ResponseMetadata;
Messages = response.Messages.Select(e => new Message(e)).ToList();
ResponseMetadata = new ResponseMetadata(response.ResponseMetadata);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

<PropertyGroup>
<TargetFrameworks>net6.0</TargetFrameworks>
<Version>1.0.0</Version>
<Version>1.1.0</Version>
<Authors>Frends</Authors>
<Copyright>Frends</Copyright>
<Company>Frends</Company>
Expand Down
80 changes: 28 additions & 52 deletions Frends.AmazonSQS.Receive/Frends.AmazonSQS.Receive/Receive.cs
Original file line number Diff line number Diff line change
Expand Up @@ -83,19 +83,14 @@ internal static dynamic ConstructAWSCredentials(Connection connection)
if (connection.UseDefaultCredentials)
return null;

switch (connection.CredentialsType)
return connection.CredentialsType switch
{
case AWSCredentialsType.BasicAWSCredentials:
return new BasicAWSCredentials(connection.AccessKey, connection.SecretKey);
case AWSCredentialsType.AnonymousAWSCredentials:
return new AnonymousAWSCredentials();
case AWSCredentialsType.EnvironmentAWSCredentials:
return new EnvironmentVariablesAWSCredentials();
case AWSCredentialsType.SessionAWSCredentials:
return new SessionAWSCredentials(connection.AccessKey, connection.SecretKey, connection.SessionToken);
default:
throw new InvalidEnumArgumentException("Unknown credentials type.");
}
AWSCredentialsType.BasicAWSCredentials => new BasicAWSCredentials(connection.AccessKey, connection.SecretKey),
AWSCredentialsType.AnonymousAWSCredentials => new AnonymousAWSCredentials(),
AWSCredentialsType.EnvironmentAWSCredentials => new EnvironmentVariablesAWSCredentials(),
AWSCredentialsType.SessionAWSCredentials => new SessionAWSCredentials(connection.AccessKey, connection.SecretKey, connection.SessionToken),
_ => throw new InvalidEnumArgumentException("Unknown credentials type."),
};
}

/// <summary>
Expand All @@ -104,46 +99,27 @@ internal static dynamic ConstructAWSCredentials(Connection connection)
[ExcludeFromCodeCoverage]
private static RegionEndpoint RegionSelection(Region region)
{
switch (region)
return region switch
{
case Region.EuNorth1:
return RegionEndpoint.EUNorth1;
case Region.EuWest1:
return RegionEndpoint.EUWest1;
case Region.EuWest2:
return RegionEndpoint.EUWest2;
case Region.EuWest3:
return RegionEndpoint.EUWest3;
case Region.EuCentral1:
return RegionEndpoint.EUCentral1;
case Region.ApSoutheast1:
return RegionEndpoint.APSoutheast1;
case Region.ApSoutheast2:
return RegionEndpoint.APSoutheast2;
case Region.ApNortheast1:
return RegionEndpoint.APNortheast1;
case Region.ApNortheast2:
return RegionEndpoint.APNortheast2;
case Region.ApSouth1:
return RegionEndpoint.APSouth1;
case Region.CaCentral1:
return RegionEndpoint.CACentral1;
case Region.CnNorth1:
return RegionEndpoint.CNNorth1;
case Region.CnNorthWest1:
return RegionEndpoint.CNNorthWest1;
case Region.SaEast1:
return RegionEndpoint.SAEast1;
case Region.UsEast1:
return RegionEndpoint.USEast1;
case Region.UsEast2:
return RegionEndpoint.USEast2;
case Region.UsWest1:
return RegionEndpoint.USWest1;
case Region.UsWest2:
return RegionEndpoint.USWest2;
default:
return RegionEndpoint.EUNorth1;
}
Region.EuNorth1 => RegionEndpoint.EUNorth1,
Region.EuWest1 => RegionEndpoint.EUWest1,
Region.EuWest2 => RegionEndpoint.EUWest2,
Region.EuWest3 => RegionEndpoint.EUWest3,
Region.EuCentral1 => RegionEndpoint.EUCentral1,
Region.ApSoutheast1 => RegionEndpoint.APSoutheast1,
Region.ApSoutheast2 => RegionEndpoint.APSoutheast2,
Region.ApNortheast1 => RegionEndpoint.APNortheast1,
Region.ApNortheast2 => RegionEndpoint.APNortheast2,
Region.ApSouth1 => RegionEndpoint.APSouth1,
Region.CaCentral1 => RegionEndpoint.CACentral1,
Region.CnNorth1 => RegionEndpoint.CNNorth1,
Region.CnNorthWest1 => RegionEndpoint.CNNorthWest1,
Region.SaEast1 => RegionEndpoint.SAEast1,
Region.UsEast1 => RegionEndpoint.USEast1,
Region.UsEast2 => RegionEndpoint.USEast2,
Region.UsWest1 => RegionEndpoint.USWest1,
Region.UsWest2 => RegionEndpoint.USWest2,
_ => RegionEndpoint.EUNorth1,
};
Comment on lines +102 to +123
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider throwing for undefined regions instead of silent fallback.

The default case silently falls back to RegionEndpoint.EUNorth1 which could mask configuration errors. Consider throwing an exception to make region misconfiguration more visible:

        return region switch
        {
            Region.EuNorth1 => RegionEndpoint.EUNorth1,
            // ... other cases ...
            Region.UsWest2 => RegionEndpoint.USWest2,
-           _ => RegionEndpoint.EUNorth1,
+           _ => throw new InvalidEnumArgumentException($"Unsupported region: {region}")
        };

Committable suggestion skipped: line range outside the PR's diff.

}
}
Loading