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

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

yenif
Copy link

@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
Copy link
Author

yenif commented Mar 11, 2011

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

@paquettej
Copy link

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
Copy link
Author

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
Copy link
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.

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
Copy link
Author

yenif commented Jun 8, 2011

Rebased and squashed.

Thanks

@jonleighton
Copy link
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

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

Successfully merging this pull request may close these issues.

None yet

4 participants