Feature #7709

[API] Upgrade API server to Rails 4.2

Added by Ward Vandewege over 2 years ago. Updated about 1 year ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
03/19/2017
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-

Description

Among other things, this will provide:
- 4.0 - encrypted session cookies, cf. http://api.rubyonrails.org/classes/ActionDispatch/Session/CookieStore.html
- 4.2 - jsonb column type (requires PostgreSQL 9.4)


Subtasks

Task #11319: Review 7709-sdk-cli-active_supportResolvedLucas Di Pentima

Task #11298: make tests pass in rails4ResolvedTom Clegg

Task #11297: update bundleResolvedTom Clegg

Task #11316: make other components' integration tests passResolvedTom Clegg

Task #11337: Check ruby warnings and Rails upgrade notesResolvedTom Clegg

Task #11264: Review 7709-api-rails4ResolvedLucas Di Pentima


Related issues

Related to Arvados - Bug #8222: [SDK] "gem install arvados" fails on ruby <2.2.2 since activesupport 5.0.0 came outResolved

Related to Arvados - Bug #11593: [API] [Workbench] config option serve_static_assets to serve_static_filesNew2017-05-01

Related to Arvados - Bug #11594: [API] [Wrokbench] set a default config.log_levelNew2017-05-01

Related to Arvados - Bug #11606: [CWL] [API] empty array submitted as nullResolved2017-05-02

Has duplicate Arvados - Task #9778: Upgrade API Server to Rails 4.2Closed2016-08-11

Blocks Arvados - Story #4019: [API] Support query of "properties" field on objectsResolved2017-12-12

Blocked by Arvados - Feature #10766: [Docs] [arvados-ws] make the arvados-ws documentation official, remove all mentions of the old puma websockets setupResolved2017-03-23

Associated revisions

Revision b50e323e
Added by Tom Clegg about 1 year ago

Merge branch '7709-sdk-cli-active_support'

refs #7709

Revision ff8d14ac
Added by Tom Clegg about 1 year ago

Merge branch '7709-api-rails4' (partial)

refs #7709
refs #11100

Revision 318c4900
Added by Tom Clegg about 1 year ago

Merge branch '7709-api-rails4'

closes #7709

Revision 6a7edde0
Added by Tom Clegg about 1 year ago

Merge branch '7709-rails-observers'

refs #7709

History

#1 Updated by Ward Vandewege over 2 years ago

  • Description updated (diff)

#2 Updated by Tom Morris over 1 year ago

  • Subject changed from [API] Upgrade API server to rails 4 to [API] Upgrade API server to Rails 4.2
  • Description updated (diff)

#4 Updated by Tom Morris over 1 year ago

  • Target version set to 2017-04-12 sprint
  • Story points set to 2.0

#5 Updated by Tom Morris over 1 year ago

  • Target version changed from 2017-04-12 sprint to 2017-03-29 sprint

#6 Updated by Tom Clegg over 1 year ago

  • Assigned To set to Tom Clegg

#7 Updated by Tom Clegg about 1 year ago

So far, the most annoying part has been making the websocket tests (or server?) work. Switching to arvados-ws and abandoning the Rails websocket server & tests might be a better use of time.

Other things that require non-trivial fixes
  • serialized attributes behave differently: Rails 4 wants to store [] and {} as NULL.
  • attr_protected is replaced by strong parameters

#8 Updated by Tom Clegg about 1 year ago

  • Status changed from New to In Progress

#9 Updated by Tom Clegg about 1 year ago

7709-sdk-cli-active_support @ 2fd606b328508babad9af6c0a30c159568b525c2

#10 Updated by Lucas Di Pentima about 1 year ago

LGTM, please merge

#11 Updated by Nico César about 1 year ago

some extra info: https://gist.github.com/nruth/a3bc1b75281109b036e4

we should do 4.2.5 because of Ubuntu16's default pg is 9.5

#12 Updated by Tom Clegg about 1 year ago

we should do 4.2.5 because of Ubuntu16's default pg is 9.5

(Done, and in the meantime we have a workaround for this issue, d31988b6da2d779a8f3ba4ba3c40760f9729c59f)

--

7709-api-rails4 @ 8d9b12f2a87ffd7183d3a36ca32ee1c7e701a0e2

#13 Updated by Lucas Di Pentima about 1 year ago

Phew, lots of reading! Some minor comments:

  • File services/api/app/controllers/application_controller.rb - Line 491: Was has_key?(...) replaced with key?(...) because it will be deprecated? If so, there’re some other places where it is being used.
  • Running tests locally, got this message:
    Use assert_nil if expecting nil from services/api/test/functional/arvados/v1/virtual_machines_controller_test.rb:62:in `block in <class:VirtualMachinesControllerTest>'. This will fail in MT6.
    

    ... there are some occurrences of assert_equal nil … that could be translated to assert_nil.
  • There are other mentions of the previous case, but the warning is not caused by an explicit use of assert_equal nil (not sure if they’re going to cause trouble), for example:
    Use assert_nil if expecting nil from services/api/test/unit/log_test.rb:41:in `assert_logged'. This will fail in MT6.
    
  • Another deprecation warning running tests:
    DEPRECATION WARNING: Passing a nested array to Active Record finder methods is deprecated and will be removed. Flatten your array before using it for 'IN' conditions. (called from readable_job_uuids at /home/lucas/arvados_local/services/api/app/controllers/arvados/v1/nodes_controller.rb:61)
    

Other than that, this LGTM. My local test run passed OK.

#14 Updated by Tom Clegg about 1 year ago

Lucas Di Pentima wrote:

  • File services/api/app/controllers/application_controller.rb - Line 491: Was has_key?(...) replaced with key?(...) because it will be deprecated? If so, there’re some other places where it is being used.

request.headers is an ActiveSomethingorother::Headers object rather than a plain Hash, and it didn't have has_key?. AFAIK there's no general trend away from has_key?.

Use assert_nil if expecting nil

Ugh, yeah. We have some parameterized tests where the expected value is a variable instead of a literal.

UserTest#test_no_username_set_when_no_base_available = Use assert_nil if expecting nil from /home/tom/src/arvados/services/api/test/unit/user_test.rb:70:in `check_new_username_setting'. This will fail in MT6.
0.03 s = .
  def check_new_username_setting(email_name, expect_name)
    set_user_from_auth :admin
    user = User.create!(email: "#{email_name}@example.org")
    assert_equal(expect_name, user.username)
  end

I've fixed the ones where use a literal nil, and added an assert_equal method that dispatches to assert_nil or super so we're compatible with MT6. 🙄

  • Another deprecation warning running tests:
    DEPRECATION WARNING: Passing a nested array to Active Record finder methods is deprecated and will be removed. Flatten your array before using it for 'IN' conditions. (called from readable_job_uuids at /home/lucas/arvados_local/services/api/app/controllers/arvados/v1/nodes_controller.rb:61)

Aha, looks like we were lazy with splat args. Fixed.

7709-api-rails4 @ e1e05845b74ce70712e414830f992ce57d7a8453

#15 Updated by Lucas Di Pentima about 1 year ago

This LGTM, thanks!

#16 Updated by Tom Clegg about 1 year ago

  • Target version changed from 2017-03-29 sprint to 2017-04-12 sprint

#17 Updated by Tom Clegg about 1 year ago

  • Story points deleted (2.0)

#18 Updated by Tom Clegg about 1 year ago

This happens sporadically

  1) Failure:
Arvados::V1::UsersControllerTest#test_setup_user_with_send_notification_param_false_and_verify_no_email [/home/tom/src/arvados/services/api/test/functional/arvados/v1/users_controller_test.rb:631]:
expected no setup email.
Expected #<Mail::Message:69977645172840, Multipart: false, Headers: <Date: Mon, 10 Apr 2017 13:24:48 -0400>, <From: arvados@example.com>, <To: arvados@example.com>, <Message-ID: <58ebbfe0df821_403f3fa4ecb3311854878@nelle.mail>>, <Subject: Profile created by >, <Mime-Version: 1.0>, <Content-Type: text/plain>, <Content-Transfer-Encoding: 7bit>> to be nil.

#19 Updated by Lucas Di Pentima about 1 year ago

Maybe the ActionMailer::Base.deliveries list is not cleared between tests, and if the tests are run in random order, that creates the sporadic failure.
I've found this post that may reflect the same problem: http://stackoverflow.com/questions/21423779/rspec-actionmailerbase-deliveries-should-be-empty

#20 Updated by Tom Clegg about 1 year ago

Yes, that fixed it. We were doing that in the User unit tests but never did it in the controller tests.

Replaced the websocket code with a stub that sends a 501 error with a link to the arvados-ws install docs, and then hangs up.

7709-api-rails4 @ de083a9fec0ca08afda5a9369c6cd32dbdcd0965

#21 Updated by Lucas Di Pentima about 1 year ago

Sorry for the delay, lgtm!

#22 Updated by Tom Clegg about 1 year ago

  • Target version changed from 2017-04-12 sprint to 2017-04-26 sprint

#23 Updated by Tom Morris about 1 year ago

  • Target version changed from 2017-04-26 sprint to 2017-05-10 sprint

#24 Updated by Tom Clegg about 1 year ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:318c49002aea966128a9d37ab29e601a104d79bb.

Also available in: Atom PDF