v1adapter response object does not support CanFulfillIntentRequest response

[inlyra]

I'm submitting a...

[ ] Regression (a behavior that used to work and stopped working in a new release)
[ ] Bug report 
[ ] Performance issue
[x] Feature request
[ ] Documentation issue or request
[ ] Other... Please describe:

Expected Behavior

Documentation at Implement CanFulfillIntentRequest reads:

to add support for the CanFulfillIntentRequest interface to your Alexa skill, you must update to the ASK SDK v2 for Node.js (Public beta)

It seems reasonable to expect that v1adapter in SDK v2 2.x_public-beta (as of 44b113b875cc6882a4a13c92b454d6fb2e08d954) will support canFulfill via responseBuilderShim.ts

Current Behavior

While ResponseBuilder.ts has been updated for CanFulfillIntentRequest response support, responseBuilderShim.ts has not been updated. Skills that use v1adapter will have further migration overhead to support this request.

Possible Solution

A simple update for ask-sdk-v1adapter/lib/responseBuilderShim.ts might be sufficient:

...
    ResponseBuilder.prototype.canFulfillIntent = function (fulfillment) {
        this.responseHelper.withCanFulfillIntent(fulfillment);
        this._responseObject.response = this.responseHelper.getResponse();
        return this;
    };
...

Context

Skills that rely on previous Node event handling and multi-state Unhandled logic, as well as those with NewSession evaluation, might demand more time and effort to complete v2 SDK migration. The V1 adapter has been and can continue to be a useful solution.

[tianrenz]

Hi @inlyra ,

Thanks for your support in ASK SDK. We understand that it may create additional cost when migrating from v1 to v2 SDK. However, in order to provide the best possible SDK support with limited bandwidth, we have to focus all feature updates on v2 SDK only. We are working at the same time to provide documentation to make migration plans as smooth as possible. The v1 adapter provides 100% feature parity for v1 SDK and meanwhile support registering additional v2 handlers (through which you will have full access to the latest SDK feature).

Regards

[inlyra]

Hi @tianrenz,

Thanks for your response! To put this into perspective, the SDKv2 will not allow even a single ask-sdk-v1adapter to remain in use in order to apply CanFulfillIntentRequest support. That implementors should be required to fully migrate to v2 handlers in order to benefit from new Alexa functionality is really the driving issue for this ticket. I think that if the SDKv2 could support a v1adapter, while still requiring CanFulfillIntentRequest handling to be performed in v2 handler, this would be a very good solution. For clarity, see:

CanFulfillIntentRequest for a skill which is not enabled by the user

Because we know that userId will not be present on certain CFIR requests, an SDKv2 "adapter" wrapped v1 handler will fail without special considerations (eg. overriding :saveState routing or the persistence adapter, as these make implicit use of userId in the v1adapter.) As a result, the dispatcher call chain will abort before any v2 handler is invoked.

Thanks again for your consideration of this matter. Please let me know if any of my comments seem unclear or inaccurate and I will do my best to improve on what is here!

Kind Regards

[tianrenz]

Hi @inlyra ,

I think this is a issue that's hard to bypass due to v1 design of attributes management.

The current behavior is that the SDK will try to read from dynamoDB table using user id at the beginning of current utterance session and load attributes into this.attributes. When handling a CFIR, the absence of userId will cause an error being thrown.

To be clear, users who are using the v1 adapter without enabling the persistent feature (i.e.: dynamoDBTableName is not set) can still add support for CFIR via adding additional v2 handlers. The absence of userId will not affect the request handling flow and the v2 handlers should be able to handle the request.

However, if user enables the persistent feature, then even with original v1 SDK, they'd still run in to problems with data persistence since v1 SDK uses userId internally for persisting data to dynamoDB and that is not configurable to developers.

To ensure that behavior match, v1 adapter will have to pertain this implementation. The purpose of v1 adapter was to allow production skill to go through a granular migration process without experiencing skill service interruption as well as creating too much of migration load in one time as they can migrate part of skills on their own schedule.

Regards

[inlyra]

What if the SDK were to bypass the persistence adapter initialization in the case that CFIR is passed without the userId? Taking a look at the work involved, and I came up with what is below. Can you comment on this quick analysis?

1. adapter.ts : Since dynamoDbPersistenceAdapter will always be initialized from DynamoDbPersistenceAdapter with the default PartitionKeyGenerators.userId, a simple "user session" check on the requestEnvelope during ValidateRequest() will resolve v1 handler initialization exceptions. Something like:

-        if (this.dynamoDBTableName && (!this._event.session.sessionId || this._event.session.new)) {
+        if (this.dynamoDBTableName && isUserSession(this._event) && (!this._event.session.sessionId || this._event.session.new)) {

2. responseHandlers.js: The same would have to be done here on :saveState since dynamoDBTableName might be defined (alternatively, a change could be made to the evaluation at : responseReady, but this seems to unnecessarily burden the implementor that might rely on :saveState routing.)

-        if (response.shouldEndSession || forceSave || this.handler.saveBeforeResponse) {
+        if (isUserSession(this.event) &&
+           (response.shouldEndSession || forceSave || this.handler.saveBeforeResponse))
+        {

This seems to resolve the issue of v1adapter instability during CFIR without side-effects. Thanks for your response.

[tianrenz]

Hi @inlyra ,

What you are requesting here is a behavior change or a feature update. However, please allow me to point out that v1adapter is meant to provide backward compatibility with v1 SDK and a happier migration path. It's not intended to support additional features apart from what's available from v1 SDK.

All future feature update, enhancement will be made in v2 SDK. This is to make sure that we can focus our resources on providing a better SDK experience in a more efficient way.

Regards