2-3-stable - fixes regression 6147-find_or_create-via-has_many-fails-for-hash-parameters #207

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@yenif

yenif commented Mar 8, 2011

Fixes regression 6147 with tests. ActiveRecord::Associations::AssociationCollection#find_or_create_by_* should match behavior of ActiveRecord::Base#find_or_create_by_*, as well as it's behavior before 2.3.9.

@yenif

This comment has been minimized.

Show comment
Hide comment
@yenif

yenif Mar 11, 2011

Should I squash those into one commit? Not sure if it would confuse the github pull request.

yenif commented Mar 11, 2011

Should I squash those into one commit? Not sure if it would confuse the github pull request.

@snowmoonsoftware

This comment has been minimized.

Show comment
Hide comment
@snowmoonsoftware

snowmoonsoftware May 4, 2011

With this patch I still see a problem with find_or_create_by with multiple parameters:

p.niod_metrics.find_or_create_by_metric_type_id_and_month(666, Date.parse('2011-06-01'))

NiodMetric Load (0.5ms)   SELECT * FROM 'niod_metrics' WHERE ('niod_metrics'.'month' IS NULL AND 
'niod_metrics'.'metric_type_id' IN( 666,'2011-06-01' )) AND ('niod_metrics'.niodmetrics_id = 1750 AND 
'niod_metrics'.niodmetrics_type = 'Profile') LIMIT 1

NiodMetric Create (0.2ms)   INSERT INTO 'niod_metrics' ('created_at', 'month', 'updated_at', 'last_activity_at', 
'niodmetrics_type', 'value', 'metric_type_id', 'niodmetrics_id') VALUES('2011-05-04 17:10:44', '2011-06-01', 
'2011-05-04 17:10:44', NULL, 'Profile', NULL, 666, 1750)

The insert is correct, but the find isn't. This is blocking a move from rails 2.3.4 to 2.3.11.

With this patch I still see a problem with find_or_create_by with multiple parameters:

p.niod_metrics.find_or_create_by_metric_type_id_and_month(666, Date.parse('2011-06-01'))

NiodMetric Load (0.5ms)   SELECT * FROM 'niod_metrics' WHERE ('niod_metrics'.'month' IS NULL AND 
'niod_metrics'.'metric_type_id' IN( 666,'2011-06-01' )) AND ('niod_metrics'.niodmetrics_id = 1750 AND 
'niod_metrics'.niodmetrics_type = 'Profile') LIMIT 1

NiodMetric Create (0.2ms)   INSERT INTO 'niod_metrics' ('created_at', 'month', 'updated_at', 'last_activity_at', 
'niodmetrics_type', 'value', 'metric_type_id', 'niodmetrics_id') VALUES('2011-05-04 17:10:44', '2011-06-01', 
'2011-05-04 17:10:44', NULL, 'Profile', NULL, 666, 1750)

The insert is correct, but the find isn't. This is blocking a move from rails 2.3.4 to 2.3.11.

@yenif

This comment has been minimized.

Show comment
Hide comment
@yenif

yenif May 9, 2011

Hey snowmoonsoftware

I believe the issue you're seeing is unrelated to this patch. This patch only effects the insert statement. To find the root cause of the select being incorrect, you need to follow the calls through pull_finder_args_from(DynamicFinderMatch.match(method).attribute_names, *args) on line 383 of activerecord/lib/active_record/associations/association_collection.rb.

Thanks

yenif commented May 9, 2011

Hey snowmoonsoftware

I believe the issue you're seeing is unrelated to this patch. This patch only effects the insert statement. To find the root cause of the select being incorrect, you need to follow the calls through pull_finder_args_from(DynamicFinderMatch.match(method).attribute_names, *args) on line 383 of activerecord/lib/active_record/associations/association_collection.rb.

Thanks

@sikachu

This comment has been minimized.

Show comment
Hide comment
@sikachu

sikachu Jun 1, 2011

Member

While multiple commits are OK, the commit doesn't cleanly merged anymore. Do you mind rebased it and force-push to your own branch, so it would update the pull request?

(And yes, if you squashed them together that would be aw3som3.

Member

sikachu commented Jun 1, 2011

While multiple commits are OK, the commit doesn't cleanly merged anymore. Do you mind rebased it and force-push to your own branch, so it would update the pull request?

(And yes, if you squashed them together that would be aw3som3.

fix #6147 find_or_create via has_many fails for hash parameters
Incorporates tests from James Badger

Add tests for edge cases and match behavior
of ActiveRecord::Base#find_or_create_by_*

Made extraction of implicit and explicit attributes more apparent
@yenif

This comment has been minimized.

Show comment
Hide comment
@yenif

yenif Jun 8, 2011

Rebased and squashed.

Thanks

yenif commented Jun 8, 2011

Rebased and squashed.

Thanks

@jonleighton

This comment has been minimized.

Show comment
Hide comment
@jonleighton

jonleighton Jul 11, 2011

Member

Hi,

Thanks for this patch, but I am afraid that the 2-3-stable branch is not being released any more (unless there are serious security issues etc).

Perhaps you could consider packaging your fix as a plugin to help people make the migration to 3.X.

Jon

Member

jonleighton commented Jul 11, 2011

Hi,

Thanks for this patch, but I am afraid that the 2-3-stable branch is not being released any more (unless there are serious security issues etc).

Perhaps you could consider packaging your fix as a plugin to help people make the migration to 3.X.

Jon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment