Arvados

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

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

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.

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

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.

Tim Pierce wrote:

Also raise if !current_user

whops, fixed this time for sure