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

Split out SimpleEmailEventsReceiptAction into different action types #496

Merged
merged 3 commits into from
Oct 24, 2019
Merged

Split out SimpleEmailEventsReceiptAction into different action types #496

merged 3 commits into from
Oct 24, 2019

Conversation

craigbrett17
Copy link
Contributor

Description of changes:

We were trying to use this library to allow us to have a lambda handle emails being received with an S3 action. However, the library as was only supports Lambda. So we're currently using dynamic to get around this limitation, but I'd rather be type safe.

So, what I've done, is split the SimpleEmailReceiptAction into an interface and 2 different action types. This type can then be passed in as a <T> at any level of the nested classes, and will resolve properly. Unit tests confirm.

It would help us be type safe were this request to be confirmed.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Craig Brett added 2 commits July 25, 2019 13:53
Refactored SimpleEmailEvents to take in a type parameter to say what type of action is expected

Updated tests and test data accordingly
Copy link
Member

@normj normj left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I would like to include the PR but we need to find a way that isn't a breaking change,

One option would be to have a non generic version of SimpleEmailEvent that extends from SimpleEmailEvent<LambdaReceiptAction>. Something like this.

public interface IReceiptAction
{
    string Type { get; set; }
}

public class LambdaReceiptAction : IReceiptAction
{
    public string Type { get; set; }
}

public class S3ReceiptAction : IReceiptAction
{
    public string Type { get; set; }
}

public class SimpleEmailEvent<TReceiptAction> where TReceiptAction : IReceiptAction
{

}

public class SimpleEmailEvent : SimpleEmailEvent<LambdaReceiptAction>
{

}


class Program
{
    static void Main(string[] args)
    {
        var nonBreaking = new SimpleEmailEvent();
        var lambdaBreaking = new SimpleEmailEvent<LambdaReceiptAction>();
        var s3Event = new SimpleEmailEvent<S3ReceiptAction>();
    }
}

We could mark the non generic SimpleEmailEvent as Obsolete with a message explaining why there is a non generic and that they should use the generic version.

These are set to just be the lambda receipts which it was before this update,
with obsolete attributes to hopefully get people to move off onto less
presumptive types
@craigbrett17
Copy link
Contributor Author

@normj: Alright, I've gone with what you suggested and implemented some lambda by default backwards compatible types. Hopefully the obsolete message will be enough to encourage people to make the hopefully painless change over. 🤞

@normj
Copy link
Member

normj commented Sep 25, 2019

I have merged this into the dev branch to go out with the next release.

@normj normj merged commit 1396c2a into aws:master Oct 24, 2019
@normj
Copy link
Member

normj commented Oct 25, 2019

Version 2.0.0 of Amazon.Lambda.SimpleEmailEvents has been released with this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants