Fixes #220 - Avoid inifinite recursion while getting wizard form list

[codecov[bot]]

Codecov Report

Merging #239 (e814fe5) into master (aaaa361) will increase coverage by 0.61%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #239      +/-   ##
==========================================
+ Coverage   93.22%   93.83%   +0.61%     
==========================================
  Files          11       11              
  Lines         531      535       +4     
  Branches       67       68       +1     
==========================================
+ Hits          495      502       +7     
+ Misses         22       21       -1     
+ Partials       14       12       -2     

Impacted Files	Coverage Δ
formtools/wizard/views.py	95.36% <100.00%> (+1.06%)	:arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[MrkGrgsn]

This change will mask the implementation issues in the custom conditional form logic that have caused the recursion, silently returning form_list instead of whatever the developer intends which would be confusing. It would be better to raise an exception with a useful message when recursion is encountered so the developer knows what they have to fix.

It would be better still to avoid this infinite recursion problem altogether while fixing the behaviour that was broken in #209 🙂

[claudep]

I understand your feeling. Basically, there are two different uses of get_form/get_form_list. One to really get the forms depending on some conditionals, and one just to access a specific form to get data or some other information (typically from some of the conditions). We could have added a kwarg to these methods to be more explicit about those two use cases, but then there are backwards compatibility issues due to method signature changes. I'm clearly not 100% satisfied with that proposed solution, but it's a way to make things going forward. I'll probably merge this soon to prepare a new release, but if you have any ideas about a better fix for this, any contribution is welcome.

[MrkGrgsn]

I understand it's an awkward situation and you need to move it forward somehow!

I think this change is okay to merge as an improvement on the current situation if it raises an exception instead of returning self.form_list. If I'm upgrading and something fails I want it to fail loudly so it doesn't slip into production. Or if I'm implementing a new wizard with conditional forms, I want the success/failure of my new code to be clear while I test and debug. Returning self.forms would be unexpected and leave it to me to look into the wizard code and find out why. I think raising an exception would communicate the problem better.

What do you think about also adding a note to the documentation at https://django-formtools.readthedocs.io/en/latest/wizard.html#conditionally-view-skip-specific-steps. I guess the example provided there will trigger infinite recursion.

Regarding you comment that "Basically, there are two different uses of get_form/get_form_list.", well, there are now but there weren't before 209. Looking at the code prior to 209, the only use for get_form() was to naively instantiate a form from form_list, while the purpose of get_form_list() was to switch those forms on and off but not add new forms. The design limited how dynamic the form list could be (all possible forms needed to be listed in form_list) but at least it was a consistent and cohesive design.

Regarding a better fix, I'm going to spend some time on it today, hopefully I can add something more practical to the discussion. If the two form list generation strategies can't be merged coherently without further breaking changes, then I'm inclined toward a solution that keeps the two strategies separate and simpler, but is a breaking change for implementations using the more dynamic approach given that it is newer and presumably has fewer implementations to update.

[schinckel]

In terms of my workflow: the default is form_list, and as such a fallback to that if get_form_list() is indeed the desired behaviour.

[MrkGrgsn]

Coming back to this after attempting my own fix I've realised I was looking at this wrong way - apologies for the misguided commentary @claudep. I can see that this change allows adding forms in get_form_list(), restores previous behaviour, and is backwards compatible. It would be a valuable improvement.

I note that it doesn't support calling get_cleaned_data_for_step() on a step/form that has been added in get_form_list() during condition evaluation. I realise this is intended to be a small step forward so perhaps this is not the change to achieve such things; perhaps this is not even a bigger picture goal.

[jsma]

I understand your feeling. Basically, there are two different uses of get_form/get_form_list. One to really get the forms depending on some conditionals, and one just to access a specific form to get data or some other information (typically from some of the conditions). We could have added a kwarg to these methods to be more explicit about those two use cases, but then there are backwards compatibility issues due to method signature changes. I'm clearly not 100% satisfied with that proposed solution, but it's a way to make things going forward. I'll probably merge this soon to prepare a new release, but if you have any ideas about a better fix for this, any contribution is welcome.

The time for exercising caution about backwards compatibility was before #209 was merged. I do not understand the continued efforts to double down on the problems in 2.4 with even more hacks to make it "work" when reverting #209 is the cleanest and clearest path forward and could have been done half a year ago. No one would accept PRs like these in Django proper, which is where django-formtools originated.

get_form_list has exactly one documented and tested use case and that is to process condition_dict to prune steps/forms. It is unfortunately named since it does not actually operate on nor return a list (form_list is an OrderedDict at this point and that is what is returned). It should have been called process_conditions(), since that is all it does. Nevertheless, it seems that some have become fixated on this method since that was the only place they could figure out to hack into things in their own projects, but that does not mean this project needs to lower its standards of code quality and make these hacks "official".

Those that need this new feature need to make a serious and thought through proposal to add the appropriate hooks for their use case, in a way that either preserves backwards compatibility (which I don't think will be possible) or at least has a clear deprecation timeline for any internals that need to be refactored along the way.

[claudep]

I will nevertheless go forward with this patch, even if not the optimal solution. This package is blocked with this recursion regression for too long now. We can always improve later, and as usual patches welcome!