Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #1993 (closed defect: wontfix)

Opened 11 years ago

Last modified 10 years ago

[PATCH] @validate recursion guard revisited

Reported by: aspineux Owned by:
Priority: normal Milestone: 1.1.x bugfix
Component: TurboGears Version: 1.0.7
Severity: normal Keywords: validate, recursion, needs tests
Cc:

Description

To avoid recursion in @validate, the function limit its use to one per HTTP request. This patch limit the number of call to @validate to an arbitrary value, here 15.

Attachments

validate_recursion.diff Download (637 bytes) - added by aspineux 11 years ago.

Change History

Changed 11 years ago by aspineux

comment:2 Changed 11 years ago by faide

  • Owner faide deleted

comment:3 Changed 11 years ago by faide

  • Summary changed from @validate recursion guard revisited + PATCH to [PATCH] @validate recursion guard revisited

comment:4 follow-up: ↓ 5 Changed 11 years ago by Chris Arndt

  • Keywords recursion, needs tests added; recursion removed

If we agree that this is a good approach (I'm not sure about it), the maximum number of calls to validate should be a config setting.

Also there should be a source code comment that explains what's going here.

Finally, if the patch is accepted, this should be documented on  http://docs.turbogears.org/1.0/ValidateDecorator (or the 1.1 equivalent).

comment:5 in reply to: ↑ 4 Changed 11 years ago by aspineux

Replying to Chris Arndt:

If we agree that this is a good approach (I'm not sure about it), the maximum number of calls to validate should be a config setting.

I agree, what about

validate.recursion_limit=15

Also there should be a source code comment that explains what's going here.

What about :

This is a simple test to avoid recursion. Instead of trying to stop recursion by checking function name like in http://trac.turbogears.org/ticket/1396 or simply limiting the number of call into one HTTP request to 1 like in previous version, it allow limited call to @validate per request.

Finally, if the patch is accepted, this should be documented on  http://docs.turbogears.org/1.0/ValidateDecorator (or the 1.1 equivalent).

in "Beware of the Recursion Guard", I don't know if "1" is still relevant, but 2 could be replaced by :

2) The number of use of the validate decorator is limited by option "validate.recursion_limit". Increase the the value if needed by your application. The default is 15.

Finally, I dont understand the utility of this test. I just provided a patch because my app was not working anymore (I was cascading @validate). If the app allow @validate recursion, this is a bug in the application design, isn't it ? And then just raise an exception, not just silently ignore the @validate !!!

My 2ct.

Alain

comment:6 Changed 10 years ago by faide

  • Milestone changed from 1.1 to 1.1 maintenance

comment:7 Changed 10 years ago by Chris Arndt

  • Status changed from new to closed
  • Resolution set to wontfix

Though I agree the vaildation limit is a wart in TurboGears, changing it as suggested here would break backward compatibility and I don't like the approach of the patch anyway, I would rather have a more general solution for this isse. So I'm closing this ticket as wontfix and have opened a new one (#2366) to generally discuss the issue of the validation limit.

Note: See TracTickets for help on using tickets.