chore(lambda-event-sources): refactoring the filters type

[marciocadev]

Description

Changing filters type from Array<{[key: string]: any}> to Array<FilterCriteria>.

This way it will be easier for the developer to understand what type of parameter should be passed

---

All Submissions:

• Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

• This PR adds new unconventional dependencies following the process described here

New Features

• Have you added the new feature to an integration test?
  • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

[gitpod-io]

Pull request has been modified.

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Pull request has been modified.

[marciocadev]

Hi @TheRealAmazonKendra

I had to update the branch last time because somehow PR Linter broke, you can check if it's good

[kaizencc]

Hi @marciocadev! I know this PR was preivously approved but I have some concerns about the behavior of the property you are deprecating. We need to make sure that filters continues to work even though its deprecated.

[marciocadev]

@kaizencc

Your comments are perfect
I will make the changes to make sure this new implementation doesn't breaks what was done previously

Pull request has been modified.

[kaizencc]

@marciocadev, refactoring is tough when we have a stable api! I think what we had before with deprecating filter and not using it internally (so we don't have to use testDeprecated) is the best way to go here.

Pull request has been modified.

[kaizencc]

@marciocadev this is still not okay. While the API is no longer breaking, the behavior is. We still need the following to work as usual:

filters: [
  FilterCriteria.filter({
    numericEquals: FilterRule.isEqual(1),
  }),
]

In this case, the resulting object includes the pattern key. We then try to send this into FilterCriteria.addFilter(filter).toPattern(), which will add another pattern key to the resulting object. This duplication is unexpected, and wrong.

The only path forward I see is a deprecation of the filters property. We cannot successfully support two different ways of providing an object to filters in a maintainable way. I expressly don't want to have any logic that checks to see if pattern exists before adding it (to avoid the duplication you've got going). We must deprecate filters and add a new property with the type you want.

Please note that I would like you to keep at least 1 test that tests the legacy behavior to make sure that it has not changed.

[marciocadev]

@kaizencc I don't undestand, if I deprecate filters a lot of test have to be changed to testDeprecated

The initial problem is that the filters parameter is of type {[key:string]: any} but needs to receive a FilterCriteria parameter

I don't know if I need to deprecate filters and create a new variable or change filters type

[kaizencc]

Hi @marciocadev:

The initial problem is that the filters parameter is of type {[key:string]: any} but needs to receive a FilterCriteria parameter

That isn't necessarily true. FilterCriteria is still a {[key: string]: any}, so the developer doesn't really get anything out of the type being Array<FilterCriteria>. AFAIK, the current user experience is:

new EventSourceMapping(stack, 'test', {
        target: fn,
        eventSourceArn: eventSourceArn,
        kafkaTopic: topicNameParam.valueAsString,
        filters: [
          FilterCriteria.filter({
            numericEquals: FilterRule.isEqual(1),
          }),
        ],
      });

And it's weird because the user doesn't know to supply a FilterCriteria.filter. However, that isn't necessary. The user could simply do:

new EventSourceMapping(stack, 'test', {
        target: fn,
        eventSourceArn: eventSourceArn,
        kafkaTopic: topicNameParam.valueAsString,
        filters: [
          pattern: {
            numericEquals: FilterRule.isEqual(1),
          }),
        ],
      });

Which is perfectly fine and should work. The alternative you've suggested doesn't really help. In fact, I think it will confuse users because the only difference is whether you supply pattern or not:

new EventSourceMapping(stack, 'test', {
        target: fn,
        eventSourceArn: eventSourceArn,
        kafkaTopic: topicNameParam.valueAsString,
        filters: [
          {
            numericEquals: FilterRule.isEqual(1),
          },
        ],
      });

Am I missing something here? What was the original motivation behind having a FilterCriteria.filter? To my eye, it doesn't seem helpful.

[marciocadev]

Hi @kaizencc

My motivation was to make it clearer that the ideal was to use FilterCriteria to generate the pattern since the type that filters receives is {[key: string]: any}

I think it's complicated to use it as it is today

But if you think the way it is today is enough, I can close this PR

Pull request has been modified.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 0bb519d
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[kaizencc]

As it stands, I don't think FilterCriteria.filter adds much. All it does is abstract out the pattern keyword. Ideally, we'd type filters better than {[key: string]: any} as has been discussed on the accompanying issue. If we can figure that out, then I think the path forward is to deprecate this filters property for a more strongly typed property. I'm going to close this PR in favor of that path. If you're interested in working on that, please go ahead! Let's talk design on a new issue and land on a good proposal.