micronaut-aws icon indicating copy to clipboard operation
micronaut-aws copied to clipboard

Move away from Jackson

Open timyates opened this issue 3 years ago • 16 comments
trafficstars

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

timyates avatar Mar 29 '22 16:03 timyates

@yawkat can you provide feedback?

sdelamo avatar Mar 30 '22 05:03 sdelamo

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:

image

timyates avatar Mar 30 '22 10:03 timyates

@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

timyates avatar Mar 30 '22 11:03 timyates

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.

alvarosanchez avatar Mar 30 '22 12:03 alvarosanchez

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

timyates avatar Mar 30 '22 12:03 timyates

So here https://github.com/micronaut-projects/micronaut-aws/blob/dca8a6cfe7a2325aac742034881d61d6cec64b56/aws-alexa-httpserver/src/test/groovy/io/micronaut/aws/alexa/httpserver/controllers/SkillControllerPathSpec.groovy#L42-L45

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 avatar Mar 30 '22 13:03 timyates

@graemerocher @yawkat I'm not sure how to use Serde to handle RequestEnvelope from AWS (see https://github.com/micronaut-projects/micronaut-aws/pull/1323#issuecomment-1083124887 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?

timyates avatar Mar 30 '22 15:03 timyates

@timyates i would use a JsonCreator in a mixin

yawkat avatar Mar 30 '22 15:03 yawkat

@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 😀

timyates avatar Mar 30 '22 16:03 timyates

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

yawkat avatar Mar 30 '22 16:03 yawkat

A custom deserializer is probably the simplest to be honest

graemerocher avatar Mar 30 '22 20:03 graemerocher

@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

yawkat avatar Mar 30 '22 20:03 yawkat

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

graemerocher avatar Mar 31 '22 07:03 graemerocher

Currently blocked by https://github.com/micronaut-projects/micronaut-serialization/issues/176

timyates avatar Apr 01 '22 08:04 timyates

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

lesteenman avatar Mar 09 '23 21:03 lesteenman