Form Wizard keeps state on subsequent requests and provides no way to cut short inside parse_params

[Michael P. Jung]

Since this issue is subtle I'll describe my concrete usecase first and than describe the issue and possible solutions to this.

Usecase:

I have a form that contains a ChoiceField which depends on the currently logged in user and some request parameters. Since I need a special behaviour I subclassed the FormWizard and did overwrite the methods parse_params, done, process_step, get_form and get_template. Inside parse_params I set some attributes of self. For some reason I assumed that returning anything but None would cause the Wizard to abort. However the return value is ignored and the wizard code continues by calling get_form and process_step. Since get_form is the only way of putting the stuff which was set earlier in parse_params I have a hook that calls some helper methods on the form to set up the options of the ChoiceField depending on the stuff set by parse_params earlier. Since the only way of injecting custom data there is self one has no chance of telling wether the value comes from the current request or something earlier which might just have failed. Thus no AttributeError is raised as the attribute is set. The data just does not come from the current request.

Even worse: In my case - since I was assuming that returning from parse_params with an HttpResponse would stop the wizard and return the HttpResponse immediately - caused invalid requests to be rendered with the state from a previous requests. A horrid leak of sensible data, as the wizard is part of a checkout process and the ChoiceField is used to pick an address. I'm just glad our QA noticed that glitch before putting this stuff live. It would not be hard to exploit and I don't like the idea of leaking sensible data.

Solutions:

1. By overwriting the __call__ method one can inject any attribute into self and return any HttpResponse object if something is wrong. I consider this rather ugly as the FormWizard explicitly states parse_params for getting stuff from the request object, but provides no way of breaking out of the wizard and returning a HttpResponse. I would like to see a way to return a HttpResponse object from within parse_params for shortcutting requests to a Wizard. (see attached patch)

2. The Wizard could be made immutable, but provide a custom 'state' attribute for transfering data between parse_params, process_step, etc. Anyhow this still would not enable the user of the FormWizard to shortcut a request with a HttpRequest. I would consider it a good idea not to be able to reuse a state from a previous request by accident. Another idea would be to discourage the users to store such state information as attribute, but rather enhance the form wizard to implement __getitem___, __setitem__, etc. so that it can be used to transfer state between the hook methods. Thus instead of doing "self.my_state = args[0]" one would do "self['my_state'] = args[0]".

3. In my case I solved this issue by wrapping the wizard inside a custom request method which creates a new instance of the Form wizard on every request. Maybe it would be a good idea to make it possible to have a class inside the url config rather than an instance. e.g. the url handler code could just check if the contained code is a class/type which needs to be instanciated first. This would make it possible to get a fresh instance of the object on every request.

Bottom line:

The FormWizard documentation needs to state that the Wizard object will be reused. Thus one must be aware that subsequent requests might get the same wizard instance. Thus any logic depending on a state set to self should be considered unsafe. I can imagine quite a lot of people using the FormWizard wrong without even knowing it.

I posted this as a 1.0 issue as I think this one should be figured out or at least documented properly. It gave me a hard time and caused a critical software bug that was hard to spot. It surely was my mistake to assume that parse_params would be able to cut short the request, but the way the FormWizard uses self to transfer state from one method to another rounded it up to some rather unpleasing issue.

I attached a simple patch which enhances parse_params so that it is capable of cutting short a request. The solutions (2.) is a bit more complex and needs some tweaking of the source and changing of the documentation so I did not provide a patch. Solution (3.) is just a quick fix that I don't consider very pretty, but use it for now.

[Michael P. Jung]

patch for FormWizard.parse_params to be able to cut a request short

[Jacob]

This isn't a 1.0 bug, mostly because you're just far too late. Apologies for being curt, but there's simple not time to discuss weather this change has to be madfe. I should note that the workaround is very simple: either don't store state on self, or instantiate the wizard in a view function.

Retagging post-1.0; I'll look at this closer later and decide if it's something that needs to be changed, or just documented better.

Thanks for the awesome bug report -- I wish more people would write stuff this complete!

[Rajesh Dhawan]

It's likely that a similar problem affects FormPreview if it's hooked into the URL conf as an instance as recommended by the docs. So, whatever decision is made on this should probably be made for FormPreview.

[(none)]

Milestone post-1.0 deleted

[hendrixski]

Ok, FormWizard is just full of fail. I've been trying to do the the dynamic form thing with it for hours now, with all sorts of crack tricks like overwriting init, then putting it in a view and it's still giving me tons of errors, one after another, that I have no idea what to do with.

Please make this flexible because now it is TOTALLY unusable in the places I need it most.
Thanks

[Luke Plant]

This ticket is confusing as it has two different issues which are mixed up. I'm going to treat this as a feature request for parse_params to return a response. #15050 is about that statefulness issue. However, it is possible that changes to FormWizard that are in the works might make this feature request moot.

[Jannis Leidel]

Superseded by #9200.