Story #2640

API server has functionality and test fixtures for folders

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

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

100%

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

Subtasks

Task #2621: Add & document group_class attribute on group resourceResolvedTom Clegg

Task #2641: Add test fixtures and tests demonstrating "object is in a folder"ResolvedTom Clegg

Task #2676: Review 2640-folder-api branchResolvedRadhika Chippada

Task #2704: Add unique constraint for names within folderResolvedTom Clegg

Task #2732: Refuse to create ownership cycles.ResolvedTom Clegg

Task #2724: Review (new) 2640-folder-api branchResolvedTom Clegg

Associated revisions

Revision d61f5659
Added by Tom Clegg over 7 years ago

Merge branch '2640-folder-api'

refs #2640

Conflicts:
services/api/app/controllers/application_controller.rb
services/api/config/routes.rb

History

#1 Updated by Tom Clegg over 7 years ago

  • Assigned To set to Tom Clegg

#2 Updated by Tim Pierce over 7 years ago

Code review for 2640-folder-api:

Is it possible for a user to create links that represent a circular hierarchy of folders? If not, should we try to protect against that?

  • apps/workbench/app/models/arvados_resource_list.rb
    • Is the behavior of links_for well-defined when link_class is nil? I see that's the default, and I assume that means "return no links" but I don't know if it's guaranteed that results.links will not include any links with a nil link_class.
  • services/api/app/controllers/application_controller.rb
    • Some confusing formatting in ApplicationController.contents - there is a large each do block that's not indented

#3 Updated by Tom Clegg over 7 years ago

Tim Pierce wrote:

Is it possible for a user to create links that represent a circular hierarchy of folders? If not, should we try to protect against that?

We don't guard against that (or any owner_uuid cycles) yet. We do detect cycles when traversing the hierarchy to find the ultimate owner user / top level directory. Disallowing owner_uuid cycles is probably a good idea but it might be OK to allow cycles with links -- much like POSIX which will happily make symlink cycles but discourages you from hard linking directories for this sort of reason.

Fixed @ 9887260 (prevents owner_uuid cycles, but not link cycles)

  • apps/workbench/app/models/arvados_resource_list.rb
    • Is the behavior of links_for well-defined when link_class is nil? I see that's the default, and I assume that means "return no links" but I don't know if it's guaranteed that results.links will not include any links with a nil link_class.

Fixed @ 3ea6744

  • services/api/app/controllers/application_controller.rb
    • Some confusing formatting in ApplicationController.contents - there is a large each do block that's not indented

Ah. The statement culminating in "each" was several lines long and 2..Nth lines get indented to the same level as the block innards.

Improved @ 10d03fd

#4 Updated by Tim Pierce over 7 years ago

Reviewing @ 76e20e69

  • apps/workbench/test/unit/arvados_resource_list_test.rb
    • Let's add an assertion to demonstrate that links_for returns multiple kinds of links when called without a link_class argument.
  • services/api/app/models/arvados_model.rb
    • ensure_owner_uuid_is_permitted
      • This method sometimes returns false and sometimes raises PermissionDeniedError - it would help to review those paths and clarify in comments why one is used over another.
  • services/api/test/unit/group_test.rb
    • test "cannot set owner_uuid to object with existing ownership cycle"
      • It's not immediately clear why g.name += " xyz" -- comment?

Ah. The statement culminating in "each" was several lines long and 2..Nth lines get indented to the same level as the block innards.

Sorry, I should have been more precise: I'm puzzled that the case...end statement is outdented to the same level as the enclosing each. It's not a huge deal and if for some reason that's the preferred indentation for an each block that encloses a case, or something, no sweat. It's just confusing to read at first.

#5 Updated by Tom Clegg over 7 years ago

Tim Pierce wrote:

Reviewing @ 76e20e69

  • apps/workbench/test/unit/arvados_resource_list_test.rb
    • Let's add an assertion to demonstrate that links_for returns multiple kinds of links when called without a link_class argument.

Fixed in 5af51b5

  • services/api/app/models/arvados_model.rb
    • ensure_owner_uuid_is_permitted
      • This method sometimes returns false and sometimes raises PermissionDeniedError - it would help to review those paths and clarify in comments why one is used over another.

Indeed. Fixed (always raise) in 8370640

  • services/api/test/unit/group_test.rb
    • test "cannot set owner_uuid to object with existing ownership cycle"
      • It's not immediately clear why g.name += " xyz" -- comment?

Fixed in e76d4b5

Ah. The statement culminating in "each" was several lines long and 2..Nth lines get indented to the same level as the block innards.

Sorry, I should have been more precise: I'm puzzled that the case...end statement is outdented to the same level as the enclosing each. It's not a huge deal and if for some reason that's the preferred indentation for an each block that encloses a case, or something, no sweat. It's just confusing to read at first.

I think the trouble is that the line-continuation indent is equal to the block indent:

    ArvadosModel.descendants.reject(&:abstract_class?).sort_by(&:to_s).
      each do |klass|
      case klass.to_s
      when ...
      end
    end

looks more reasonable as

    ArvadosModel.descendants.reject(&:abstract_class?).sort_by(&:to_s).each do |klass|
      case klass.to_s
      when ...
      end
    end

#6 Updated by Tim Pierce over 7 years ago

Tom Clegg wrote:

Tim Pierce wrote:

  • services/api/app/models/arvados_model.rb
    • ensure_owner_uuid_is_permitted
      • This method sometimes returns false and sometimes raises PermissionDeniedError - it would help to review those paths and clarify in comments why one is used over another.

Indeed. Fixed (always raise) in 8370640

Also raise if !current_user

Sorry, I should have been more precise: I'm puzzled that the case...end statement is outdented to the same level as the enclosing each. It's not a huge deal and if for some reason that's the preferred indentation for an each block that encloses a case, or something, no sweat. It's just confusing to read at first.

I think the trouble is that the line-continuation indent is equal to the block indent:

Ahh, the light dawns. Thanks for the explanation.

Otherwise, LGTM, go ahead and merge once that return/exception is resolved.

#7 Updated by Tom Clegg over 7 years ago

Tim Pierce wrote:

Also raise if !current_user

whops, fixed this time for sure

#8 Updated by Tom Clegg over 7 years ago

  • Status changed from New to Resolved

Also available in: Atom PDF