Skip to content

Conversation

timyates
Copy link
Contributor

@timyates timyates commented Mar 29, 2022

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 timyates changed the title Start with function-aws-api-proxy to gather feedback Move away from Jackson Mar 29, 2022
Base automatically changed from update-build to master March 30, 2022 04:54
Copy link
Contributor

@sdelamo sdelamo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* @return Return the object mapper.
*/
protected abstract ObjectMapper objectMapper();
protected abstract JsonMapper objectMapper();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*/
default ObjectMapper getObjectMapper() {
return getJsonCodec().getObjectMapper();
default JsonMapper getObjectMapper() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


@Override
protected ObjectMapper objectMapper() {
protected JsonMapper objectMapper() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sdelamo sdelamo requested a review from yawkat March 30, 2022 05:09
@sdelamo
Copy link
Contributor

sdelamo commented Mar 30, 2022

@yawkat can you provide feedback?

@sdelamo sdelamo added the status: next major version The issue will be considered for the next major version label Mar 30, 2022
@sdelamo sdelamo added this to the 4.0.0 milestone Mar 30, 2022
Comment on lines +449 to +450
final Map<?, ?> node = lambdaContainerEnvironment.getJsonCodec().decode(Map.class, body.get());
((MicronautAwsProxyRequest) containerRequest).setDecodedBody(node);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Contributor Author

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

@timyates
Copy link
Contributor Author

So here

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
Copy link
Contributor Author

@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
Copy link
Member

yawkat commented Mar 30, 2022

@timyates i would use a JsonCreator in a mixin

@timyates
Copy link
Contributor Author

@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
Copy link
Member

yawkat commented Mar 30, 2022

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

@graemerocher
Copy link
Contributor

A custom deserializer is probably the simplest to be honest

@yawkat
Copy link
Member

yawkat commented Mar 30, 2022

@graemerocher
Copy link
Contributor

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

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

66.7% 66.7% Coverage
0.0% 0.0% Duplication

@timyates
Copy link
Contributor Author

timyates commented Apr 1, 2022

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

@timyates timyates added status: awaiting third-party Awaiting changes to a third party library relates-to: jackson labels Apr 1, 2022
@timyates timyates marked this pull request as ready for review April 1, 2022 08:05
@lesteenman
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

relates-to: jackson status: awaiting feedback status: awaiting third-party Awaiting changes to a third party library status: next major version The issue will be considered for the next major version

Projects

No open projects
Status: No status

Development

Successfully merging this pull request may close these issues.

6 participants