Feature #2228

In create and update, assign/check _kind attributes by inspecting corresponding _uuid.

Added by Tom Clegg almost 7 years ago. Updated over 6 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Start date:
03/24/2014
Due date:
% Done:

100%

Estimated time:
(Total: 8.00 h)
Story points:
1.0

Description

Including
  • Reject _kind if it doesn't match _uuid.
  • Reject _uuid if _kind is arvados#someType and the referenced object is not visible to the current user.
Acceptance
  • Test cases (e.g., right _kind, wrong _kind, no _kind, nonexistent referent)
  • Remove unnecessary *_kind in tutorial/doc examples
  • Explain in auto generated API docs that *_kind is populated automatically

Subtasks

Task #2298: Reject mismatched _kind/_uuid pairs in before_create/update hooks in ArvadosModelClosedPeter Amstutz

Task #2299: Fill in _type automatically if not provided and corresponding _uuid is providedClosedPeter Amstutz

Task #2304: Reject create/update if _uuid is an arvados object but does not map to a real object that is visible to the current userResolvedPeter Amstutz

Task #2418: Add "is_a" to filtersResolvedPeter Amstutz

Task #2436: Remove _kind columnsResolvedPeter Amstutz

Task #2303: Remove now-unnecessary _kind values in tutorials/docs/examplesResolvedPeter Amstutz

Task #2417: Write testsResolvedPeter Amstutz

Task #2311: Check that owner_uuid exists (even though it has no corresponding _kind)ResolvedPeter Amstutz


Related issues

Blocked by Arvados - Task #2440: Review 2228-check-filter-uuid-columnsResolved03/26/2014

History

#1 Updated by Tom Clegg almost 7 years ago

  • Description updated (diff)

#2 Updated by Tom Clegg almost 7 years ago

  • Story points set to 1.0

#3 Updated by Tom Clegg almost 7 years ago

  • Description updated (diff)

#4 Updated by Tom Clegg almost 7 years ago

  • Target version changed from 2014-05-07 Storing and Organizing Data to 2014-03-26 Debt service and dev painkillers

#5 Updated by Tom Clegg over 6 years ago

  • Status changed from New to In Progress

#6 Updated by Tom Clegg over 6 years ago

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?

#7 Updated by Tom Clegg over 6 years ago

  • Target version changed from 2014-03-26 Debt service and dev painkillers to 2014-04-16 Dev tools and data/resource management

#8 Updated by Tom Clegg over 6 years ago

  • Assigned To set to Peter Amstutz

#9 Updated by Tom Clegg over 6 years ago

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

#10 Updated by Peter Amstutz over 6 years ago

  • Assigned To changed from Peter Amstutz to Tom Clegg
  • Removed 'rescue' clause. I think I left that in from debugging.
  • Fixed :log -> :logs
  • Refactored to remove load_kind_params filter and instead override load_where_param and load_filters_param in the links and logs controllers to add the _kind functionality.
  • filters: ['tail_kind','=','arvados#user'] works
  • Added more tests.
  • Added equivalent logic and tests for logs controller for object_kind

#11 Updated by Peter Amstutz over 6 years ago

  • Assigned To changed from Tom Clegg to Peter Amstutz

#12 Updated by Tom Clegg over 6 years ago

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?

#13 Updated by Peter Amstutz over 6 years ago

  • 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.

#14 Updated by Tom Clegg over 6 years ago

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

Other than that, I'm done.

#15 Updated by Peter Amstutz over 6 years ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF