feat: Scan classes that do not implement an interface#25
Conversation
| * Returns true if the class has the word "handler" in the name. | ||
| */ | ||
| private boolean hasHandlerInClassName(XClass xClass) { | ||
| return xClass.toString().contains("Handler") || xClass.toString().contains("handler"); |
There was a problem hiding this comment.
I'm worried that this might increase the noise significantly. Isn't there any other way to detect whether a class is auto-wired to a Lambda function?
There was a problem hiding this comment.
It's not really possible to do that. Basically customers can't use almost any class and method as a handler for a Lambda function (A very minimal example and a broader description in the issue related to this PR: #24). So we're following the most typical naming convention to catch more cases that we're not checking yet. And the condition here is a class name AND a specific method name, so hopefully that's enough of a restriction.
The main problem here is that customers might not be using any of these as handlers, but they could, and there's no way of knowing that. (It's just a different handler in their function configuration and they start using it without changing anything in their code base)
There was a problem hiding this comment.
I would just keep Handler in upper case only though, considering that it's a class name, you would expect CamelCasing, so "Handler" should start with a capital H.
There was a problem hiding this comment.
Got it, thanks. Updated.
mbfreder
force-pushed
the
enhancement
branch
from
6ee574b to
6987589
Compare
valerena
left a comment
valerena
left a comment
There was a problem hiding this comment.
Let's use a somewhat standard format for commit messages, using conventional commits, starting with a prefix feat: for new features, and trying to not make it too long (there are many places with guidance around this, but for example here).
In this case, it should be something more like
feat: Scan classes that do not implement an interface
| /** | ||
| * Returns true if the class has a method called "handleRequest" | ||
| */ | ||
| private boolean hasMethodCalledHandleRequest(XClass xClass) { |
There was a problem hiding this comment.
Very minor, but I would prefer this to be called hasHandleRequestMethod
There was a problem hiding this comment.
You didn't update this naming yet. We can discuss if you prefer to not change the name.
There was a problem hiding this comment.
You're right. I first thought it might be confusing since we already have a hasLambdaHandlerMethod method. That's why I tried to make it more explicit and remove any confusion by naming it the way I did. But I think the method's comment is clear enough in this regard, so hasHandleRequestMethod makes more sense.
mbfreder
changed the title
| /** | ||
| * Returns true if the class has a method called "handleRequest" | ||
| */ | ||
| private boolean hasMethodCalledHandleRequest(XClass xClass) { |
There was a problem hiding this comment.
You didn't update this naming yet. We can discuss if you prefer to not change the name.
| } | ||
|
|
||
| /** | ||
| * Returns true if the class has the word "handler" in the name. |
There was a problem hiding this comment.
Another minor thing, but in the comment specify "Handler" with capital H
mbfreder
force-pushed
the
enhancement
branch
from
6987589 to
9c9b727
Compare
3 participants
Issue #, if available: Issue #24
Description of changes:
As explained here, these changes allow our rules to visit classes that do not implement an interface or do not have a method that receives either (Map, com.amazonaws.services.lambda.runtime.Context) or (InputStream, OutputStream, com.amazonaws.services.lambda.runtime.Context) as parameters, but are still valid lambda handler classes.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.