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

Do not fetch attribute values for excluded models #350

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Gasparila
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Related Issue N/A
Need Doc update no

Describe your change

Upon upgrading from 1.20 => 1.22 we noticed errors due to changes around
handling excluded records. This PR implements a fix by only fetching
attribute names when determining if an index operation is necessary and
adds a test to ensure that our use case will not break. Example model

class Example < ActiveRecord::Base
  include AlgoliaSearch

  algoliasearch :unless => :nil_name do
    attribute :name_length do
      # will crash if name is nil
      name.length
    end
  end

  def nil_name
    name.nil?
  end
end

Upon upgrading from 1.20 => 1.22 we noticed errors due to changes around
handling excluded records. This PR implements a fix by only fetching
attribute names when determining if an index operation is necessary and
adds a test to ensure that our use case will not break. Example model

```rb
class Example < ActiveRecord::Base
  include AlgoliaSearch

  algoliasearch :unless => :nil_name do
    attribute :name_length do
      # will crash if name is nil
      name.length
    end
  end

  def nil_name
    name.nil?
  end
end
```
@Gasparila
Copy link
Contributor Author

@julienbourdeau any thoughts on this?

@Gasparila Gasparila mentioned this pull request May 10, 2019
@julienbourdeau
Copy link
Contributor

@Gasparila I think it's a good improvement 👍

I don't see what caused the issue between 1.20 and 1.22 tho 🤔 did you track it down?

@Gasparila
Copy link
Contributor Author

@julienbourdeau yes I did

af11c8f#diff-dfaec67e907148ea8211e17340f0f30cR149

Changed the get_attribute_names to get get_attributes(object).keys instead
of the previous behavior that would only look at the names. As a result, the values
are computed when we are fetching the names

@Gasparila
Copy link
Contributor Author

Any update on this @julienbourdeau? We'd love to move back onto the main fork of the repo

@bkdir
Copy link

bkdir commented Feb 21, 2020

Curious if this is a consideration? Calculating values just to get names causing unexpected errors.

else
{}
end
end

def get_attributes(object)
def get_unresolved_attributes(object)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line 164 would still resolve your attributes. should be changed as @serializer._attributes to get attribute names without actually calculating the values.

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.

3 participants