cagataygurturk / lambadaframework

Build serverless REST API's with JAVA. It implements the JAX-RS API and deploys your application easily to AWS Lambda and API Gateway
MIT License
244 stars 48 forks source link

Post data #19

Open arran4 opened 8 years ago

arran4 commented 8 years ago

Post data doesn't seem to deserialize. It complains of wrong number of arguments with this sort of definition:

@POST
@PATH("/post")
public Response(String body)

And the following simply doesn't work:

@POST
@PATH("/post")
public Response(@FormParam("query1") String query)

Sorry for the code examples; I'm away from my desk atm.

cagataygurturk commented 8 years ago

Seems that serious bug, I'll try to fix it and include to 0.5 release this week. Thanks!

arran4 commented 8 years ago

So no luck I take it?

cagataygurturk commented 8 years ago

I am fixing the issue right now and it'll be released in 0.0.6 in the upcoming days

arran4 commented 8 years ago

Awesome. Thanks!

arranubels commented 8 years ago

What's needed to make this change?

arranubels commented 8 years ago

Okay I got some more details. I get the following error:

java.lang.IllegalArgumentException: wrong number of arguments at 
sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at 
java.lang.reflect.Method.invoke(Method.java:498) at 
org.lambadaframework.runtime.ResourceMethodInvoker.invoke(ResourceMethodInvoker.java:107) at 
org.lambadaframework.runtime.Handler.handleRequest(Handler.java:62) at 
org.lambadaframework.runtime.Handler.handleRequest(Handler.java:13) at 
lambdainternal.EventHandlerLoader$PojoHandlerAsStreamHandler.handleRequest(EventHandlerLoader.java:370) at lambdainternal.EventHandlerLoader$2.call(EventHandlerLoader.java:972) at 
lambdainternal.AWSLambda.startRuntime(AWSLambda.java:231) at lambdainternal.AWSLambda.<clinit>(AWSLambda.java:59) at java.lang.Class.forName0(Native Method) at 
java.lang.Class.forName(Class.java:348) at 
lambdainternal.LambdaRTEntry.main(LambdaRTEntry.java:93)

Regardless of the permutations of configurations I've tried that seems to be the result.

With the following changes to the boilerplate demo: Added to pom dep:

        <dependency>
            <groupId>com.amazonaws</groupId>
            <artifactId>aws-lambda-java-log4j</artifactId>
            <version>1.0.0</version>
        </dependency>

The whole example controller:

@Path("/")
public class ExampleController {
    static final Logger logger = Logger.getLogger(ExampleController.class);

    public static class Input {
        public String value;

        public Input(String value) {
            this.value = value;
        }
    }

    @POST
    @Path("test/test1")
    @Produces(MediaType.APPLICATION_JSON)
    @Consumes(MediaType.APPLICATION_JSON)
    public Response test1(@FormParam("value") Input input) {

        logger.debug("Request got");
        return Response.status(201)
                .entity(input)
                .build();
    }
}

(I've tried a number of different variants here. @QueryParam, no annotations, of String type, various @Consumes just to be sure.

Log4j.properties:

log = .
log4j.rootLogger = DEBUG, LAMBDA

#Define the LAMBDA appender
log4j.appender.LAMBDA=com.amazonaws.services.lambda.runtime.log4j.LambdaAppender
log4j.appender.LAMBDA.layout=org.apache.log4j.PatternLayout
log4j.appender.LAMBDA.layout.conversionPattern=%d{yyyy-MM-dd HH:mm:ss} <%X{AWSRequestId}> %-5p %c{1}:%L - %m%n

My test: curl -X POST -d @testfiles/data.txt https://xxx.execute-api.eu-west-1.amazonaws.com/production/test/test1 --header "Content-Type: APPLICATION/JSON"

And test file:

{ "value": "testtesttest" }

Also, I'm using the latest code in master. (0.0.6)

cagataygurturk commented 8 years ago

There is basically no support for those annotations, that's why it is not working :)

Supported parameter annotations are listed here:

https://github.com/lambadaframework/lambadaframework/blob/master/lambada-maven-plugin/src/main/java/org/lambadaframework/aws/ApiGateway.java#L507

https://github.com/lambadaframework/lambadaframework/blob/bcf18d50ffa33ad3274fe5cdf749cf80e19c76c7/runtime/src/main/java/org/lambadaframework/runtime/ResourceMethodInvoker.java#L66

This two files should be modified, and maybe there should be another little change. I'll be adding this support as soon as I've some time.

DavidBoman commented 8 years ago

I'm also in desperate need for this! @arran4 , @cagataygurturk - should we join forces and try to fix it?

@cagataygurturk - can you elaborate on what you think is needed?

cagataygurturk commented 8 years ago

It's a bit more complicated than I was thinking. API Gateway is designed to be REST-compliant, which means they like to accept POST parameters as JSON body. Still looking for a way to support this feature. Please stay tuned.

As a general practice, if you can avoid post params in your application, I'd recommend to accept request body as JSON instead of form values.

DavidBoman commented 8 years ago

I'm all fine with JSON. The body of my post-data is a pure JSON-snippet. Perhaps @arranubels FormParams are more difficult in that sense. My problem is that I get the "wrong number of arguments" even with a pure JSON-body. Looking in the code mentioned above I can't see anything that performs unmarshalling of the body to a POJO. Do you have something un-commited in the works or is all body-processing missing?

cagataygurturk commented 8 years ago

Hey, please see the latest commits, https://github.com/lambadaframework/lambadaframework/compare/f0b41bf7dc86cd9ba17682b0d2b74f8a338352c5...master

It was very quick fix and I did not write any test.

  1. Clone the repo
  2. mvn install -DskipTests to install the development version to your local repository
  3. Change lambada version to 0.0.6 (which is the next release), maven will use the development version from your repo
  4. In your controller use post data like this:
    @POST
    @Consumes(MediaType.APPLICATION_JSON)
    @Path("/resource")
    public Response exampleSecondEndpointPost(
            String requestBody
    ) {

        logger.debug("Request got");
        return Response.status(201)
                .entity(new Entity(requestBody))
                .build();
    }

Consumes annotation is important and parameter type should be String.

I was also planning to add automatic deserialisation of JSON but to do that I've to add Jackson (or other JSON library) to runtime dependencies which would increase JAR file size.

I'll look if Jersey is supporting automatic deserialisation because we've to be full jersey-compliant.

If this feature is working for you guys I'll include it to the next release which will happen this weekend.

arranubels commented 8 years ago

Ok. I have closed my old PR.. Creating a new one with some new changes with regards how post works. I don't understand the Api Gateway side enough to make changes to allow actual url encoded post data to be passed through. My changes should work fine once those changes have been made. I have used extensive use of the jacksons objectmapper.

Unfortunately I have formatted the code slightly differently to how it currently has.. So sorry for the large white space changes.

I will put the PR through in the next couple hours.

arranubels commented 8 years ago

Done. Let me know what I need to change.

cagataygurturk commented 8 years ago

Please see it: https://forums.aws.amazon.com/message.jspa?messageID=668814#668814 As i said it is a bit tricky. Let's see if we can add @FormData as well.

cagataygurturk commented 8 years ago

Be aware of https://github.com/lambadaframework/lambadaframework/commit/7340ae4b4a9b9d472be73e54743a07294c48e7e7 which adds automatic deserialization for post body.

arranubels commented 8 years ago

I'll rebase and create a new PR.

arranubels commented 8 years ago

Are you going to consider using Jackson's ObjectMapper's ReadValue function for a substitute to the toObject function you're currently using, as we have it as a dependency anyway?

cagataygurturk commented 8 years ago

I think it's good option to use that method, instead of our "bad" toObject method. Thanks god Jackson comes as a dependency in Lambda runtime.

arranubels commented 8 years ago

I haven't completed eliminated it in my PR. But removing it should help remove most of the explicit typecasts..