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

Upgrade Rails to 5.2.3 #1408

Merged
merged 14 commits into from Apr 3, 2019
Merged

Upgrade Rails to 5.2.3 #1408

merged 14 commits into from Apr 3, 2019

Conversation

rhymes
Copy link
Contributor

@rhymes rhymes commented Dec 27, 2018

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Documentation Update

Description

Inspired by #477 I've tried to bump Rails to the latest version, 5.2.2.

In addition to bumping Rails and its dependencies I've upgraded act-as-taggable-on to 6.0 as @greysteil did earlier, because it's the first version in the changelog to support Rails 5.2: https://github.com/mbleigh/acts-as-taggable-on/blob/master/CHANGELOG.md

Lastly, I ran rails app:update to upgrade all config files (adding also bootsnap). The update process added code for active storage, a new disabled initializer for content security policy and the framework defaults that in the future can be switched on to mimick Rails 5.2 defaults.

I'm currently, locally, fighting an error with Algolia's indexing in the tests with Rails 5.2. Upgrading Algolia's gems to the latest versions does not fix the issue.

If you have suggestions, I'm all hears.

I've also encountered a possible bug (yet to be determined) in Rails 5.2.2 and the handling of the Cache-Control header disallowing public, no-cache as a value. See rails/rails#34780.

There's work to do to bring dev.to's source to 5.2 but I think it can be done :)

Related Tickets & Documents

Added to documentation?

  • docs.dev.to
  • readme
  • no documentation needed

@maestromac
Copy link
Member

This is awesome @rhymes ! Can I see what error you are experiencing with Algolia?

@rhymes
Copy link
Contributor Author

rhymes commented Dec 27, 2018

@maestromac I ran rspec -b ./spec/requests/chat_channel_memberships_spec.rb:15

The short error is

  1) ChatChannelMemberships POST /chat_channel_memberships creates chat channel invitation
     Failure/Error: cache_name = "user-#{id}-#{updated_at}/followed_tag_names"
     
     ActiveModel::MissingAttributeError:
       missing attribute: updated_at
     # ./app/models/user.rb:264:in `cached_followed_tag_names'
     # ./app/models/user.rb:486:in `tag_list'
     # ./app/controllers/chat_channel_memberships_controller.rb:12:in `create'
     # ./spec/requests/chat_channel_memberships_spec.rb:18:in `block (3 levels) in <top (required)>'
     # ./spec/rails_helper.rb:57:in `block (3 levels) in <top (required)>'
     # ./spec/rails_helper.rb:57:in `block (2 levels) in <top (required)>'

The long error is here: ccm.txt

It's triggered by this line: https://github.com/rhymes/dev.to/blob/rhymes/rails-522/app/controllers/chat_channel_memberships_controller.rb#L12

@rhymes
Copy link
Contributor Author

rhymes commented Jan 8, 2019

Apparently they fixed the bug I submitted - rails/rails#34780 - with this PR - rails/rails#34885

🎉

@maestromac
Copy link
Member

As per @rhymes , we'll wait for the next minor release of Rails 5.2

@rhymes rhymes changed the title [WIP] Upgrade Rails to 5.2.2 [WIP] Upgrade Rails to 5.2.3 Mar 26, 2019
@rhymes
Copy link
Contributor Author

rhymes commented Mar 27, 2019

@maestromac I'm giving you an update:

If you have any ideas let me know.

A side note: the update changed config/puma.rb usage of MAX_THREADS to RAILS_MAX_THREADS which is the new env variable used by Rails 5.2+. This would require adding a new env variable to your Heroku's instances, otherwise we can just tell Ruby to check both env vars before booting up :)

@rhymes
Copy link
Contributor Author

rhymes commented Mar 27, 2019

I re-ran bundle exec rspec -b ./spec/requests/chat_channel_memberships_spec.rb:15 after having commented out https://github.com/rhymes/dev.to/blob/05132a7cc7e21c971611de9491830299fbc07c4c/app/controllers/chat_channel_memberships_controller.rb#L12-L13 and the test passed, so at least now I'm sure the issue is somewhere inside AlgoliaSearch-Rails.

I wonder if this closed algoliasearch-rails issue has something to do with what is happening, it's a russian doll of callbacks and monkey patching, one of those few things I dislike about Rails gems :D

I'll keep hunting for the bug. I reckon I should try to trigger it in development mode and not just during the test to see if it fails there as well

@rhymes
Copy link
Contributor Author

rhymes commented Mar 27, 2019

Ok I've narrowed it down to these lines that I executed in the Rails console:

cc = ChatChannel.last
cc.index! # this works

ccm = ChatChannelMembership.create(user_id: User.first.id, chat_channel_id: cc.id, status: "pending")
cc.index! # this breaks

ccm.destroy!
cc.index! # this works

Basically, indexing the ChatChannel after creating a pending membership breaks, it works before, it works after I destroy the membership. I've also tried using the synchronous = true param for index!

🤷‍♂️

@rhymes
Copy link
Contributor Author

rhymes commented Mar 27, 2019

I think I'm close to the solution and I think an exception is masked by another one:

on master the serialization of the dynamic field pending_users_select_fields works correctly, with Rails 5.2.3 it doesn't.

I've narrowed it down more to https://github.com/algolia/algoliasearch-rails/blob/d389875f5c07b7e405eb0786d10f6738bff1c2f1/lib/algoliasearch-rails.rb#L173 that calls:

Hash[attributes.map { |name, value| [name.to_s, value.call(object) ] }]

attributes is a hash of Procs, created by Algolia from the defined attributes in the ChatChannel model. When this method calls value.call(object) (object is the ChatChannel instance, value is the proc inside Algolia which in turn calls:

  def pending_users_select_fields
    pending_users.select(:id, :username, :name)
  end

inside chat_channel.rb

On master that select produces:

#<ActiveRecord::AssociationRelation [#<User id: 1, name: "Byron Abshire", username: "byron_abshire">]>

ends up in the hash to be serialized and goes on with its business. On Rails 5.2.3 the same select returns a different type of object:

#<User::ActiveRecord_AssociationRelation:0x00007fbd6f2c3b00>

which seems to break the association.

As a confirmation, if I remove pending_users_select_fields from the indexable attributes, everything works fine!!

I'm definitely close to the solution :D

UPDATE: this made it work for me locally - 52a5834 - apparently the indexed relationship needs to know about updated_at as well, otherwise when it deserializes it breaks. I'm not still 100% sure about what goes on at each step (Rails magic I guess 😅) but the fixed worked

@maestromac
Copy link
Member

@rhymes 👏 how did you figure out that it needs updated_at?

@rhymes
Copy link
Contributor Author

rhymes commented Mar 27, 2019

@maestromac just a hunch - I was lost in the rabbit hole of serialization/deserialization, then I went to eat something and I got the idea :D

On a related note: I think they recently changed how relationships in Algolia are supposed to work https://www.algolia.com/doc/framework-integration/rails/indexing/working-with-relationship/

@rhymes
Copy link
Contributor Author

rhymes commented Mar 28, 2019

@maestromac Rais 5.2.3 is officially out :-)

https://weblog.rubyonrails.org/2019/3/28/Rails-5-2-3-has-been-released/

@rhymes rhymes changed the title [WIP] Upgrade Rails to 5.2.3 Upgrade Rails to 5.2.3 Mar 28, 2019
@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Mar 28, 2019
Copy link
Member

@maestromac maestromac left a comment

Choose a reason for hiding this comment

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

Got local development up and running. Thanks @rhymes !!

@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Apr 3, 2019
@maestromac maestromac removed the blocked blocked by internal or external issues label Apr 3, 2019
@maestromac maestromac merged commit 3a53d57 into forem:master Apr 3, 2019
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Apr 3, 2019
@rhymes
Copy link
Contributor Author

rhymes commented Apr 3, 2019

Woot, woot! Thanks @maestromac - we've worked on this branch for three months :D

@maestromac
Copy link
Member

and thank you @rhymes for pushing to the finish line!

@rhymes rhymes deleted the rhymes/rails-522 branch April 3, 2019 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants