Skip to content
This repository

find_or_create dynamic finder doesn't use type attribute when creating an instance #7845

Closed
ytaras opened this Issue · 7 comments

6 participants

Yura Taras Yves Senn Steve Klabnik BrandonMathis Jon Leighton Andrew White
Yura Taras
class Company < ActiveRecord::Base; end
class Firm < Company; end
class Client < Company; end
Company.find_or_create_by_type_and_name('Firm', 'Google') 
# Returns instance of Company when 
# running first time and instance of Firm when running second time

The issue here is that find_or_create isn't smart enough to figure out that 'type' is inheritance_column and should be used for newly created record instantiation

Yves Senn
Collaborator

@ytaras to clarify, the first time it runs, it creates a Company instance with the type set to Firm and persists correctly and when running the second time (retrieving the record) it works as expected.

The behavior you want to change is that it creates a Firm on the first call?

Yura Taras

Yes, I understand why it's working like that right now, anyway I think it should create instance of inherited type instead. Otherwise this code:
Company.where(type: 'Firm', name: 'SomeFirm').first_or_create
has to be changed with this one:
Company.where(type: 'Firm', name: 'SomeFirm').first || Firm.create(name: 'SomeFirm')
to have consistent behavior between create and load scenario

Yves Senn
Collaborator

I see, just wanted to make sure that I understand the problem. I'll investigate and report back. I think this could be troublesome to implement though.

Yves Senn senny referenced this issue from a commit in senny/rails
Yves Senn senny failing test case for #7845 4d7870d
Yves Senn
Collaborator

I added a failing test case to illustrate the issue. Since first_or_create simply delegates the work to create this becomes more an issue of: Company.create(:type => 'Firm') should this result in a Company or a Firm?
supposed this would respect the inheritance_column and create a Firm, in order to keep the API consistent we would then have to make Company.new(:type => 'Firm') to also return a Firm. This seems silly.

I think the current functionality is fine. You should add code to your application, which resolves the class from the type string so that you can ask the right class for first_or_create

if ['Company', 'Firm'].include?(params[:type])
  klass = params[:type].constantize
  klass.where(:name => 'Google').first_or_create
end

I think we should close this issue and not consider this as a bug.

@steveklabnik @rafaelfranca what is your opinion?

Steve Klabnik
Collaborator

I personally think this is not a bug, but I'd rather get someone else to chime in as well.

BrandonMathis

I understand the issue and why you would want Rails' STI to behave like this but Company.create returning anything except for an instance of Company kinda sends off alarms in my head. I would shy away from this and use the solution that @senny has detailed. I don't feel like this is a bug, rails is giving you exactly what you have asked for which is a Company whose type is "Firm".

It feels like you are fighting the framework instead of letting it work for you. If you want to persist some instance of Firm then call create on that class instead of modifying the type column directly. STI can be hard to work with and modifying the type column just makes it harder (in my experience).

Jon Leighton
Owner

I also don't consider this a bug. I would write a factory method in Company to implement this behaviour in my application. The risk of supporting magic like this is that certain users won't expect it and will either trip up or worse, introduce a security issue in their application.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.