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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Upgrade to rails master/4.2 #2784

Merged
merged 12 commits into from Nov 10, 2014

Conversation

5 participants
@chancancode
Copy link
Contributor

chancancode commented Sep 11, 2014

I was going to send this when I have the complete set of patches required, but I noticed @SamSaffron has started working on it too, so we should probably share notes 馃槃

@discoursebot

This comment has been minimized.

Copy link

discoursebot commented Sep 11, 2014

You've signed the CLA, chancancode. Thank you! This pull request is ready for review.

@SamSaffron

This comment has been minimized.

Copy link
Member

SamSaffron commented Sep 12, 2014

I mainly did a quick and dirty thing there cause I needed to confirm a bug on master.

rails/rails#14696 (comment)

Your change looks way more complete, let me know when its ready to merge.

@michaelkirk

View changes

Gemfile_master.lock Outdated
rspec-expectations (3.0.4)
diff-lcs (>= 1.2.0, < 2.0)
rspec-support (~> 3.0.0)
rspec (2.14.1)

This comment has been minimized.

@michaelkirk

michaelkirk Sep 27, 2014

Contributor

Why roll back rspec?

This comment has been minimized.

@SamSaffron

SamSaffron Sep 27, 2014

Member

I upgraded to 2.99 there are a pile of deprecations notices that still need
fixing. any PRs to help fix it up welcome.

3.0 was totally broken so I rolled back.

On Sat, Sep 27, 2014 at 1:00 PM, Michael Kirk notifications@github.com
wrote:

In Gemfile_master.lock:

@@ -338,29 +333,25 @@ GEM
netrc (~> 0.7)
rinku (1.7.3)
rmmseg-cpp (0.2.9)

  • rspec (3.0.0)
  •  rspec-core (~> 3.0.0)
    
  •  rspec-expectations (~> 3.0.0)
    
  •  rspec-mocks (~> 3.0.0)
    
  • rspec-core (3.0.4)
  •  rspec-support (~> 3.0.0)
    
  • rspec-expectations (3.0.4)
  •  diff-lcs (>= 1.2.0, < 2.0)
    
  •  rspec-support (~> 3.0.0)
    
  • rspec (2.14.1)

Why roll back rspec?


Reply to this email directly or view it on GitHub
https://github.com/discourse/discourse/pull/2784/files#r18121497.

@chancancode chancancode force-pushed the chancancode:upgrade-to-rails-master-4-2 branch Oct 15, 2014

@chancancode

This comment has been minimized.

Copy link
Contributor Author

chancancode commented Oct 15, 2014

zomg, so close, 6 more failures to go!

Failures:

  1) Search Chinese search finds chinese topic based on title
     Failure/Error: Search.execute('绀惧尯鎸囧崡').posts.first.id.should == post.id
     NoMethodError:
       undefined method `id' for nil:NilClass
     # ./spec/components/search_spec.rb:299:in `block (3 levels) in <top (required)>'

  2) Admin::SiteCustomizationsController while logged in as an admin  .index returns JSON
     Failure/Error: ::JSON.parse(response.body).should be_present
       expected present? to return true, got false
     # ./spec/controllers/admin/site_customizations_controller_spec.rb:23:in `block (4 levels) in <top (required)>'

  3) TopicsController has_escaped_fragment? when the SiteSetting is enabled uses the application layout when there's no param
     Failure/Error: assert_select "meta[name=fragment]", true, "it has the meta tag"
     Minitest::Assertion:
       it has the meta tag.
       Expected 0 to be >= 1.
     # ./spec/controllers/application_controller_spec.rb:50:in `block (4 levels) in <top (required)>'

  4) TopicsController crawler when not a crawler renders with the application layout
     Failure/Error: assert_select "meta[name=fragment]", true, "it has the meta tag"
     Minitest::Assertion:
       it has the meta tag.
       Expected 0 to be >= 1.
     # ./spec/controllers/application_controller_spec.rb:71:in `block (4 levels) in <top (required)>'

  5) TopicLink internal links extracts onebox
     Failure/Error: TopicLink.extract_from(post)
     ActiveRecord::StatementInvalid:
       PG::Error: ERROR:  value "9999999999" is out of range for type integer
       : SELECT  "topics".* FROM "topics" WHERE ("topics"."deleted_at" IS NULL) AND "topics"."id" = $1 LIMIT 1
     # ./app/models/topic_link.rb:132:in `block (2 levels) in extract_from'
     # ./app/models/topic_link.rb:110:in `each'
     # ./app/models/topic_link.rb:110:in `block in extract_from'
     # ./app/models/topic_link.rb:100:in `extract_from'
     # ./spec/models/topic_link_spec.rb:65:in `block (3 levels) in <top (required)>'

  6) pool drainer can drain with idle time setting
     Failure/Error: pool.drain(1.minute)
     NoMethodError:
       undefined method `last_use' for #<ActiveRecord::ConnectionAdapters::PostgreSQLAdapter:0x007faa0822a830>
     # ./lib/freedom_patches/pool_drainer.rb:24:in `try_drain?'
     # ./lib/freedom_patches/pool_drainer.rb:10:in `block (2 levels) in drain'
     # ./lib/freedom_patches/pool_drainer.rb:9:in `delete_if'
     # ./lib/freedom_patches/pool_drainer.rb:9:in `block in drain'
     # ./lib/freedom_patches/pool_drainer.rb:7:in `drain'
     # ./spec/components/freedom_patches/pool_drainer_spec.rb:35:in `block (2 levels) in <top (required)>'

Finished in 8 minutes 8 seconds
3900 examples, 6 failures, 3 pending

Failed examples:

rspec ./spec/components/search_spec.rb:294 # Search Chinese search finds chinese topic based on title
rspec ./spec/controllers/admin/site_customizations_controller_spec.rb:21 # Admin::SiteCustomizationsController while logged in as an admin  .index returns JSON
rspec ./spec/controllers/application_controller_spec.rb:47 # TopicsController has_escaped_fragment? when the SiteSetting is enabled uses the application layout when there's no param
rspec ./spec/controllers/application_controller_spec.rb:68 # TopicsController crawler when not a crawler renders with the application layout
rspec ./spec/models/topic_link_spec.rb:53 # TopicLink internal links extracts onebox
rspec ./spec/components/freedom_patches/pool_drainer_spec.rb:23 # pool drainer can drain with idle time setting

@SamSaffron it looks like we have one failure for the pool drainer

@chancancode

This comment has been minimized.

Copy link
Contributor Author

chancancode commented Oct 15, 2014

Ignore failure 1 from above, that's always been failing for me, probably just that my machine is not configured correctly (@SamSaffron do you know why?).

I thought 1b892e908409ad950f50114881ec49cd87e5f282 would fix failure 5, but apparently not. @SamSaffron is it important that the invalid URL is in that format, or it just have to be any invalid URL? (It's probably failing now because the find_by call wasn't using prepared statements before and it is now. Anyhow, the error it raises seems reasonable, so I suggest that we fix the spec.)

@chancancode chancancode force-pushed the chancancode:upgrade-to-rails-master-4-2 branch Oct 16, 2014

@chancancode

This comment has been minimized.

Copy link
Contributor Author

chancancode commented Oct 16, 2014

Resolved 3/4/5 from above, 2 left! (ignoring 1)

@chancancode

View changes

app/views/user_notifications/digest.text.erb Outdated
@@ -36,7 +36,6 @@

<%=raw(t :'user_notifications.digest.unsubscribe',
site_link: site_link,
unsubscribe_link: raw(@markdown_linker.create(t('user_notifications.digest.click_here'), email_unsubscribe_path(key: @user.temporary_key)))) %>
unsubscribe_link: raw(@markdown_linker.create(t('user_notifications.digest.click_here'), email_unsubscribe_url(host: Discourse.base_url, key: @user.temporary_key, only_path: true)))) %>

This comment has been minimized.

@chancancode

chancancode Oct 16, 2014

Author Contributor

@pixeltrix Sorry I force-pushed and lost your comment. :( To recap, this change is needed because...

  1. Using email_unsubscribe_path produces a deprecation warning
  2. Using email_unsubscribe_url without the host option raises an error, even with only_path: true

As for why they want a path here, it's because that the host is dynamic, and to avoid duplicating Discourse.base_url in every call they extracted it into a helper which takes a patch and "glues" the host plus other formatting stuff onto it, which doesn't seem like an unreasonable thing to do.

cc @fxn @schneems @matthewd @sgrif again

This comment has been minimized.

@sgrif

sgrif Oct 16, 2014

Can this be accomplished with default_url_options?

This comment has been minimized.

@sgrif

sgrif Oct 16, 2014

I feel like we had to do something similar on a past project and had a solution that wasn't completely terrible. @derekprior do you remember what it was?

This comment has been minimized.

@chancancode

chancancode Oct 16, 2014

Author Contributor

Possibly, I can't tell for sure though because I don't know what Discourse.base_url actually depends on...

My point is that, if our intention of the deprecation is to warn people "you probably don't want this (using paths in mailers)" to protect 90% of the users, we should provide an easy alternative for those who want to say "yes I am sure and I have a valid reason for using paths" to do that explicitly.

Allowing *_url(..., only_path: true) without a host option would work for me, if it's feasible. Not sure why we are still complaining about the missing host when we know that we are only generating a path?

This comment has been minimized.

@chancancode

chancancode Oct 17, 2014

Author Contributor

Actually, this is a regression.

The error message I was mentioning is:

     ActionView::Template::Error:
       Missing host to link to! Please provide the :host parameter, set default_url_options[:host], or set :only_path to true

As you can see, the error message suggests that you add only_path: true to make it go away, but it doesn't do that and still complain about the missing host option.

So, I think it's supposed to work the way I proposed, but it's currently broken. I confirmed that it was working fine on 4.1, so it's a regression on master.

This comment has been minimized.

@chancancode

chancancode Oct 17, 2014

Author Contributor

FWIW, I "just" changed the implementation here: chancancode@a66c3fe

Moving the discussion to rails/rails#17294

@chancancode

This comment has been minimized.

Copy link
Contributor Author

chancancode commented Oct 16, 2014

@SamSaffron In rails/rails#14360 last_use was removed from AR, I don't know enough about the context to fix it, can you check? (From what I can tell, that patch has the same goal as your patch)

@chancancode chancancode force-pushed the chancancode:upgrade-to-rails-master-4-2 branch 3 times, most recently Oct 16, 2014

@SamSaffron

This comment has been minimized.

Copy link
Member

SamSaffron commented Oct 17, 2014

#1 is a postgres installation issue http://stackoverflow.com/questions/24461059/to-tsvector-in-simple-mode-throwing-away-non-english-in-some-setups postgresapp has bugs, works fine on ubuntu

the connection pool monkey patch is super critical, its better to get it into AR now. just involves exposing some APIs. https://github.com/discourse/discourse/blob/master/lib/freedom_patches/pool_drainer.rb

In fact strongly recommend we review all the freedom patches to ensure we get stuff into rails that belongs in rails.

@chancancode

This comment has been minimized.

Copy link
Contributor Author

chancancode commented Oct 17, 2014

@SamSaffron does rails/rails#14360 not address the same issue that the pool drainer solves, but in an alternative way? (I am not sure, but from the description in the PR, it sounds like it)

@chancancode

This comment has been minimized.

Copy link
Contributor Author

chancancode commented Oct 17, 2014

Ah, I noticed you commented there, so I guess not 馃槃

@chancancode chancancode force-pushed the chancancode:upgrade-to-rails-master-4-2 branch 4 times, most recently Oct 25, 2014

@chancancode

This comment has been minimized.

Copy link
Contributor Author

chancancode commented Oct 27, 2014

It's done! 馃槃 There are current a few broken tests because of rails/rails#17294, once we fix that on Rails side all should be well.

@SamSaffron would you consider adding RAILS_MASTER=1 to the build matrix once this is merged? (By the way, one annoying thing I had to do is keeping the Gemfile.lock in sync 鈥 when we upgrade a gem we need to do that with RAILS_MASTER=1 as well.)

cc @rafaelfranca

@chancancode chancancode force-pushed the chancancode:upgrade-to-rails-master-4-2 branch Oct 28, 2014

@chancancode

This comment has been minimized.

Copy link
Contributor Author

chancancode commented Oct 28, 2014

@SamSaffron last error fixed on Rails side via rails/rails@aa1fadd.

All green now!

@SamSaffron

This comment has been minimized.

Copy link
Member

SamSaffron commented Oct 28, 2014

AWESOME 鉂わ笍 鉂わ笍

I don't mind if you add master into the travis test matrix.

@SamSaffron

View changes

app/models/topic_link.rb Outdated
@@ -129,7 +129,11 @@ def self.extract_from(post)
post_number = route[:post_number] || 1

# Store the canonical URL
topic = Topic.find_by(id: topic_id)
if rails_master?
topic = Topic.find_by(id: topic_id) rescue nil

This comment has been minimized.

@SamSaffron

SamSaffron Oct 28, 2014

Member

this looks fishy, why is find_by changing ?

This comment has been minimized.

@chancancode

chancancode Oct 28, 2014

Author Contributor

wait, something is wrong, I thought I force pushed this away... let me check

This comment has been minimized.

@chancancode

chancancode Oct 28, 2014

Author Contributor

By the way, this is because of rails/rails#17380. But the "fix" totally doesn't work, which is why I changed the test instead.

@@ -68,7 +68,7 @@
<span class='footer-notice'>
<%=raw(t :'user_notifications.digest.unsubscribe',
site_link: html_site_link,
unsubscribe_link: link_to(t('user_notifications.digest.click_here'), email_unsubscribe_path(host: Discourse.base_url, key: @user.temporary_key, only_path: false))) %>
unsubscribe_link: link_to(t('user_notifications.digest.click_here'), email_unsubscribe_url(host: Discourse.base_url, key: @user.temporary_key))) %>

This comment has been minimized.

@SamSaffron

SamSaffron Oct 28, 2014

Member

are we certain this will not break?

This comment has been minimized.

@chancancode

chancancode Oct 28, 2014

Author Contributor

Why would it? :) We are generating a URL here, so we should use the url helper (doing _path(... only_path: false) is deprecated in the commit I linked in the comment)

@@ -115,7 +115,7 @@ def send
end

begin
@message.deliver
@message.deliver_now

This comment has been minimized.

@SamSaffron

SamSaffron Oct 28, 2014

Member

this is a good change api wise

@SamSaffron

View changes

lib/sass/discourse_sass_compiler.rb Outdated
# As far as I can tell, sprockets has always expected the third argument to
# be a Pathname object, not sure why this was working before
if rails_master?
pathname = Pathname.new("app/assets/stylesheets/#{@target}.scss")

This comment has been minimized.

@SamSaffron

SamSaffron Oct 28, 2014

Member

does this branch in the code need to exist?

This comment has been minimized.

@chancancode

chancancode Oct 28, 2014

Author Contributor

Do you mean that we could just always convert the string into a Pathname? Yeah I believe we can do that

This comment has been minimized.

@SamSaffron

SamSaffron Oct 28, 2014

Member

yeah just have it be a pathname unconditionally

@@ -93,7 +102,11 @@ def map_exec(klass = OpenStruct, args = {})
setters.each_with_index do |mapper, index|
translated = row[index]
if mapper[1] && !translated.nil?
translated = ActiveRecord::ConnectionAdapters::Column.send mapper[1], translated
if rails_master?
translated = mapper[1].type_cast_from_database(translated)

This comment has been minimized.

@SamSaffron

@chancancode chancancode force-pushed the chancancode:upgrade-to-rails-master-4-2 branch Oct 28, 2014

@chancancode

This comment has been minimized.

Copy link
Contributor Author

chancancode commented Oct 28, 2014

@SamSaffron fixed up the issues, and Travis should now be running it with RAILS_MASTER=1 for the first time 馃槃

@chancancode

This comment has been minimized.

Copy link
Contributor Author

chancancode commented Oct 28, 2014

The sha would be locked down in Gemfile_master.lock though, so I'm not sure how useful that ultimately is (shrug)

@chancancode chancancode force-pushed the chancancode:upgrade-to-rails-master-4-2 branch Oct 28, 2014

@chancancode

This comment has been minimized.

Copy link
Contributor Author

chancancode commented Oct 28, 2014

@SamSaffron I am getting this failure ocasionally: https://travis-ci.org/discourse/discourse/jobs/39328830

Is my monkey patch not working?

The failure is on this line, so it looks like the pool started out with 2 connections before the test manuiplated it... is that expected?

@SamSaffron

This comment has been minimized.

Copy link
Member

SamSaffron commented Oct 29, 2014

not sure, but I have never seen that failure before.

On Wed, Oct 29, 2014 at 10:48 AM, Godfrey Chan notifications@github.com
wrote:

@SamSaffron https://github.com/SamSaffron I am getting this failure
ocasionally: https://travis-ci.org/discourse/discourse/jobs/39328830

Is my monkey patch not working?


Reply to this email directly or view it on GitHub
#2784 (comment).

@SamSaffron

This comment has been minimized.

Copy link
Member

SamSaffron commented Nov 9, 2014

@chancancode now that we got 1.1 out the door I think this is a great time to merge it in... but :( its no longer merging cleanly, can you help us get it ready so I can pull it in.

happy to do so today.

@chancancode

This comment has been minimized.

Copy link
Contributor Author

chancancode commented Nov 10, 2014

Working through it now 馃槃

@chancancode chancancode force-pushed the chancancode:upgrade-to-rails-master-4-2 branch 3 times, most recently Nov 10, 2014

@chancancode

This comment has been minimized.

Copy link
Contributor Author

chancancode commented Nov 10, 2014

@SamSaffron Just fixed the last breakage via rails/rails@aa6637d, should be all green now!

Besides fixing the "HAX" commits, there are two other things to deal with:

  1. There are a ton of deprecation warnings about named_routes.helpers. This is coming from rspec-rails and has already been fixed on the 3.x series. So we "just" have to finish upgrading to rspec 3 to get rid of that.

  2. There is another deprecation warning:

    DEPRECATION WARNING: Currently, Active Record suppresses errors raised within `after_rollback`/`after_commit` callbacks and only print them to the logs. In the next version, these errors will no longer be suppressed. Instead, the errors will propagate normally just like in other Active Record callbacks.
    
    You can opt into the new behavior and remove this warning by setting:
    
     config.active_record.raise_in_transactional_callbacks = true
    
    (called from rate_limit at /Users/godfrey/Projects/oss/discourse/lib/rate_limiter/on_create_record.rb:47)
    

    This one appears to be more involved 鈥 I tried simply adding the config but that seems to be causing failures. (See here... it's a lot of errors!)

However, we have some time (until master points to Rails 5.0) to fix these. Hopefully getting this branch merged into discourse master would help motivate other contributors to investigate and fix these problems 馃榿

Let me know if you need anything else!

@chancancode

This comment has been minimized.

Copy link
Contributor Author

chancancode commented Nov 10, 2014

Actually, @SamSaffron it would be good if you can review my monkey patch for the pool drainer stuff as well. I'm still getting flaky/random failures on travis (vs this build, for example) about that, although I can't reproduce that locally. Since you said that feature critical to discourse, it would probably be a good to get to the bottom of this.

chancancode added some commits Aug 18, 2014

Use Rails 4.2+ API for typecasting on master
Note: this is still considered a private (internal) API on Rails side and is
subject to change in the future.
Boolean -> String quoting has changed on Rails master
CustomField.create(name: 'zomg', value: true).reload.value # => 't' on Rails 4.1, '1' on 4.2

rails/rails@42be84b
HAX: force the lazy `MessageDelivery` object to create the mailer
Starting from Rails 4.2, calling MyMailer.some_method no longer result in an
immediate call to MyMailer#some_method. Instead, a "lazy proxy" is returned
(this is changed to support #deliver_later). As a quick hack to fix the test,
calling #message (or anything, really) would force the Mailer object to be
created and the method invoked.
`Sprockets::Context` takes a `Pathname` for its third argument
As far as I can tell, sprockets has always expected the third argument to be a
`Pathname` object. Not sure what actually changed and why it was working before,
but it now throws a `NoMethodError` for `dirname` on `String` in
`Context#resolve`

    # , not sure why this was working before
HAX: check the `message` object, not the `MessageDelivery` object
See 669bf73 for background. It's probably better to rewrite these test without
using the internal `NullMail` class anyway.
Upgrade to the latest AMS 0.8.x (unreleased)
The current released version (0.8.2) does not work with Rails master at all. In
fact, it was quite surprising to me that this is the only test that broke...

See rails-api/active_model_serializers#655

@chancancode chancancode force-pushed the chancancode:upgrade-to-rails-master-4-2 branch to 5cc9f5b Nov 10, 2014

@SamSaffron

This comment has been minimized.

Copy link
Member

SamSaffron commented Nov 10, 2014

I see, will have a look at it

On Mon, Nov 10, 2014 at 8:00 PM, Godfrey Chan notifications@github.com
wrote:

Actually, @SamSaffron https://github.com/SamSaffron it would be good if
you can review my monkey patch for the pool drainer stuff as well. I'm
still getting flaky/random failures on travis
https://travis-ci.org/discourse/discourse/jobs/40518702 about that,
although I can't reproduce that locally. Since you said that feature
critical to discourse, it would probably be a good to get to the bottom of
this.


Reply to this email directly or view it on GitHub
#2784 (comment).

@SamSaffron

This comment has been minimized.

Copy link
Member

SamSaffron commented Nov 10, 2014

Pulling this in for now and running some benches first.

We can do more PRs to get the open issues sorted, thanks heaps @chancancode

SamSaffron added a commit that referenced this pull request Nov 10, 2014

@SamSaffron SamSaffron merged commit 1a775aa into discourse:master Nov 10, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment