IPersistenceStore Interface is Incorrect

[ncipollina]

IPersistenceStore Interface is Incorrect

[stoiveyp]

Sorry for the delay in replying. Currently trying to get over a horrible cold.

There will always be differences in my packages compared to the official SDKs, I'm not a copy/paste kinda person - but you're spot on with the need for some way of the persistence store being able to handle the uniqueness of each request.

If you want to submit a PR, feel free - that isn't going to break the RequestHandlers repo. Anything that changes the contract will be a breaking change, and therefore a major version bump, so it won't get picked up by the dependent repos. That's why I don't need to worry about merging the repos, if I version properly it shouldn't cause trouble.

I'll be looking at fixing this in both repos moving forward anyway - now you've highlighted it I don't know how I missed it to start with 😄 Thank you!

[ncipollina]

I know you said that you aren't a copy/paste kinda person, but I do feel like there is real value in having a separate Save method for the IPersistence interface. Especially because you are setting records one at a time, I don't want to have to implement saving every single time a value is set. I'd rather set a collection and save all at once. If you add a Save method to the interface, I would at least have the option. I understand that not everyone will implement this the same way I have, and may prefer to save on each Set, but having the Save method gives the end user the option of implementing either way.

Just my $.02

[stoiveyp]

@ncipollina I agree it's down to each store to decide how to save data, so why should it be forced for all?

There's nothing stopping your implementation having a final save method that is called - the interface just ensures it will work within the rest of the package structures. What am I missing?

[ncipollina]

I'm just suggesting you add a Save method to the interface and to the ISkillState interface. If you don't want to use that method in your implementation, you just don't add the saving logic to the method. Below is an example of how I envision this interface looking:

    public interface IPersistenceStore
    {
        Task<T> GetAsync<T>(SkillRequest request, string key);

        Task SetAsync<T>(SkillRequest request, string key, T value);

        Task SaveAsync(SkillRequest request);
    }

and

    public interface ISkillState
    {
        Task<T> Get<T>(string key);
        T GetRequest<T>(string key);
        T GetSession<T>(string key);
        Task<T> GetPersistent<T>(string key);

        void SetRequest<T>(string key, T value);
        void SetSession<T>(string key, T value);

        Task SetPersistent<T>(string key, T value);

        Session Session { get; }

        Task SavePersistent();
    }

Otherwise, if I want to implement the Saving logic separately from the Setting logic, I have to implement my own ISkillState instance rather than using the SkillState implementation provided in your package.

[stoiveyp]

Sorry @ncipollina - not trying to be difficult, I understood the logic you were asking for - I just needed a solid use case explaining why this is good for all implementations. The SkillRequest change you requested was spot on - very difficult to generate any persistent store that can work alongside lots of requests without that information being available in the interface. Adding it makes everything clearer and cleaner. That's why it's already changed 👍

To use the SkillState object though, you have to pass in the IPersistenceStore object, or set it against the Pipeline if you're using the RequestHandler package.

If you look at the following examples below, they seem to give the same result - but don't force this possibly optional method into the implementation. Yes, they're simplistic for examples in this thread, but the point is to show that saving the store isn't an integral part of the SkillState object - it's code specific and therefore not required in the interface. These save methods could just as well be part of ASP.NET Core MiddleWare or DI life cycle maintenance etc.

var skillState = new SkillState(request, myPersistenceStore);
//...skillState logic...
await myPersistenceStore.Save(request);

or, if you're using RequestHandlers

var conversation = new AlexaRequestPipeline();
//...pipeline setup....
conversation.StatePersistence = myPersistenceStore;

await conversation.Process(skillRequest)
await myPersistenceStore.Save(skillRequest);

Appreciate you talking this through - it's hard when I have implementations that would definitely use this. But it still feels like an optional step, and therefore not right for the interface.

[ncipollina]

@stoiveyp, I too am not trying to be difficult. I could definitely go with the solution you suggested, my only issue is that in my RequestHandler, I have a reference to an IPersistenceStore object and not the actual implementation type (I'm using dependency injection). Since the type is IPersistenceStore, the compiler won't know about my Save method, and the example you provided will not work.

[stoiveyp]

That's great, thanks! Now I have a use case I can look at a suitable alternative.

Got an idea - I'll take a proper look after work tonight (few hours) and add you as a reviewer.

[stoiveyp]

@ncipollina while I'm in my meeting - could this not be handled by inheriting IPersistenceStore and using that interface for DI?

public ISaveablePersistenceStore
: IPersistenceStore
{
  Task Save(SkillRequest request);
}

Apologies if the formatting is off - using mobile app

[stoiveyp]

Adding this to the package would be an easy minor version bump - totally doable. Thoughts?

[ncipollina]

@stoiveyp this seems like a good approach. You'd need to update the SkillState class to use this interface, but this would be a good compromise. For folks that don't want to have a separate Save method, they could use the IPersistenceStore interface and for those that want save, they could use ISaveablePersistenceStore. Seems like a good compromise.

[stoiveyp]

Why would SkillState need it if, in your use case, you can access it via the interface?

[ncipollina]

I'm interfacing with the Persistence through the SkillState instance. I guess my idea of how to implement this just differs from your vision.

[stoiveyp]

I do feel we're entering the realm of customising the library specifically for your use case I'm afraid. Casting the SkillState store to ISavablePersistenceStore and running the method would work in your scenario.

SkillState is there to allow code to access the different levels of state in a single way, and RequestHandlers are there to easily alter logic within the skill without changing the flow in and out of the hosting code.

Neither package are built to handle user specific data life cycle management, which is why they're not in the interfaces.

The overriden interface is probably your best bet for handling this, but I appreciate you raising the issue, and I was happy to add the SkillRequest code to the persistence stores - thanks for the conversation :slightly_smiling:

Equally happy to carry on the conversation from a tech view outside of the library context - SkillState requires per request knowledge so it sounds like really interesting DI setup.

[ncipollina]

Yeah, I'd say let's leave well enough alone.