search

mikeflynn / go-alexa

A collection of Amazon Echo / Alexa tools for Go development.
MIT License
260 stars 60 forks source link

Proposal: separate alexa request/response from http server #22

Open harrisonhjones opened 7 years ago

harrisonhjones commented 7 years ago

Found this project the other night. Looks great. Any thoughts about separating the http server components and the Alexa request/response into separate packages? I'm looking at developing my Alexa skills using a Lambda Go shim so I don't need the HTTP server part of this package but I like the SSML parts.

Also, I don't see a LICENSE for this package. How are you licensing it?

mikeflynn commented 7 years ago

This has come up a few times before but I haven't had a chance to dig in a refactor it to the point they are separated.

As for license, the skillserver dir originally had a GNU license file, but I just moved it to MIT with the file in the project root so GitHub picks it up.

harrisonhjones commented 7 years ago

I'm happy to attempt the refactor but I'd like to coordinate it ahead of time. Here's my proposal:

Thoughts?

mikeflynn commented 7 years ago

This is definitely along the same lines I've been thinking from the start (which is why it's been nested under go-alexa).

The project breakout makes sense and most of these moves should workable without too much logic changing, which is good. I'd hate to change too much on people in one shot as the breaking out of the projects is already a big change.

Anyone else want to weigh on this?

harrisonhjones commented 7 years ago

Proposal for migration:

  1. Add the new customskill and ssml directory and COPY code as needed
  2. Update skillserver main logic to use these new packages
  3. Mark duplicate code in skillserver as deprecated (From GoDoc: "To signal that an identifier should not be used, add a paragraph to its doc comment that begins with "Deprecated:" followed by some information about the deprecation.")
  4. Don't contribute any new code to deprecated packages/types

That should be pretty painless for endusers. If they need the new features they can refactor but until they do it will continue to work.

rking788 commented 7 years ago

I think all of that sounds reasonable and a useful change for future development.

mikeflynn commented 7 years ago

Sounds good to me. It's only one package now, so the vast majority of people are just using the skillserver as a single piece, meaning they would probably not even know the difference, but since Go isn't big on versions, this is safest way to go.

harrisonhjones commented 7 years ago

Great. I'm happy to start the refactoring but I'm a bit unsure the best way to send in a PR. I prefer small PRs to make them easy to review but I also don't want to commit unfinished code to mainline. How about I fork the repo, develop on a development branch, issue a PR against a development branch on this repo and then, once it's all stable (in a few PRs) issue a PR to bring it into mainline and remove the development branch? Thoughts?

mikeflynn commented 7 years ago

Yup, development branch is definitely the way to go for this.

I might have some free time later this week so let me know if you need any help, but this should be pretty straightforward I think.

rking788 commented 7 years ago

FYI i'm also working on dialog support in this PR, https://github.com/mikeflynn/go-alexa/pull/21

i'll be submitting a new commit to add the type safety as discussed. I would imagine this would need to be refactored into possibly a dialog.go file as part of this work after its merged.

mikeflynn commented 7 years ago

Yup, we'll have to have a little air traffic control to get both of these projects landed, but lets just see which is done first and go from there.

harrisonhjones commented 7 years ago

Hey @mikeflynn (and anyone else @rking788?) I'm working on the refactor for the custom skills request / response and I wanted y'all's input. As I have it now I've renamed EchoRequest (the high-level request struct) to Envelope (not married to the name) and built a custom JSON unmarshaller for it. You hand it the incoming question to your skill and you get an Envelope back. This Envelope has the following signature: 

type Envelope struct {
    Version string  `json:"version"`
    Session Session `json:"session"`
    Context Context `json:"context"`
    Request interface{} `json:"request"`
}

The part I want feedback on is what I've done with the Request. I wanted to do away with our single Request type that has lots of fields that aren't used for any particular request (all request types smooshed into one) so Request is an interface{} and its populated automatically by the unmarhsaller with the appropriate *Request type. Unfortunately that means that you have to do some fancy work to actually read the data. You need to use reflection to check the type and then cast it to that type. Depending on how you implement your custom skill I don't think this is too much of a big deal. You simple do something like switching off of the type reflect returns and then go from there. Thoughts?

rking788 commented 7 years ago

Would it make sense to provider specific getters for the different request types that do the type assertion/casting and returns either that request type or an error if Request is not of that type?

Just seems like that would be convenient for cases where the client usually knows what type of request should be of a specific type but also leaves flexibility for clients to handle the Request property more dynamically if they want.

harrisonhjones commented 7 years ago

@rking788 I don't think there is a way to avoid the casting by the client (if you can figure out a way I'd appreciate a code snippet). What we could do it something like Envelope.GetType which handles the reflection for the client and then all they need to do is perform the cast.

I don't think clients usually do know what the type of an incoming request is ahead of time. How would they (or maybe I am misunderstanding?)

rking788 commented 7 years ago

I think i need to go back and look at the Alexa request structure docs later tonight. Is the plan or the Request to be a custom struct type? or will it be more like a map[string]interface{}. I guess i assumed when you said they would switch off the type reflect that there was a predefined set of Request structs that could possibly be returned. The way things are defined now, isn't the EchoRequest type just a union of all the fields that are returned in the different request types?

harrisonhjones commented 7 years ago

Is the plan or the Request to be a custom struct type? ... I guess i assumed when you said they would switch off the type reflect that there was a predefined set of Request structs that could possibly be returned.

Yes, that's what I was thinking. I've got request types for IntentRequest, SessionStart, etc

The way things are defined now, isn't the EchoRequest type just a union of all the fields that are returned in the different request types?

Yep. That's how it is now and while it works it's not ideally (imo) because it exposes a number of fields that have nothing to do with the request. Ideally clients should only be presented with an object that is 100% relevant to the request that is being made.

So you'd do something like

// inside of your switch's case "IntentRequest"
request, ok := envelope.Request.(intentRequest)

if !ok {
panic("Oh no, my switch statement is messed up")
}

// request is a intentRequest
harrisonhjones commented 6 years ago

@rking788 I was thinking about this and I don't actually think the cast is needed by the client. We could provide something like the official alexa-sdk (for NodeJS) and let the client register handlers for each request type. These handlers would already take the specific *Request object as a parameter.

Top of my head I imagine something like this:

skill = customskill.New(w io.Writer)

skill.SetIntentRequestHandler(func(Request.IntentRequest) error)

and when a new request came in you would call skill.Handle(envelope string) which would parse the envelope and automatically call the appropriate registered handler.

Thoughts?

rking788 commented 6 years ago

that sounds like it could work. so the current skillserver does something similar to this I think. The EchoApplication struct already allows you to specify different handlers for OnLaunch, OnIntent, and OnSessionEnded. Would the plan be to register handlers based on intentName? or refine these different handlers to take specialized request types instead of their generic EchoRequest all of the handlers currently take.

I was looking into this some more and maybe we could do something similar to the type switch described on this page: https://github.com/golang/go/wiki/Switch

I think if there are specialized request type handlers then the next step is just to switch on the intent name though.

mikeflynn commented 6 years ago

Coming in late on this, but the handler for each type of request is in there and I think that's the best way to do it. I think API interactions like this can be easily overthought but the best way to go is to make the user (developer) clear about what they are trying to do (specifying a specific handler, etc) and then offload a much complexity as possible in to the library.

I agree the "all-in-one" request object was getting large, but it did allow the handlers to not have to worry about the input as much. The library started off with a single handler, but now that it's broken out to multiple handlers you could have each one expect a different request type without adding much complexity to the handler developer...which sounds exactly where you both ended up!

As for the "Envelope" name, fair enough that the "EchoRequest" name ended up not making much sense in relation to how Amazon evolved the product, but any new name should include "Request" in the name just so it's appropriately descriptive.

harrisonhjones commented 6 years ago

I agree the "all-in-one" request object was getting large, but it did allow the handlers to not have to worry about the input as much. The library started off with a single handler, but now that it's broken out to multiple handlers you could have each one expect a different request type without adding much complexity to the handler developer...which sounds exactly where you both ended up!

I agree. Let me think about a way to address this properly and submit a PR for it. I obviously need to take another look at the existing logic before moving forward.

As for the "Envelope" name, fair enough that the "EchoRequest" name ended up not making much sense in relation to how Amazon evolved the product, but any new name should include "Request" in the name just so it's appropriately descriptive.

I'm on board with this. How about something like RequestEnvelope ? Alternately, and I'm not sure if I like this, we could treat the incoming request as two things: metadata (version & session) and the actual request and then pass those two things to the handlers: a new metadata object (doing away with the envelope entirely) and the actual request. Thoughts?

harrisonhjones commented 6 years ago

@mikeflynn & @rking788 new PR for your review/thoughts: #28

mikeflynn commented 6 years ago

I'm back from vacation and catching up!

RequestEnvelope works for me. Not sure about the breaking things out at first blush. I'd need to see it in action...and I'm headed to your PR next so I'm guessing I'll see it there!

rking788 commented 6 years ago

still getting back into things after vacation too. taking a look at the PR now.

harrisonhjones commented 6 years ago

With #28 merged into the refactor branch I thought we could talk about the remaining work to get this branch pushed to mainline/master. As per @mikeflynn's comments near the end of #28 I think there are a few different types of examples we need to provide: usage examples and skill examples. I've listed what I've come up with below.

Usage Examples

These examples illustrate how to use the customskill package in various forms. These are absolutely required to merge the refactor branch into mainline. I'm unsure where these examples should "live" so I would appreciate feedback @mikeflynn. Perhaps go-alexa/customskill/examples/usage/?

I also suggested:

Skill Examples

These example illustrate how to build skills with different features (audio player, dialog, etc). All examples should come with SMAPI (https://developer.amazon.com/alexa-skills-kit/smapi) skill definitions, instructions on how to deploy and test them, and any assets required to run them. I don't think these are 100% required for the merger of the refactor branch but by implementing them we should discover any issues with the code before release and therefor make it easier to maintain the Go compatibility promise.

I can test this skill on an Echo Show and Echo Spot.

This skill might be a bit hard to test as I am not sure which devices on the market make use of this interface. Perhaps the Echo Show and Echo Spot w/ on-screen controls?

I can test this skill on an Echo Show and Echo Spot.

Stretch Skills

We should also consider skills which implement the following Alexa features:

Note, some of these "stretch" skill ideas require calls to the Alexa API. We should consider creating a new sub-package for accessing this API.

Finally

Assuming we (@mikeflynn, @rking788, + anyone else :) ) all agree with the above I would appreciate some help hammering each example out. I'm happy to break it out into individual issues, assign owners (if anyone wants to take ownership of a particular task), and get it done. Let me know what y'all think.

rking788 commented 6 years ago

That plan sounds reasonable to me. I have never worked with SMAPI before so i'll had to look into how that works. Is it possible to refactor the existing example in the repo to work for the "base Alexa skill" example?

I would just recommend that any Lambda shim examples maybe be a low priority since official support is coming at some point, it may not be worth the trouble to build out the workaround example.

harrisonhjones commented 6 years ago

The existing example at customskill/examples/http shows how to create a skill using go's standard http server. I would prefer we keep that one very simple. It should be updated so at least it works (and has some install instructions) but I won't build it out much.

I'm imaging the following folder structure:

Does that work for everyone?

akshaylb commented 5 years ago

@harrisonhjones @mikeflynn , been using the refactor branch. Has been really useful! Although I need to add support for CanFullfilIntentRequest. I could try and submit a PR too.

Things, however, seem a little slow here. Any way I can help?

mikeflynn commented 5 years ago

Yeah, this refactor kept getting larger and then things stalled. Any help in additional changes, lists of what's needed, or testing would be appreciated!