fix: event parsing iterator #2829
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yenfryherrerafeliz
force-pushed
the
fix_event_parsing_iterator
branch
from
34c30be to
11f4391
Compare
stobrien89
requested changes
Nov 24, 2023
yenfryherrerafeliz
force-pushed
the
fix_event_parsing_iterator
branch
4 times, most recently
from
177f2f8 to
96b045e
Compare
yenfryherrerafeliz
force-pushed
the
fix_event_parsing_iterator
branch
from
96b045e to
b092a56
Compare
yenfryherrerafeliz
force-pushed
the
fix_event_parsing_iterator
branch
from
b092a56 to
52874eb
Compare
stobrien89
requested changes
Dec 11, 2023
yenfryherrerafeliz
force-pushed
the
fix_event_parsing_iterator
branch
from
52874eb to
08e961d
Compare
stobrien89
approved these changes
Dec 11, 2023
yenfryherrerafeliz
force-pushed
the
fix_event_parsing_iterator
branch
from
08e961d to
1dde0de
Compare
The EventParsingIterator implementation does not parse correctly events when their payload do not have the eventpayload property set. The current behavior is that if said property "eventpayload" is not present then it fallback to extracting the value from the headers, but this is not a reliable method for parsing event streams since seems that not all services uses this pattern to provide the events. One example is the InvokeModelWithResponseStream operation from bedrock-runtime service. To fix this we pretty much followed the specs found in the smithy reference: https://smithy.io/2.0/spec/streaming.html#eventpayload-trait#id6, which explains that when eventpayload and eventheader is not set for any of the members of the shape then, the payload itself should be considered as the top level shape structure to be parsed. The test implementation for eventstream have also been refactored as follow: - We now have to specify the rest protocol that the input test data will be using, and based on that protocol we have a factory method to create either a rest xml or rest JSON parser. - Instead of validating if a specific member is present to validate if the parsed data type is the correct one, we now validate each parsed member to make sure they are all parsed into the expected data type.
yenfryherrerafeliz
force-pushed
the
fix_event_parsing_iterator
branch
from
1dde0de to
55dfcfe
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The EventParsingIterator implementation does not parse correctly events when their payload do not have the eventpayload property set. The current behavior is that if said property "eventpayload" is not present then it fallback to extracting the value from the headers, but this is not a reliable method for parsing event streams since seems that not all services uses this pattern to provide the events. One example is the InvokeModelWithResponseStream operation from bedrock-runtime service. To fix this we check if the event key is set in the headers, and if so then we pick the value from the headers, otherwise we set the event key value based on the event payload.
Issues #2817, #2787
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.