Project

General

Profile

Actions

Feature #7709

closed

[API] Upgrade API server to Rails 4.2

Added by Ward Vandewege over 7 years ago. Updated almost 6 years 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 6 (0 open6 closed)

Task #11319: Review 7709-sdk-cli-active_supportResolvedLucas Di Pentima03/19/2017

Actions
Task #11298: make tests pass in rails4ResolvedTom Clegg03/19/2017

Actions
Task #11297: update bundleResolvedTom Clegg03/19/2017

Actions
Task #11316: make other components' integration tests passResolvedTom Clegg03/19/2017

Actions
Task #11337: Check ruby warnings and Rails upgrade notesResolvedTom Clegg03/19/2017

Actions
Task #11264: Review 7709-api-rails4ResolvedLucas Di Pentima03/27/2017

Actions

Related issues

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

Actions
Related to Arvados - Bug #11593: [API] [Workbench] config option serve_static_assets to serve_static_filesNew05/01/2017

Actions
Related to Arvados - Bug #11594: [API] [Workbench] set a default config.log_levelResolvedWard Vandewege05/01/2017

Actions
Related to Arvados - Bug #11606: [CWL] [API] empty array submitted as nullResolvedTom Clegg05/02/2017

Actions
Has duplicate Arvados - Task #9778: Upgrade API Server to Rails 4.2Closed08/11/2016

Actions
Blocks Arvados - Story #4019: [API] Support query of "properties" field on objectsResolvedPeter Amstutz12/12/2017

Actions
Blocked by Arvados - Feature #10766: [Docs] [arvados-ws] make the arvados-ws documentation official, remove all mentions of the old puma websockets setupResolvedTom Clegg03/23/2017

Actions
Actions #1

Updated by Ward Vandewege over 7 years ago

  • Description updated (diff)
Actions #2

Updated by Tom Morris about 6 years ago

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

Updated by Tom Morris almost 6 years ago

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

Updated by Tom Morris almost 6 years ago

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

Updated by Tom Clegg almost 6 years ago

  • Assigned To set to Tom Clegg
Actions #7

Updated by Tom Clegg almost 6 years 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
Actions #8

Updated by Tom Clegg almost 6 years ago

  • Status changed from New to In Progress
Actions #9

Updated by Tom Clegg almost 6 years ago

7709-sdk-cli-active_support @ 2fd606b328508babad9af6c0a30c159568b525c2

Actions #10

Updated by Lucas Di Pentima almost 6 years ago

LGTM, please merge

Actions #11

Updated by Nico César almost 6 years 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

Actions #12

Updated by Tom Clegg almost 6 years 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
Actions #13

Updated by Lucas Di Pentima almost 6 years 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.

Actions #14

Updated by Tom Clegg almost 6 years 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

Actions #15

Updated by Lucas Di Pentima almost 6 years ago

This LGTM, thanks!

Actions #16

Updated by Tom Clegg almost 6 years ago

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

Updated by Tom Clegg almost 6 years ago

  • Story points deleted (2.0)
Actions #18

Updated by Tom Clegg almost 6 years 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.
Actions #19

Updated by Lucas Di Pentima almost 6 years 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

Actions #20

Updated by Tom Clegg almost 6 years 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

Actions #21

Updated by Lucas Di Pentima almost 6 years ago

Sorry for the delay, lgtm!

Actions #22

Updated by Tom Clegg almost 6 years ago

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

Updated by Tom Morris almost 6 years ago

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

Updated by Tom Clegg almost 6 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:318c49002aea966128a9d37ab29e601a104d79bb.

Actions

Also available in: Atom PDF