Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #9392 (closed defect: fixed)

Opened 1 year ago

Last modified 1 year ago

[PATCH] flaw in AR Validations validates_presence_of

Reported by: sur Assigned to: core
Priority: normal Milestone: 1.x
Component: ActiveRecord Version: edge
Severity: normal Keywords: active_record, validations, validates_presence_of, verified
Cc: tarmo

Description

The method to add errors on "validates_presence_of" validation directive i.e. the method "add_on_blank" is accepting array of attributes and iterating over them to push the error messages. There is unnecessary loop in the method "validates_presence_of" which is sending one by one all attributes from the array specified in the mode and there is nothing in the loop which is specific for a particular attribute to be executed infact it seems that it is redundantly evaluating the conditions using "unless (configuration[:if] && !evaluate_condition(configuration[:if], record)) (configuration[:unless] && evaluate_condition(configuration[:unless], record))".

Attachments

ar_flaw_validates_presence_of_fixed.diff (1.2 kB) - added by sur on 08/26/07 11:59:54.
patch fixing the flaw

Change History

08/26/07 11:59:54 changed by sur

  • attachment ar_flaw_validates_presence_of_fixed.diff added.

patch fixing the flaw

08/26/07 22:10:55 changed by tarmo

  • cc set to tarmo.

Nice catch. Tests pass, patch looks good.

+1

08/27/07 05:03:12 changed by sur

Just noticed the inconsistency of singular and plural in the called and the calling method as attr_name and attributes

08/27/07 05:04:16 changed by sur

  • keywords changed from active_record, validations, validates_presence_of to active_record, validations, validates_presence_of, verified.

08/27/07 12:38:45 changed by sur

  • summary changed from flaw in AR Validations validates_presence_of to [PATCH] flaw in AR Validations validates_presence_of.

08/27/07 16:30:45 changed by lifofifo

grrr..Nice catch.

+1

08/27/07 17:04:00 changed by sur

  • priority changed from normal to high.
  • severity changed from normal to major.

I think this patch doesn't need any test case to get committed.

08/27/07 17:08:49 changed by tarmo

  • priority changed from high to normal.
  • severity changed from major to normal.

I don't think removing redundant code is a high priority, not is it a sever defect especially as the tests that work with the new code also worked with the old code.

major severity is for things that can cause data loss or security breach.

08/28/07 23:15:51 changed by nzkoz

  • status changed from new to closed.
  • resolution set to fixed.

(In [7362]) Remove unnecessary loop in validates_presence_of. [sur] Closes #9392

08/29/07 07:21:07 changed by sur

Thanks for accepting that