Feature #7709

[API] Upgrade API server to Rails 4.2

Added by Ward Vandewege over 1 year ago. Updated 14 days ago.

Status:In ProgressStart date:03/19/2017
Priority:NormalDue date:
Assignee:Tom Clegg% Done:

100%

Category:-
Target version:2017-04-26 sprint
Story points-Remaining (hours)0.00 hour
Velocity based estimate-

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 ac... Resolved
Duplicated by Arvados - Task #9778: Upgrade API Server to Rails 4.2 Closed 08/11/2016
Blocks Arvados - Story #4019: [API] Support query of "properties" field on objects New
Blocked by Arvados - Feature #10766: [Docs] [arvados-ws] make the arvados-ws documentation off... Resolved 03/23/2017

Associated revisions

Revision b50e323e
Added by Tom Clegg about 1 month ago

Merge branch '7709-sdk-cli-active_support'

refs #7709

Revision ff8d14ac
Added by Tom Clegg 27 days ago

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

refs #7709
refs #11100

History

#1 Updated by Ward Vandewege over 1 year ago

  • Description updated (diff)

#2 Updated by Tom Morris 5 months 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 about 1 month ago

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

#5 Updated by Tom Morris about 1 month ago

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

#6 Updated by Tom Clegg about 1 month ago

  • Assignee set to Tom Clegg

#7 Updated by Tom Clegg about 1 month 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 month ago

  • Status changed from New to In Progress

#9 Updated by Tom Clegg about 1 month ago

7709-sdk-cli-active_support @ 2fd606b328508babad9af6c0a30c159568b525c2

#10 Updated by Lucas Di Pentima about 1 month ago

LGTM, please merge

#11 Updated by Nico César about 1 month 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 30 days 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 29 days 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 29 days 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 29 days ago

This LGTM, thanks!

#16 Updated by Tom Clegg 28 days ago

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

#17 Updated by Tom Clegg 28 days ago

  • Story points deleted (2.0)

#18 Updated by Tom Clegg 16 days 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 16 days 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 15 days 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 15 days ago

Sorry for the delay, lgtm!

#22 Updated by Tom Clegg 14 days ago

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

Also available in: Atom PDF