Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ability to reject deeply nested blank attributes #23001

Closed
wants to merge 1 commit into from
Closed

Ability to reject deeply nested blank attributes #23001

wants to merge 1 commit into from

Conversation

ignat-z
Copy link
Contributor

@ignat-z ignat-z commented Jan 11, 2016

Allow passing :nested_all_blank as argument for accepts_nested_attributes_for to reject fully blank attributes by recursion traversing of all hashes in attributes.

Can be used to filter blank attributes if model depends more than one level of nesting.
Related to #19490

Using recursive traverse instead of stack because of:

Calculating -------------------------------------
           recursion     5.320k i/100ms
               stack     3.905k i/100ms
-------------------------------------------------
           recursion     58.193k (± 3.7%) i/s -    292.600k
               stack     40.710k (± 3.3%) i/s -    206.965k

Comparison:
           recursion:    58193.3 i/s
               stack:    40710.1 i/s - 1.43x slower

Calculating -------------------------------------
recursion with value     8.036k i/100ms
    stack with value     4.658k i/100ms
-------------------------------------------------
recursion with value     87.259k (± 3.8%) i/s -    441.980k
    stack with value     48.642k (± 3.5%) i/s -    246.874k

Comparison:
recursion with value:    87259.5 i/s
    stack with value:    48642.5 i/s - 1.79x slower

source

Allow to pass :nested_all_blank to reject fully blank attributes by recursion
traverse of all hashes in attributes.
@sgrif
Copy link
Contributor

sgrif commented Jan 11, 2016

Thank you for the feature proposal, but I do not think there is a compelling enough use case for this change in the framework itself. Perhaps you could make this a gem, and see if there's interest.

@sgrif sgrif closed this Jan 11, 2016
@siakaramalegos
Copy link

👍

@ignat-z
Copy link
Contributor Author

ignat-z commented Jan 23, 2016

If somebody meets the same problem
https://github.com/ignat-zakrevsky/reject_deeply_nested

@danielricecodes
Copy link
Contributor

danielricecodes commented Aug 30, 2017

@sgrif - IMHO the Rails framework should offer support for rejecting deeply nested attributes. It is a framework feature to accept_nested_attributes_for in the first place. Shouldn't the Rails framework feature work when you stack accept_nested_attributes_for declarations and your ActiveRecord models are related using Rails associations like has_many, has_one, or belongs_to?

Seems pretty simple to do really since all that this would required is a slight tweak to this proc

To illustrate what a deeply nested blank Param's hash looks like, here is an example of one. Rails really should consider this not blank???

{"first_name"=>"",
 "middle_name"=>"",
 "last_name"=>"",
 "suffix"=>"",
 "home_address_attributes"=>{"street1"=>"", "street2"=>"", "county"=>"", "city"=>"", "state"=>"", "zipcode"=>""},
 "mailing_address_attributes"=>{"street1"=>"", "street2"=>"", "county"=>"", "city"=>"", "state"=>"", "zipcode"=>""},
 "home_phone_number"=>"",
 "mobile_phone_number"=>"",
 "email_address"=>"",
 "at_address_years"=>"",
 "at_address_months"=>"",
 "monthly_mortgage"=>"",
 "home_ownership"=>"",
 "date_of_birth"=>"",
 "ssn"=>"",
 "drivers_license_id_number"=>"",
 "drivers_license_state"=>"",
 "drivers_licence_expires_at"=>"",
 "employer_name"=>"",
 "employment_address_attributes"=>{"city"=>"", "state"=>""},
 "time_at_employer_years"=>"",
 "time_at_employer_months"=>"",
 "job_title"=>"",
 "employment_status"=>"",
 "employer_phone_number"=>"",
 "gross_monthly_income"=>"",
 "other_monthly_income"=>""}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants