Arvados

Migration up+down loses data. If anything goes wrong, we can't just deploy an old version; everything is screwed until we fix all the bugs (or hastily write a real down-migration). Either

• Make the down-migration restore the contents of the fields to what they were before using regexp or something, instead of leaving them null, or
• Leave the database fields there (but generate the API responses automatically instead of using the db contents) until we have deployed and we're confident everything works.

In Workbench, "Combine selected collections" → "Can't mass-assign protected attributes: tail_kind, head_kind". grep -r reveals a bunch of *_kind stuff in apps/workbench and crunch_scripts/pgp-survey-import. (Should merge master, then try grep; new stuff in 2187.)

We should (at least) ignore provided *_kind in create/update, so old clients/jobs work. Crashing when receiving *_kind makes jobs unrepeatable if people used *_kind like we told them to in their scripts (or just ran our example script).

As a convenience to the client, we should still provide *_kind in API responses. If it's easy for the client to deduce the type, then it's even easier for the server. (I think this is still a polite API feature even aside from the issue of breaking existing clients.)

We could conceivably break existing clients anyway by not supporting {where:{foo_kind:"bar"}} but at least (afaik) we haven't distributed example code and existing jobs that rely on that.

As an aside, perhaps we should figure out a deprecation approach -- e.g., include warnings in API responses, and make SDKs print them on stderr?

In arvados_model.rb: Don't send a stack trace to the client. If you're expecting something in particular here, catch the appropriate exception type and send a helpful error message. Otherwise, get rid of this rescue block and rely on the existing mechanism for unexpected exceptions.

     rescue Exception => e
       bt = e.backtrace.join("\n")
       errors.add(attr, "'#{attr_value}' error '#{e}'\n#{bt}\n")
     end

(Aside: e.backtrace can be nil when you don't know what kind of exception you're catching...)

Migration broken

• Table name :log should be :logs

LinksController#load_kind_params is confusing me a bit.

• In what circumstance is params[:tail_uuid] expected to be non-nil? AFAICT this code block is untested/unused, and it looks kind of broken.
• This whole filter could go in ApplicationController after the existing "load_where_param" and "load_filters_param" filters, and avoid duplicating the JSON-decoding logic. This version looks like it misses various cases like params[:filters] arriving as JSON.

filters: ['tail_kind','=','arvados#user'] used to work, throws 422 now

Some more notes

• Probably a mistake to remove arvados-cli dependency from Gemfile.
• Error checking has become obfuscated here. The first raise here no longer checks whether Oj.load returns an Array. Assuming nobody else sets @filters to a non-Array before load_filters_param, you could just remove the first raise entirely. (Checking Oj.load's return value before appending it to @filters would also make sense.)
  

  -        @filters = Oj.load params[:filters]
  +        @filters += Oj.load params[:filters]
           raise unless @filters.is_a? Array
         rescue
           raise ArgumentError.new("Could not parse \"filters\" param as an array")
  

• Remove #puts @response.body etc. in services/api/test/integration/valid_links_test.rb (maybe we should add checks for this in our git commit hooks?)
• Noticed we still don't get the sanity check the story asked for ("reject if _kind was provided but does not match _uuid"). But this branch does other worthwhile things and doesn't need to block on that. (We can revisit that part some other day if we still want it, and do something generic that applies to all _uuid fields.)
• "API server tests pass, workbench tests are failing" is a rather lackluster summary of what happened in that giant commit. Maybe worth rewording before merge?

• Reverted accidental change to Gemfile.
• Restored parameter check in load_filters_param.
• Removed spurious commented-out code from test.
• Added check in links controller that _kind matches uuid.kind

The commit message that cc5023d40182e503e8ba109fc86e09efd6337836 should have had (I'm pasting this in here because otherwise I'd have to spend hours learning git how to use git rebase to change a buried commit...)

Added logic to LinksController to provide backwards compatibilty with where: tail_kind and head_kind
Fixed bug in user controller where response was not returned as_api_response.
Added Email model object as a hack to stand in for head_uuid and/or tail_uuid fields that store email
addresses.
Added virtual attributes head_kind and tail_kind to Link model.
Down migration restores head_kind and tail_kind column contents.
Added tests that virtual head_kind and tail_kind attributes are returned and searchable.
Fixed tests.

Need to "bundle install" to get Gemfile.lock back to where it was.

Other than that, I'm done.