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

private/model/api: Add EventStream over RPC support #1998

Merged
merged 9 commits into from
Jul 2, 2018

Conversation

jasdel
Copy link
Contributor

@jasdel jasdel commented Jun 18, 2018

Adds support for EventStream over JSON PRC protocol. This adds support for the EventStream's initial-response event, EventStream headers, and EventStream modeled exceptions.

Also replaces the hand written tests with generated tests for EventStream usage.

@jasdel jasdel force-pushed the feature/EventStreamRPC branch 2 times, most recently from 445ad54 to eebb93a Compare June 21, 2018 00:22
@jasdel
Copy link
Contributor Author

jasdel commented Jun 21, 2018

Change to RESTXML not complete, fails for S3 CompleteMultipartUpload

@jasdel jasdel force-pushed the feature/EventStreamRPC branch from eebb93a to 35c328d Compare June 21, 2018 22:15
@jasdel
Copy link
Contributor Author

jasdel commented Jun 21, 2018

fixed RESTXML's handling of unnamed implicit payload marshaling.

@jasdel jasdel force-pushed the feature/EventStreamRPC branch 3 times, most recently from c462eea to 348b582 Compare June 27, 2018 23:31
@jasdel jasdel requested a review from xibz June 27, 2018 23:32
@@ -103,7 +107,15 @@ func (b *xmlBuilder) buildStruct(value reflect.Value, current *XMLNode, tag refl
}
}

child := NewXMLElement(xml.Name{Local: tag.Get("locationName")})
Copy link
Contributor

Choose a reason for hiding this comment

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

Move to code gen locationName for event stream shapes.

@@ -444,6 +466,11 @@ func (r *read{{ $.ShapeName }}) readEventStream() {
return
}

if err, ok := event.(awserr.Error); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

Cast to error instead of awserr.Error

@@ -516,22 +622,443 @@ func (s *{{ $.ShapeName }}) UnmarshalEvent(
) error {
{{- range $fieldIdx, $fieldName := $.MemberNames }}
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for $fieldIdx. Just use range which already sorts, which is what fieldIdx was used for.

if err := payloadUnmarshaler.UnmarshalPayload(
bytes.NewReader(msg.Payload), s,
); err != nil {
return fmt.Errorf("failed to unmarshal payload, %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Serialization error should be used here.

}
{{- end }}
{{- end }}
expectEvents = expectEvents[1:]
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on why we are chopping off first element

sess := unit.Session
svc := New(sess, &aws.Config{
Endpoint: aws.String("https://example.com"),
DisableParamValidation: aws.Bool(true),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this disabled for benchmarks but not unit tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot tests do actually disable the parameter validation in the eventstreamtest.SetupEventStreamSession helper.

@@ -253,8 +258,11 @@ func (o *Operation) GoCode() string {

if len(o.OutputRef.Shape.EventStreamsMemberName) != 0 {
// TODO need better was of updating protocol unmarshalers
o.API.imports["fmt"] = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Can go away with removal of fmt.Errorf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to eventstream template generation, needed for a few error messages there.

sess, cleanupFn, err := eventstreamtest.SetupEventStreamSession(t,
eventstreamtest.ServeEventStream{
T: t,
Events: eventMsgs,
},
false,
true,
Copy link
Contributor

Choose a reason for hiding this comment

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

See if the operation is HTTP/2 and if it is able to be determined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't look like it.

{{ end }}

{{ define "set event message header" }}
{{- if $.memRef.IsEventHeader }}
{{/* Parms: *Shape */}}
Copy link
Contributor

Choose a reason for hiding this comment

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

*Params


{{- $offsetIdx := OptionalAddInt (eq $.Operation.API.Metadata.Protocol "json") 0 1 }}
{{- $exception := index $.Inbound.Exceptions 0 }}
{{- template "set event message" Map "idx" $offsetIdx "parentShape" $exception.Shape "eventName" $exception.Name }}
Copy link
Contributor

Choose a reason for hiding this comment

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

This template will not generate event message as an exception type.

jasdel added 3 commits June 28, 2018 21:33
* Use code generation for XML implicit payload location name
* swaps awserr.Error for error in event stream reader.
* cleanup template iteration.
* cleanup typos
* Fix test making sure events chan is closed.
@jasdel jasdel force-pushed the feature/EventStreamRPC branch from e4d4661 to dd76c95 Compare June 29, 2018 05:58
@jasdel jasdel merged commit d543020 into aws:master Jul 2, 2018
@jasdel jasdel deleted the feature/EventStreamRPC branch July 2, 2018 21:14
jasdel added a commit that referenced this pull request Jul 2, 2018
@awstools awstools mentioned this pull request Jul 3, 2018
xibz pushed a commit to xibz/aws-sdk-go that referenced this pull request Sep 12, 2018
Adds support for EventStream over JSON PRC protocol. This adds support for the EventStream's initial-response event, EventStream headers, and EventStream modeled exceptions.

Also replaces the hand written tests with generated tests for EventStream usage.
xibz pushed a commit to xibz/aws-sdk-go that referenced this pull request Sep 12, 2018
@diehlaws diehlaws added the needs-review This issue or pull request needs review from a core team member. label Jan 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-review This issue or pull request needs review from a core team member.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants