Item7146: Deep recursion bug in query search using formfield in form

This is Item6082 revived.

-- TWiki:Main/HideyoImazu - 2013-02-15

The culprit was revision 24909, where the lines fixing Item6082 have been (inadvertedly?) removed while implementing Item7140 or TWiki:Codev/VarSEARCHFormatTokenEncodeParam. Just re-applying the patch for Item6082 did the trick for me, but maybe the patch is incomplete with regard to the implementation of TWiki:Codev/VarSEARCHFormatTokenEncodeParam?

-- TWiki:Main.HaraldJoerg - 2014-06-24

Hideyo-san, could you please check on this?

See removed code at end of TWikirev:24909 - if re-added does that break the TWiki:Codev/VarSEARCHFormatTokenEncodeParam feature?

-- TWiki:Main.PeterThoeny - 2014-06-24

I see, The reason why I removed Item6082 fix back then was that it broke the feature to limit result length as in $formfield(Foo, 30). I'm taking a deeper look.

-- TWiki:Main.HideyoImazu - 2014-06-25

Is FormDefTests::test_Item6082() really a useful test? In that test, TestForm is defined/saved. And the definition has %SEARCH{"TestForm.Ecks~'Blah'"...}% as part of it. Why does it need to refer to topics having TestFrom? Why not %SEARCH{"TestSubForm.Ecks~'Blah'"...}% avoiding self-reference? I cannot think of a useful use case of self-referencing in a form definition. The test creates an artificial and unstable situation where if the SplodgeOne topic is edited, its Ecks field cannot keep the current value "SplodgeOne;Blah" because of the way TestForm is defined. The only option is "SplodgeOne;SplodgeOne;Blah".

Maybe making TWiki withstand this situation is a path to an industrial strength TWiki?

I think the practical way to deal with this situation is to detect a deep recursion proactively and shows an error instead of having Perl cause a deep recursion error.

What do you guys think?

-- TWiki:Main.HideyoImazu - 2014-06-25

Well... in the Old Days tests like test_Item6082 were, as the name suggests, intended to cover one single bug, Item6802. The benefit is that there's documentation about the reason for the test case, which is not always the case in unit tests. If you read Item6802, you learn about the real-world scenario where the problem had happened. As far as I am concerned, I understand the use case outlined in TWiki:Support.DeepRecursionUsingFormfieldInForm: The valid entries in a form field BladeCenter are those which are already present in another formfield MachineType of other topics, i.e. you only can add new machines to blade centers you've already added to your set. Cute. In one sentence I'd describe it as "allow to create a tree of related topics, without the chance to inadvertedly create orphans". You might well need a self-reference form for such an application since a topic can only have one form. The test case simplifies the use case but keeps the "problem" intact.

If I add the attachments of TWiki:Support.DeepRecursionUsingFormfieldInForm to my SVN Sandbox web and remove the 6082 patch, I get deep recursion. With the 6082 patch applied, it works as desired.

Please note also that the section you removed in Revision 24909 also contained a two-line fix for Item6167 which is not in the original patch of Item6082.

-- TWiki:Main.HaraldJoerg - 2014-06-25

OK. Let me study TWiki:Support.DeepRecursionUsingFormfieldInForm. But as I mentioned, the original fix for Item6082 introduced a bug, which has arguably a bigger impact than Item6082. As far as I tested, the current code doesn't have Item6167. I suspect Item6167 was caused by the half-baked nature of the original fix for Item6082. I'll look into it further too.

-- TWiki:Main.HideyoImazu - 2014-06-26

In the example shown in TWiki:Support.DeepRecursionUsingFormfieldInForm refers to the MachineType field in the definition of the BladeCenter field, which is not unstable. While test_Item6082() is using an unstable situation where the Ecks field definition refers to the Ecks field. So I kind of understand the validity of the example.

And now TWiki::Meta::renderFormFieldForDisplay() is enhanced not to cause infinite recursion. The issue is TWiki::Form::new() is called there and it ended up evaluating TWiki variables in value options. Since value options are not needed for displaying the current value, evaluation of value options can be and needs to be suppressed to avoid infinite recursion. As such, FormDefTests::test_Item6082() now passes cleanly.

I've confirmed Item6167 was introduced by the original fix of Item6082. Now that that original fix is gone, Item6167 doesn't exist. Before the original fix of Item6082, $n, $nop, etc. in a field value were taken care of by TWiki::Render::protectedFormFieldValue(). This is the case with the current code too.

-- TWiki:Main.HideyoImazu - 2014-06-26

Great! This fix looks much cleaner than the original patch to Item6082. The test case testItem6082 passes, and the example application from TWiki:Support.DeepRecursionUsingFormfieldInForm works fine for me, too. Thanks for a deeper look into the issue!

-- TWiki:Main.HaraldJoerg - 2014-06-26

Thank you Hideyo-san for fixing this properly!

-- TWiki:Main.PeterThoeny - 2014-06-26