Skip to content

Duplicate all eventstream event shapes + add new legacy event modes #6052

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

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

alextwoods
Copy link

@alextwoods alextwoods commented Apr 22, 2025

Fix issues with eventstreams with shared event shapes: Rename all eventstream event shapes with a unique name + add new legacy event modes.

Motivation and Context

When an event shape is shared between multiple eventstreams, it causes SDK generation/compilation failures. The top level shape POJO implements the event stream interface for each stream and the return type of the sdkEventType method conflicts.

Modifications

This PR does two things:

  1. Modify the useLegacyEventGenerationScheme to take an enum per event shape - the old legacy event generation uses NO_EVENT_SUBCLASS and the current behavior uses NO_UNIQUE_EVENT_NAMES.
  2. If useLegacyEventGenerationScheme is unset or DEFAULT for an event, the event shape will be duplicated and given a unique name: <ShapeName><EventStreamName>.

Note: There are a large number of changes in the codegen test to add the new customization mode to existing test eventstreams.

Open Questions

What naming scheme should we use to produce unique names?

This PR currently uses <ShapeName><EventStreamName>, for example an event named Payload in eventstream StreamA would get the name: PayloadStreamA. We could reverse this (StreamAPayload).

The downside of this naming scheme is that it can produce long names. For example the existing HeartbeatEvent from LexRuntimeV2's StartConversationResponseEventStream would be (without the legacy customization) renamed to: HeartbeatEventStartConversationResponseEventStream.

FAQ

Why do we always need to rename the event shape? Couldn't we do that only when there is a shared event?

We must ensure that shapes names are deterministic and do not depend on the shapes in the model because if a shared event shape is added in the future, we have no way of telling which was the existing shape.

Alternatives

For POCs of alternative approaches see:

TODO:

  • Improve testing

User Experience

There are no changes for users for any existing Eventstreams/events. New eventstreams or new events added to existing evenstreams will have unique, longer names, but the fluid visitor and builder methods will continue to use the member names. For example, assuming we have Payload event (with member name "PayloadEvent") in eventstream StreamA, the handler and builder methods will still be onPayloadEvent and payloadEventBuilder.

// Handling events
StreamAResponseHandler.builder()
    .subscriber(
        StreamAResponseHandler.Visitor.builder()
            .onPayloadEvent(event -> { // NOTE: onPayload is still used because the member name is "PayloadEvent"
                System.out.println("Payload event: " + event);
            })
            .build()
    );

// sending events
subscriber.onNext(
    StreamA.payloadEventBuilder() // NOTE: We still use `payloadEventBuilder` because the member name is "Payload"
        .payload("text/plain;charset=utf-8")
        .build()
);

Note: For users using switch/case or if/else on sdkEventType or checking instance of, when casting the events they will need to use the longer shape names. Using the example from above:

subsciber(event -> {
    if (event instanceof PayloadStreamA) { // NOTE: we need to check the rename/unique shape name here
        handlePayloadEvent((PayloadStreamA)event);
    }
});

subscriber(event -> {
    switch(event.sdkEventType()) {
        case PAYLOAD_EVENT: // the sdkEventType will still use the member name
            handlePayloadEvent((PayloadStreamA)event); // Note: Need to cast to the renamed/uniqe shape name
            break;
    }
})

Testing

New Unit tests - see shared event stream cases (model was modified to add new streams that share an event)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed
  • I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 licens

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.

1 participant