Move away from Jackson #1323

timyates · 2022-03-29T16:32:16Z

Started with function-aws-api-proxy to gather feedback

Is this going in the right direction?
Are request bodies always Maps in Lambdas? I couldn't find a way to parse a JsonNode here
I had to @SerdeImport` quite a few AWS classes, but I'm not sure our coverage has shown them all
I had to write my own replacements for ObjectWriter and ObjectReader

When I look at a scan for running the tests, I still see a LOT of Jackson, so I'm not sure if I'm doing the right thing

https://ge.micronaut.io/s/s7dmxjatvn6c4/dependencies?dependencies=fasterxml&expandAll

And direction

sdelamo

This will be a breaking change. We have to do it for next mayor of AWS.

sdelamo · 2022-03-30T04:45:32Z

function-aws-api-proxy/build.gradle

@@ -4,12 +4,15 @@ plugins {

 dependencies {
    annotationProcessor libs.micronaut.graal
+    annotationProcessor("io.micronaut.serde:micronaut-serde-processor:1.0.1")


I would recommend to update the build to 3.4.0, githubCoreBranch to 3.5.0 and use the serde version in the BOM.

sdelamo · 2022-03-30T05:05:57Z

...-api-proxy/src/main/java/io/micronaut/function/aws/proxy/AbstractLambdaContainerHandler.java

@@ -119,7 +117,7 @@ protected AbstractLambdaContainerHandler(Class<RequestType> requestClass,
     *
     * @return Return the object mapper.
     */
-    protected abstract ObjectMapper objectMapper();
+    protected abstract JsonMapper objectMapper();


There is a import io.micronaut.serde.ObjectMapper;. Why not return it instead?

sdelamo · 2022-03-30T05:07:14Z

...api-proxy/src/main/java/io/micronaut/function/aws/proxy/MicronautLambdaContainerContext.java

     */
-    default ObjectMapper getObjectMapper() {
-        return getJsonCodec().getObjectMapper();
+    default JsonMapper getObjectMapper() {


There is a import io.micronaut.serde.ObjectMapper; why not use instead of JsonMapper?

JsonCodec returns a JsonMapper, which isn't an ObjectMapper 😔

sdelamo · 2022-03-30T05:08:00Z

...api-proxy/src/main/java/io/micronaut/function/aws/proxy/MicronautLambdaContainerHandler.java

@@ -202,18 +220,18 @@ public ApplicationContext getApplicationContext() {
    }

    @Override
-    protected ObjectMapper objectMapper() {
+    protected JsonMapper objectMapper() {


why not use import io.micronaut.serde.ObjectMapper instead?

sdelamo · 2022-03-30T05:09:48Z

@yawkat can you provide feedback?

…t artifacts

timyates · 2022-03-30T09:04:17Z

...api-proxy/src/main/java/io/micronaut/function/aws/proxy/MicronautLambdaContainerHandler.java

+                                final Map<?, ?> node = lambdaContainerEnvironment.getJsonCodec().decode(Map.class, body.get());
+                                ((MicronautAwsProxyRequest) containerRequest).setDecodedBody(node);


We can't decode to a serde JsonNode here (it fails to create one), so I fell back to a Map. This will break if the body is not a map (ie: a simple json string), but I'm not sure if Lambdas can receive simple strings

timyates · 2022-03-30T10:29:31Z

The HateOas model doesn't seem to be coming through correctly...

{"message":"Internal Server Error","_links":{"self":[{"href":"https://null.execute-api.us-east-1.amazonaws.com/filter-error-spec-3","templated":false}]},"_embedded":{"errors":[{}]}}

Notice the errors are not serialized with any properties...

I've tried @SerdeImport(JsonError.class) but it has no effect on the result

The model should look like this:

timyates · 2022-03-30T11:33:35Z

@alvarosanchez Any idea how we migrate this to Serde serialization? The tests are checking for Jackson features which I don't believe we support in Serde

alvarosanchez · 2022-03-30T12:30:42Z

This was the original reason for that code.

If you are removing Jackson from here, then I don't think that feature is needed at all. Note that this change requires a new major version of the module.

timyates · 2022-03-30T12:50:40Z

@alvarosanchez Thanks! We're well into new major version territory already, so that should be fine 👍

timyates · 2022-03-30T13:13:12Z

So here

micronaut-aws/aws-alexa-httpserver/src/test/groovy/io/micronaut/aws/alexa/httpserver/controllers/SkillControllerPathSpec.groovy

Lines 42 to 45 in dca8a6c

    
           RequestEnvelope re = launchRequestEnvelop(skillId) 
        
           HttpRequest request = HttpRequest.POST("/computer", re) 
        
           HttpResponse<ResponseEnvelope> response = client.exchange(request, ResponseEnvelope)

We send an AWS RequestEnvelope(link), and expect an AWS ResponseEnvelope in return.

Serde fails to deserialize the request model as No default constructor exists... I'm going to look into a custom deserializer

timyates · 2022-03-30T15:20:19Z

@graemerocher @yawkat I'm not sure how to use Serde to handle RequestEnvelope from AWS (see #1323 (comment) above)

The class iteself and a lot of it's member classes use

@JsonDeserialize(builder = RequestEnvelope.Builder.class)

Which we don't support in Serde 🤔. Anyone got any ideas about getting round this?

yawkat · 2022-03-30T15:34:15Z

@timyates i would use a JsonCreator in a mixin

timyates · 2022-03-30T16:17:46Z

@yawkat Like this?

public class RequestEnvelopeMixin {

    @JsonCreator
    public static RequestEnvelope factory(
            @JsonProperty("version") String version,
            @JsonProperty("session") Session session,
            @JsonProperty("context") Context context,
            @JsonProperty("request") Request request
    ) {
        return RequestEnvelope.builder()
                .withVersion(version)
                .withSession(session)
                .withContext(context)
                .withRequest(request)
                .build();
    }
}

I guess I need to mixin the whole tree of types before it will start working 😀

yawkat · 2022-03-30T16:19:26Z

sure something like that. But yea looking at the other classes, this seems infeasible

graemerocher · 2022-03-30T20:35:56Z

A custom deserializer is probably the simplest to be honest

yawkat · 2022-03-30T20:37:21Z

@graemerocher the classes only get bigger, see eg https://github.com/alexa/alexa-apis-for-java/blob/master/ask-sdk-model/src/com/amazon/ask/model/Context.java

graemerocher · 2022-03-31T07:10:21Z

seems like we are going to have to add support for builder=

sonarcloud · 2022-03-31T08:54:19Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

66.7% Coverage
0.0% Duplication

timyates · 2022-04-01T08:03:49Z

Currently blocked by micronaut-projects/micronaut-serialization#176

lesteenman · 2023-03-09T21:09:00Z

The blocking issue seems closed; Any intentions to continue work on this?

timyates added 7 commits March 29, 2022 12:26

Settings to 5.3.1 and update build

f8d1ac3

Remove BOM from catalog as it fails bomcheck

fadb85b

Use version from catalog for boms

0fc14d6

Cleanup gradle.properties

d22a685

More metaspace for Sonar

8e6df2c

Up to 5.3.2 and fix boms accordingly

f791a7c

Start with function-aws-api-proxy to gather feedback

46ad900

And direction

timyates requested review from sdelamo and graemerocher March 29, 2022 16:32

timyates changed the title ~~Start with function-aws-api-proxy to gather feedback~~ Move away from Jackson Mar 29, 2022

timyates added the status: awaiting feedback label Mar 29, 2022

Base automatically changed from update-build to master March 30, 2022 04:54

Merge branch 'master' into remove-jackson

0233b83

sdelamo requested changes Mar 30, 2022

View reviewed changes

sdelamo requested a review from yawkat March 30, 2022 05:09

sdelamo added the status: next major version The issue will be considered for the next major version label Mar 30, 2022

sdelamo added this to the 4.0.0 milestone Mar 30, 2022

timyates added 2 commits March 30, 2022 09:42

Use processor, import classes to abstract runtime, try to fix tests

0d4ca45

inject java annotation processor so serde processor picks up java tes…

3f00ec0

…t artifacts

timyates commented Mar 30, 2022

View reviewed changes

timyates added 2 commits March 30, 2022 10:17

Ignore failing validation test for now

6446fc0

Document JAPICMP violations

02afcb5

Reinstate ignored test now we know the problem

8089f30

More updates

dca8a6c

Unserialized errors and RequestEnvelope deserialization remain

ce9dcbf

Remove custom ObjectMapper and docs for it

cb73990

timyates added 2 commits March 30, 2022 21:01

Leave Jackson in AWS Alexa module

6fecc10

Undo whitespace change

df8d67c

timyates added 2 commits March 31, 2022 09:45

Merge branch 'master' into remove-jackson

599267d

Fix merge

745a03d

timyates added status: awaiting third-party Awaiting changes to a third party library relates-to: jackson labels Apr 1, 2022

timyates marked this pull request as ready for review April 1, 2022 08:05

timyates mentioned this pull request Nov 2, 2022

Use Serde for aws-alexa-httpserver #1498

Open

sdelamo assigned timyates Mar 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move away from Jackson #1323

Move away from Jackson #1323

timyates commented Mar 29, 2022 •

edited by sdelamo

Loading

sdelamo left a comment

sdelamo Mar 30, 2022

sdelamo Mar 30, 2022

sdelamo Mar 30, 2022

timyates Mar 30, 2022

sdelamo Mar 30, 2022

sdelamo commented Mar 30, 2022

timyates Mar 30, 2022

timyates commented Mar 30, 2022

timyates commented Mar 30, 2022

alvarosanchez commented Mar 30, 2022

timyates commented Mar 30, 2022

timyates commented Mar 30, 2022

timyates commented Mar 30, 2022

yawkat commented Mar 30, 2022

timyates commented Mar 30, 2022

yawkat commented Mar 30, 2022

graemerocher commented Mar 30, 2022

yawkat commented Mar 30, 2022

graemerocher commented Mar 31, 2022

sonarcloud bot commented Mar 31, 2022

timyates commented Apr 1, 2022

lesteenman commented Mar 9, 2023

		final Map<?, ?> node = lambdaContainerEnvironment.getJsonCodec().decode(Map.class, body.get());
		((MicronautAwsProxyRequest) containerRequest).setDecodedBody(node);

Move away from Jackson #1323

Are you sure you want to change the base?

Move away from Jackson #1323

Conversation

timyates commented Mar 29, 2022 • edited by sdelamo Loading

sdelamo left a comment

Choose a reason for hiding this comment

sdelamo Mar 30, 2022

Choose a reason for hiding this comment

sdelamo Mar 30, 2022

Choose a reason for hiding this comment

sdelamo Mar 30, 2022

Choose a reason for hiding this comment

timyates Mar 30, 2022

Choose a reason for hiding this comment

sdelamo Mar 30, 2022

Choose a reason for hiding this comment

sdelamo commented Mar 30, 2022

timyates Mar 30, 2022

Choose a reason for hiding this comment

timyates commented Mar 30, 2022

timyates commented Mar 30, 2022

alvarosanchez commented Mar 30, 2022

timyates commented Mar 30, 2022

timyates commented Mar 30, 2022

timyates commented Mar 30, 2022

yawkat commented Mar 30, 2022

timyates commented Mar 30, 2022

yawkat commented Mar 30, 2022

graemerocher commented Mar 30, 2022

yawkat commented Mar 30, 2022

graemerocher commented Mar 31, 2022

sonarcloud bot commented Mar 31, 2022

timyates commented Apr 1, 2022

lesteenman commented Mar 9, 2023

timyates commented Mar 29, 2022 •

edited by sdelamo

Loading