Fix correct parsing of collections in AmazonLambdaRecorder #40464
Conversation
This comment has been minimized.
This comment has been minimized.
...azon-lambda/runtime/src/main/java/io/quarkus/amazon/lambda/runtime/AmazonLambdaRecorder.java
Outdated
Show resolved
Hide resolved
...azon-lambda/runtime/src/main/java/io/quarkus/amazon/lambda/runtime/AmazonLambdaRecorder.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
hamburml
force-pushed
the
feature/AmazonLambdaHandlerCollections
branch
4 times, most recently
from
46ee579
to
ecfbe08
Compare
...da/deployment/src/test/java/io/quarkus/amazon/lambda/deployment/testing/LambdaHandlerET.java
Outdated
Show resolved
Hide resolved
hamburml
force-pushed
the
feature/AmazonLambdaHandlerCollections
branch
from
ecfbe08
to
0886509
Compare
gsmet
force-pushed
the
feature/AmazonLambdaHandlerCollections
branch
from
0886509
to
06d24d4
Compare
Status for workflow
|
|
@patriot1burke are you happy with it? If so, we can merge. |
|
@gsmet My reviews have not been resolved yet. |
|
@patriot1burke which reviews? I don't see any in this PR? Could you point me to where your comments are? |
|
Maybe the review was not posted by accident (the GH UI tends to make this extremely easy to happen)? |
|
Maybe he means this comment in the linked Issue #40349 (comment) |
| } | ||
| objectWriter = new JacksonOutputWriter(objectMapper.writerFor(handlerMethod.getReturnType())); | ||
| objectWriter = new JacksonOutputWriter(objectMapper.writerFor(javaType)); |
There was a problem hiding this comment.
You must get the generic return type for the object writer. You cannot reuse the method parameter for the output.
There was a problem hiding this comment.
You'll have to handle this case for this PR to be approved:
public class GenericTypeHandler implements RequestHandler<List<String>, List<Food>> {
public List<String> handleRequest(List<Food> food) {
...
}
This case is a little harder:
public abstract class AbstractHandler implements RequestHandler<R,T> {
public R handleRequest(T input) {
...
}
}
public class ConcreteHandler extends AbstractHandler<String, String> {
}
| public class PersonListLambda implements RequestHandler<List<Person>, String> { | ||
|
|
||
| @Override | ||
| public String handleRequest(List<Person> people, Context context) { |
There was a problem hiding this comment.
Should test returning a collection as well.
| if (parameterType.equals(S3Event.class)) { | ||
| objectReader = new S3EventInputReader(objectMapper); | ||
| } else if (Collection.class.isAssignableFrom(parameterType)) { | ||
| objectReader = new CollectionInputReader<>(objectMapper, handlerMethod); |
There was a problem hiding this comment.
You don't need a separate class just to abstract out 3 lines of code. Gmet is just asking to put the generic setup in an if-then block.
| } else { | ||
| objectReader = new JacksonInputReader(objectMapper.readerFor(parameterType)); | ||
| } | ||
|
|
||
| objectWriter = new JacksonOutputWriter(objectMapper.writerFor(handlerMethod.getReturnType())); |
There was a problem hiding this comment.
You are still ignoring the generic return type.
That is one of the comments. This PR is incomplete without supporting generic return type. |
|
@patriot1burke Now your comments are visible. Maybe tomorrow I will find some time. Not sure. Thanks. |
test to make sure that the objectMapper parses the Input correct.
@patriot1burke thx for the hint.
I also adjusted the objectWriter but I am not sure if this is really necessary.