Arvados

• In ArvadosModel#make_log_around, log.save should probably be log.save! so failure to log causes transaction rollback/fail, rather than silently missing logs.
• I don't see how we avoid (or test that we avoid) logging the creation of a (user-initiated) Log object
• We might want to skip logging on ApiClientAuthorization updates that only change last_used_at, because every single API call does that to the authorizing token. (Is there a reasonably easy way to skip that particular case?)
• New Log semantics (can't update or delete your own voluntarily-added logs) are a surprise, but arguably a good new feature. (Existing permission system would be enough to protect the automatically generated logs.)
  • is_admin==true should still be able to update and delete logs -- at least user-generated ones, if not all. Generally our approach is "if it doesn't break database consistency, is_admin lets you do it".
  • if permission_to_update/delete is false, setting owner_uuid=system_user_uuid has no effect other than making the log unreadable by the user who created it (which we don't want, do we?)
  • Special "can't update/delete" semantics should be mentioned at http://doc.arvados.org/api/methods/logs.html and possibly http://doc.arvados.org/api/permission-model.html

• In ArvadosModel#make_log_around, log.save should probably be log.save! so failure to log causes transaction rollback/fail, rather than silently missing logs.

I'm not sure which is stylistically better, but according to the docs I believe the current implementation will cause a rollback. Rails' callback documentation says that any callback can force a rollback by returning false, which log.save should do if it fails.

• I don't see how we avoid (or test that we avoid) logging the creation of a (user-initiated) Log object

I'll add a test, but if I'm following you right, I think the self.is_a? Log conditional in make_log_around covers this.

• New Log semantics (can't update or delete your own voluntarily-added logs) are a surprise, but arguably a good new feature. (Existing permission system would be enough to protect the automatically generated logs.)

Sorry for the surprise. I went there because I had to overwrite permission_to_create, and then figured overriding the others made sense for consistency's sake, and I didn't fully understand how that hooked into permission to view. If you're okay with this general approach, I'm happy to make the other changes you suggested.

Brett Smith wrote:

• In ArvadosModel#make_log_around, log.save should probably be log.save! so failure to log causes transaction rollback/fail, rather than silently missing logs.

I'm not sure which is stylistically better, but according to the docs I believe the current implementation will cause a rollback. Rails' callback documentation says that any callback can force a rollback by returning false, which log.save should do if it fails.

I see this:

The whole callback chain is wrapped in a transaction. If any before callback method returns exactly false or raises an exception, the execution chain gets halted and a ROLLBACK is issued; after callbacks can only accomplish that by raising an exception.

But this seems to say "before callbacks" do what you want, "after callbacks" don't do what you want, and "around callbacks" ... uh, might do what you want? (One of us is missing something...)

OTOH, raising an exception is almost certain (ha) to do what you want.

Perhaps the best answer involves a test case, although I can't think offhand of a convenient way for a test to make log.save fail.

• I don't see how we avoid (or test that we avoid) logging the creation of a (user-initiated) Log object

I'll add a test, but if I'm following you right, I think the self.is_a? Log conditional in make_log_around covers this.

Ah, indeed, I missed that. I was too busy looking for a skip_around_filter in the Log class. That works.

• New Log semantics (can't update or delete your own voluntarily-added logs) are a surprise, but arguably a good new feature. (Existing permission system would be enough to protect the automatically generated logs.)

Sorry for the surprise. I went there because I had to overwrite permission_to_create, and then figured overriding the others made sense for consistency's sake, and I didn't fully understand how that hooked into permission to view. If you're okay with this general approach, I'm happy to make the other changes you suggested.

Yes, I think it does make sense. Normally a logging facility is not expected to provide "update log" and "delete log" operations, right?

You're right about callbacks.

Perhaps the best answer involves a test case, although I can't think offhand of a convenient way for a test to make log.save fail.

Oh, that's easy, we can just redefine the right Log method for the test so it always fails. :)

I pushed a new version that I believe incorporates all your feedback. Please take a look and let me know what you think. Thanks again.

All looks good to me, thanks!

Oops, I just thought of one more suggestion: Don't log the api_token attribute when logging transactions on ApiClientAuthorizations -- or do something else to prevent "non-trusted" clients from trivially retrieving all of the user's tokens. (Currently if you look at workbench/logs after logging out+in, you can see secret tokens in log properties. Keeping these out of logs entirely is probably a reasonable thing to do, in any case...?)