Make ConversationManagers easier to subclass [SWF-756]

[spring-operator]

!!Use pwebb rather than philw opened SWF-756 and commented

Could you consider changing the package scoped ConversationContainer and ContainedConversation classes in org.springframework.webflow.conversation.impl to be public?

I would like to override SessionBindingConversationManager, there is a hook method called createConversationContainer but because the return type is not public I cannot override it.

---

Affects: 2.0.2

Attachments:

• conversationpublic.patch (2.29 kB)
• updated-conversationpublic.patch (6.09 kB)

2 votes, 3 watchers

[spring-operator]

!!Use pwebb rather than philw commented

A hook factory method on ConversationContainer to create the ContainedConversation would also be helpful. As would access to the conversations list.

[spring-operator]

!!Use pwebb rather than philw commented

Attached a patch

[spring-operator]

!!Use pwebb rather than philw commented

OK, sorry about the rambling nature of this issue, I should have thought about this a bit more before I started typing. I have an updated patch, but perhaps I should try to explain what I am doing then the suggested changes will make a bit more sense.

I have the requirement to allow users to save their current flows without completing them and restore them again later. The saved flows will be stored to a database linked to a user ID. When the user returns and logs in they can restore any previously saved flows.

They way I intend to implement this is to subclass DefaultFlowExecutionRepository and provide additional load/save methods.

The save method gets the specified flow and saves the Conversation and FlowExecution. I remove the "flowExecutionSnapshotGroup" from the conversation before it is saved as I only need the current snapshot.

The load method creates a new flow execution key then restores the conversation. The problem now is that the conversation is not linked to a manager. What I need to do is rebind the conversation to the existing conversation manager along with the newly generated conversation id.

As I am working with SessionBindingConversationManager I currently have no way of implementing the rebind functionality without a lot copy/paste code.

If the conversation.impl package could be updated with the following changes I
would not need to copy the code:

• Change the package classes to public so that they can be subclasses.

• Provide protected setters on ContainedConversation to allow the container and ID to be replaced.

• Provide protected methods on ConversationContainer to get the conversations list and a factory method to create a new ContainedConversation.

• Change the getConversationContainer() method on SessionBindingConversationManager to protected.

Obviously you might be deliberately keeping these classes package scope to prevent subclassing. Perhaps these classes are just too likely to change to be made public.

[spring-operator]

!!Use pwebb rather than philw commented

Updated patch (ignore the older one)

[spring-operator]

Donnchadh O Donnabhain commented

It would be really nice if createConversationContainer returned a public type since the comment in the source code indicates that it is a hook function.
It would also be nice if a custom FlowExecutionKeyFactory could be plugged into AbstractFlowExecutionRepository/DefaultFlowExecutionRepository.

[spring-operator]

Donnchadh O Donnabhain commented

#247 looks like it would be relevant to the use case mentioned above.

[spring-operator]

Donnchadh O Donnabhain commented

Any news on this. Any chance of getting it scheduled? Making ConversationContainer public and ConversationContainer.nextId() protected would help.
Would a pull request help?

[spring-operator]

Phil Webb commented

I have scheduled this for 2.4

[spring-operator]

Phil Webb commented

Donnchadh O Donnabhain,

I have increased the visibility of several conversation scope classes [1]. Can you let me know if I missed anything that you need? I did not change the existing AbstractFlowExecutionRepository implementation in the end, but it should be possible to apply your own FlowExecutionKeyFactory logic by subclassing DefaultFlowExecutionRepository and overriding the four appropriate methods.

I will mark this JIRA as resolved for now; but feel free to re-open or submit additional pull requests as appropriate.

Cheers,
Phil

[1] 6f6e062

[spring-operator]

Donnchadh O Donnabhain commented

Hi Phil, that helps a lot. I would have liked ConversationContainer.nextId() to be protected but allowing ConversationContainer.createContainedConversation to be overriden should suffice.

[spring-operator]

Phil Webb commented

Donnchadh O Donnabhain, ConversationContainer.nextId() is now protected.

Thanks for the feedback!