timheuer / alexa-skills-dotnet

An Amazon Alexa Skills SDK for .NET
MIT License
547 stars 108 forks source link

Task interface blocks deserialization. #218

Closed aadupirn closed 3 years ago

aadupirn commented 3 years ago

https://github.com/timheuer/alexa-skills-dotnet/blob/e866b3dd87eba299145f91015294522ddb2f896f/Alexa.NET/ConnectionTasks/IConnectionTask.cs#L14

https://github.com/timheuer/alexa-skills-dotnet/blob/e866b3dd87eba299145f91015294522ddb2f896f/Alexa.NET/ConnectionTasks/IConnectionTask.cs#L17

The two properties linked above, "type" and "version" are included in the IConnectionTask interface. These properties seem to be on the wrong file though I might be missing something .

The IConnectionTask interface is used to populate the "input" object that are intended to hold task parameters.

See this documentation to view an example request.

The actual "task" object has the "name" and "version" properties that can be used to determine what task input should be de-serialized. The "task" and "version" properties have no guarantee to be on the input object at all.

Am I missing something here about these properties being included when tasks are invoked? Its definitely possible that I missed this in the documentation somewhere.

Otherwise I can PR a fix to remove these properties from the interface.

github-actions[bot] commented 3 years ago

👋 Hi there! Thanks for your contribution to the project by posting your first issue!

stoiveyp commented 3 years ago

@aadupirn I believe that interface is for the StartConnection directive - docs here https://developer.amazon.com/en-US/docs/alexa/custom-skills/amazon-predefined-task-reference.html#example-response-with-input

If you look at the attached converter it deals with the known schemas

https://github.com/timheuer/alexa-skills-dotnet/blob/e866b3dd87eba299145f91015294522ddb2f896f/Alexa.NET/Response/Converters/ConnectionTaskConverter.cs

I know there's a lot of similar functionality in this area, took me a while to get it sorted out originally. It's late and I'm on my phone but will check the code in the morning 👍

stoiveyp commented 3 years ago

Actually, just spotted it on my phone.

LaunchRequest has a LaunchRequestTask - which is the object you're seeing in the docs.

https://github.com/timheuer/alexa-skills-dotnet/blob/master/Alexa.NET/Request/Type/LaunchRequest.cs https://github.com/timheuer/alexa-skills-dotnet/blob/master/Alexa.NET/Request/Type/LaunchRequestTask.cs

The interface you're looking at - IConnectionTask - is for the LaunchRequestTask input object or a StartConnection directive, depending on whether or not you're hosting or starting a custom task.

This is an interface with a converter as the exact definition depends on the task.

Hope that clears it up

stoiveyp commented 3 years ago

It's possible I've misread your request because of the "on the wrong file" comment.

If you mean that you've got a custom task that doesn't adhere to the interface - by all means raise a PR if you need to change it, the interface was written before custom tasks were generally available.

aadupirn commented 3 years ago

I think we're on the same page here. I'm accepting a LaunchRequestTask and my understanding is that the input property can be completely custom depending on the task configuration. Currently the ConnectionTaskConverter doesn't work if the type and version properties don't exist on the input property. Since the input property can be completely custom those properties won't always be there. You can get the task name from the name property from LaunchRequestTask as well as the version.

aadupirn commented 3 years ago

My proposed change would be something like allowing the input property to just be an object that the SDK user casts themselves to the input they expect.

stoiveyp commented 3 years ago

I personally wouldn't appreciate that level of functionality removal - you could keep the interface so that the converter works in the same way we do for Requests - the interface allows for custom conversion to be assigned, the converter can be re-written to handle the built in use cases, and users can extend its functionality for their custom objects.

It also means there's no additional change to the start connection directive.

stoiveyp commented 3 years ago

Can you give an example of the custom task input you're trying to handle?

aadupirn commented 3 years ago

Currently I'm testing a custom task with a request similar to this: https://developer.amazon.com/en-US/docs/alexa/custom-skills/implement-custom-tasks-in-your-skill.html#to-invoke-your-task-handler

I'm using an input that is an object with a single string property called "phrase".

aadupirn commented 3 years ago

I'm not sure if a converter like the RequestConverter would work here. Doesn't the request converter rely on an @type property?

The input here can be completely custom and has no properties that can be relied on because they can be completely custom per task configuration.

aadupirn commented 3 years ago

Here's an example request.


{
    "version": "1.0",
    "session": {
        "new": true,
        "sessionId": "amzn1.echo-api.session.aaf7b112-434c-11e7-2563-6bbd1672c748",
        "application": {
            "applicationId": "amzn1.ask.skill.7c648e9e-d5c4-45e4-bf4c-a55f1b1d4e33"
        },
        "user": {
            "userId": "amzn1.ask.account.12345ABCDE31H"
        }
    },
    "context": {
        "System": {
            "application": {
                "applicationId": "amzn1.ask.skill.7c648e9e-d5c4-45e4-bf4c-a55f1b1d4e33"
            },
            "user": {
                "userId": "amzn1.ask.account.12345ABCDEFGH"
            }
        }
    },
    "request": {
        "type": "LaunchRequest",
        "requestId": "amzn1.echo-api.request.da528275-5aa6-4f69-8038-06efc94d1923",
        "timestamp": "2020-01-29T23:11:48Z",
        "locale": "en-US",
        "task": {
            "name": "amzn1.ask.skill.7c648e9e-d5c4-45e4-cf4c-a55f1b134e33.LinkToPhrase",
            "version": "1",
            "input": {
                "phrase": "I want to ski"    
            }
        }
    }
}
aadupirn commented 3 years ago

To further clarify, the above request will only work if the input is modified to include these properties:


"input": {
    "@type": "LinkToPhrase",
    "@version": "1",
    "phrase": "I want to ski"    
}
stoiveyp commented 3 years ago

I understand the issue. Sorry if I wasn't clear - I didn't mean an exact copy and paste of the code. More that we have several instances in our libraries of a converter mechanism where you can register extensions - similar to that of Request. In this case something like Func<JObject,IConnectionTask> that returned the first non-null value would allow for a better experience overall.

The issue I have with dropping it to object is you lose any strong typing without the converter, JSON.Net will make it a JObject and you're back to magic strings or manually mapping your data - for anyone using the built in types (or their own where they've just added the two properties to get around the problem) that's a really bad experience.

If the change feels too big I'm happy to pick it up, it's a breaking change regardless of how it's achieved. In the interim you could add an alternative launch request into the request converter or add the two properties temporarily

aadupirn commented 3 years ago

Gotcha, I'll take a stab at it. I'll need a solution either way and I might as well add it to the project.

I'll close this issue and PR shortly and we can go from there.

Thanks for the prompt replies and help!

jesshannon commented 12 months ago

This still seems to be an issue. After creating a task in the Developer Console, it's not possible to remove them, and this will fail on deserialising the JSON when the task is present. It blocks certification.

Also when implementing the IConnectionTaskConverter, it's not possible to tell what the task is if it doesn't have properties. There's no way to access the task name in the CanConvert method.

stoiveyp commented 12 months ago

Hi @jesshannon - as this was closed so long ago I'm just going to step through this in my head.

So to confirm I'm thinking of the right problem your request JSON (hacked to show basics) looks something like this snippet below, and so you're stuck unable to progress. Is this accurate?

"request":{
  "version": "1.0",
  "session": ...etc
  "type": "launchRequest",
  ...etc
  "task" :{ },
  ...etc
}
jesshannon commented 12 months ago

@stoiveyp yes, although the JSON looks more like this

 "request":{
   "version": "1.0",
   "session": ...etc
   "type": "launchRequest",
   ...etc
    "task": {
     "name": "amzn1.ask.skill.[guid].MyTask",
     "version": "1",
     "input": {}
    }
   ...etc
 }

I kept getting unable to deserialize response. unrecognized task type '' with version ''

I eventually solved it by adding a converter with this as the CanConvert

public bool CanConvert(JObject jObject)
{
    return !jObject.Properties().Any();
}