Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Events v4 serialization v2 #331

Merged
merged 2 commits into from
May 5, 2022
Merged

Events v4 serialization v2 #331

merged 2 commits into from
May 5, 2022

Conversation

andclt
Copy link
Contributor

@andclt andclt commented Apr 22, 2022

Issue #, if available:
#282

Description of changes:
Initial implementation of Events V4 and Serialization V2

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@andclt andclt changed the base branch from master to events-v4-serialization-v2 April 22, 2022 17:11
aws-lambda-java-events/pom.xml Outdated Show resolved Hide resolved
aws-lambda-java-events/pom.xml Show resolved Hide resolved
@@ -0,0 +1 @@
com.amazonaws.services.lambda.serialization.JacksonPojoSerializer
Copy link
Contributor

Choose a reason for hiding this comment

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

How will translate when GraalVM is applied? Will it still use reflection?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From 1.0-RC8:

The native image generator now has automatic support for services loaded using ServiceLoader. All service implementation classes, listed in the META-INF directory, are available automatically as soon as the service interface is used. This eliminates the need to manually register resources and reflection support for such classes. The automatic registration can be disabled with the -H:-UseServiceLoaderFeature option.

return mapper;
}

public static final class VoidDeserializer extends JsonDeserializer<Void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to serialize Void as not null.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you give me an example ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still trying to understand why you need the Void type. Is there any real-world example?

@andclt andclt merged commit d8deb6b into aws:events-v4-serialization-v2 May 5, 2022
@AdamBien
Copy link

I reviewed the recent changes / diffs. Built-in Java libraries, like e.g. java.time reduce the complexity and make maintenance easier.
Lombok is controversial - essentially, lombok is a code generator, so the coupling is tighter and the dependency more critical.
I prefer JSON-B / JSON-P for json handling - however, Jackson can be also considered as "quasi" standard.

@GoodforGod
Copy link

GoodforGod commented Jun 3, 2022

  1. Standardization is definitely what was missing and its good that it is coming in v4. Nice that code base become consistent.

  2. Lombok in this type of library in my opinion is okay (only Java 11+), cause the whole purpose of library is to provide DTO, lombok allow for easier source code reading for developers and allow devs to maintain library easier, win-win.

  3. Ability to instantiate it via new Event(), new Event(constructor ... args), lombok builder, static builders, DTOs are mutable even there is builder.. kinda messed up for DTO library, too many API for the same purpose.

Data annotation with Accessors(chain = true) is more than enough in my opinion.

  1. Jackson as a first citizen serializer is not a good way for a DTO library. Cause now lib is incompatible with other serializers at this point (GSON for example). It should be then aws-jackson-events library to point directly that it is jackson focused lib or support all types of serializers by proper field naming where it is possible.

  2. Removing joda-time is a way to go, should be done like starting when Java 8 was out, but thats better late than never. In my opinion it is better to serialize time into ZonedDateTime and not Instant with force UTC time conversion, cause that's not what users expects.

  3. Keeping clone interface compatability for v4 is not the way to go in my opinion, v4 is a Major version so thats okay if there are breaking changes (devs should expect that), so clone methods should be removed.

  4. Some events when created initialize new collections, like new ArrayList() in their contractor or field based immediately on instantiation, collection will prob be fully replaced by serializer so this just generates garbage at this point, can be cleaner up.

Here is some feedback and my thoughts on this topic 🙃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants