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

to_param incorrectly returns nil for an unpersisted object #804

Closed
jcoyne opened this issue Jun 26, 2018 · 8 comments
Closed

to_param incorrectly returns nil for an unpersisted object #804

jcoyne opened this issue Jun 26, 2018 · 8 comments
Labels
waiting-for-user Waiting for user input to proceed

Comments

@jcoyne
Copy link

jcoyne commented Jun 26, 2018

On an ActiveRecord model (see User below), calling to_param on an unsaved object returns it's key. This is part of ActiveRecord::Integration however on Elasticsearch::Persistence::Model calling to_param on an unsaved object returns nil. It would be nice if Elasticsearch::Persistence::Model matched the behavior of ActiveRecord::Integration: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/integration.rb#L49-L52

class Document
    include Elasticsearch::Persistence::Model
end

class User < ActiveRecord::Base
end


Document.new(id:'123').to_param
nil
> u.to_param
"123"
> User.new(id: '123').to_param
"123"
> Document.new(id:'123').method(:to_param).source_location
["/Users/jcoyne/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/gems/activemodel-5.2.0/lib/active_model/conversion.rb", 82]
> User.new(id:'123').method(:to_param).source_location
["/Users/jcoyne/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/gems/activerecord-5.2.0/lib/active_record/integration.rb", 49]
jcoyne added a commit to projectblacklight/blacklight that referenced this issue Jun 26, 2018
This makes it possible to use with Elasticsearch due to:
elastic/elasticsearch-rails#804
jcoyne added a commit to projectblacklight/blacklight that referenced this issue Jun 27, 2018
This makes it possible to use with Elasticsearch due to:
elastic/elasticsearch-rails#804
@estolfo
Copy link
Contributor

estolfo commented Jun 29, 2018

Hi @jcoyne

I'm sorry you're running into this inconsistency between ActiveRecord::Base and Elasticsearch::Persistence's implementation of the ActiveRecord pattern.

Typically, we would work to fix bugs in the elasticsearch gems but we have plans to deprecate the ActiveRecord pattern (Elasticsearch::Persistence::Model) in the next major version, 6.0.

I recommend checking out the Repository pattern in the Elasticsearch::Persistence gem, as we will focus our future development efforts there.

If you don't mind, I'd like to close this issue as it's unlikely we will work on it. Thanks for your understanding.
Emily

@estolfo estolfo added the waiting-for-user Waiting for user input to proceed label Jul 2, 2018
@estolfo
Copy link
Contributor

estolfo commented Jul 6, 2018

Hi @jcoyne, I'm going to close this issue. Please open a new one if you have further questions. Thanks

@estolfo estolfo closed this as completed Jul 6, 2018
@jcoyne
Copy link
Author

jcoyne commented Jul 6, 2018

I am using the repository pattern. But I’d still like the to_param to behave consistently in order to use the rails route helpers.

@estolfo
Copy link
Contributor

estolfo commented Jul 9, 2018

Hi @jcoyne, in your example above, you've included the Elasticsearch::Persistence::Model module so can you please provide an example using the Repository pattern and what the expected behavior is? I'm reopening this issue so we can continue looking into it. Thanks!

@estolfo estolfo reopened this Jul 9, 2018
@jcoyne
Copy link
Author

jcoyne commented Jul 12, 2018

Sure. I'm retrieving an object like this:

repo = Elasticsearch::Persistence::Repository.new do
         klass Document
       end
@doc = repo.find(id)

Now, when I want to route to a controller to handle that document using the rails routing helpers I am doing:

  <%= link_to 'More info', @doc %>

In order to do that latter part, you need to include Elasticsearch::Persistence::Model on Document so that Document will respond to to_param, right?

@estolfo
Copy link
Contributor

estolfo commented Jul 12, 2018

@jcoyne You can also define the #to_param method yourself on the model (Document) to return the id. Would that be a solution for you?

@estolfo
Copy link
Contributor

estolfo commented Jul 18, 2018

@jcoyne I just wanted to check in and see if you could define #to_param on the model?

@jcoyne
Copy link
Author

jcoyne commented Jul 19, 2018

Yeah, I can add all that ActiveModel stuff myself. Thanks for following up.

@estolfo estolfo closed this as completed Jul 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-for-user Waiting for user input to proceed
Projects
None yet
Development

No branches or pull requests

2 participants