Skip to content

Adding Lex v2 event and response #321

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

Merged
merged 26 commits into from
Aug 22, 2022

Conversation

Sordie
Copy link

@Sordie Sordie commented Apr 4, 2022

Issue #, if available:
#242

Description of changes:
Adding both Lex version 2 event and response as defined here: https://docs.aws.amazon.com/lexv2/latest/dg/lambda.html

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

@Sordie
Copy link
Author

Sordie commented Apr 5, 2022

Thanks for the quick feedback, I have added the book trip example event as test case. However, for now there is a mismatch. The Json contains

"slots": {
    "ReturnDate": null,
    "PickUpDate": null,
    "DriverAge": null,
    "CarType": null,
    "PickUpCity": null
}

while the result is

"slots": {},

I'm unsure if that's the primary reason for the failing test and how to best fix this. Additionally, "nluConfidence": 1 gets parsed as "nluConfidence": 1.0 as expected but might still result in errors while testing.

As the test Json right now does not contain all possible fields, I will see if I can capture them in the next few days.

Finally, after having merge the current master the LambdaEventSerializersTests appear to have been skipped, at least on my end.

private double mixed;
private double negative;
private double neutral;
private double positiv;
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this a typo?

@msailes
Copy link
Collaborator

msailes commented May 13, 2022

Hi @Sordie,

Could this PR be change to target the V4 branch? You can use the Jackson annotations to handle the capitalisation.

@Sordie Sordie changed the base branch from master to events-v4-serialization-v2 May 14, 2022 20:02
@msailes
Copy link
Collaborator

msailes commented May 14, 2022

Could you check the tests?

@Sordie
Copy link
Author

Sordie commented May 25, 2022

Hi @msailes,
I have added an additional Lex event that includes the optional parts for both sentiment and Kendra. However, I’m still struggling with the tests. As already mentioned above the tests currently fail, because the slots map is empty after the deserialization and subsequent serialization. At this point I haven’t found an easy way to fix this. Could you share your opinion on how to best proceed? Maybe you already have an idea how to solve this issue?

@msailes
Copy link
Collaborator

msailes commented May 28, 2022

The testLexV2Event seems to be failing because the test input lex_v2_event.json contains a Map with null values. In the JacksonPojoSerializer class we set mapper.setSerializationInclusion(Include.NON_NULL); which excludes null values.

          "intent": {
              "slots": {
                  "ReturnDate": null,
                  "PickUpDate": null,
                  "DriverAge": null,
                  "CarType": null,
                  "PickUpCity": null
              },

I don't know if this is bad data or possible? @Sordie do you know?

@msailes
Copy link
Collaborator

msailes commented May 28, 2022

Same for the testLexV2KendraSentimentEvent test.

@Sordie
Copy link
Author

Sordie commented May 28, 2022

@msailes thanks for pointing out where the configuration to exclude null values is, as I had not seen it before.
Regarding the possibility of bad data in the test events, both event json files contain data taken directly from the input of a TypeScript Lambda that was attached to a Lex bot. I only replaced some unique identifiers with example strings. Therefore, I would say that this data is representative of the actual input a lambda will receive from the Lex service.

@msailes
Copy link
Collaborator

msailes commented May 28, 2022

@Sordie thank you.

I think the tests you've added should be slightly changed then to show this behavior. The actual will be different to the json from the test file because maps which include items will null values are excluded.

@andclt, @smirnoal I'm not sure how you want this implementing, maybe two files would be a suitable way?

@andclt, @smirnoal I found this a hard problem to track down since there was no specific JSON knowledge in the testing lib. I would suggest with move to JSONassert either in this PR or another seperate one. This library gave much better error messages and I was able to spot the problem straight away.

@msailes
Copy link
Collaborator

msailes commented Jun 2, 2022

@Sordie, can you update your branch to pull in the new JSONAssert changes.

Then I suggest you change your tests to assert that given a json document with a map of items with null values, these are removed during serialisation.

#344

@andclt
Copy link
Contributor

andclt commented Jun 2, 2022

First of all thanks for contributing @Sordie!

I agree with @msailes' suggestion. However, if you want to keep those null values after the serialization, I think you can achieve it adding the @JsonInclude annotation (https://fasterxml.github.io/jackson-annotations/javadoc/2.9/com/fasterxml/jackson/annotation/JsonInclude.html) at the field level. This will override the global custom serialization behaviour that is currently excluding null values.

@msailes msailes marked this pull request as ready for review July 3, 2022 15:56
@msailes msailes requested review from andclt and smirnoal July 3, 2022 15:56
@andclt
Copy link
Contributor

andclt commented Aug 7, 2022

Hi @Sordie,

As I said in a previous message on this thread, I think we should use the @JsonInclude annotation to override our current Jackson configuration (that's excluding null values) instead of using a different json file to compare the lex event v2 serialization.

I was able to test successfully the following implementation, could you please take a look at it?

private Map<String, Slot> slots;

@JsonInclude(value=JsonInclude.Include.NON_NULL, content=JsonInclude.Include.ALWAYS)
public Map<String, Slot> getSlots() {
    return this.slots;
}

@JsonInclude(value=JsonInclude.Include.NON_NULL, content=JsonInclude.Include.ALWAYS)
public void setSlots(Map<String, Slot> slots) {
    this.slots = slots;
}

@Sordie
Copy link
Author

Sordie commented Aug 8, 2022

Hi @andclt, thanks for providing a code snippet. I will add your suggestion next week once I have a computer in front of me again.

@andclt andclt added the events-v4 To be pulled into aws-lambda-java-event v4 label Aug 10, 2022
@Sordie
Copy link
Author

Sordie commented Aug 21, 2022

Hi @andclt, I have applied your suggestion and removed the now unused json files.

@andclt
Copy link
Contributor

andclt commented Aug 22, 2022

Hi @Sordie, thank you! Approved!

@andclt andclt merged commit 8370536 into aws:events-v4-serialization-v2 Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
events-v4 To be pulled into aws-lambda-java-event v4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants