Project

General

Profile

Actions

Bug #16826

closed

AuditLogs.UnloggedAttributes is being ignored

Added by Lucas Di Pentima over 3 years ago. Updated over 3 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Target version:
Story points:
-
Release relationship:
Auto

Description

When adding "manifest_text" to this config list, collection's manifests are still being logged.
A customer reported that the issue is fixed by using strings on keys instead of symbols on ArvadosModel::logged_attributes.


Subtasks 1 (0 open1 closed)

Task #16835: Review 16826-unlogged-attrs-fixResolvedLucas Di Pentima09/11/2020Actions
Actions #1

Updated by Lucas Di Pentima over 3 years ago

  • Release set to 25
Actions #2

Updated by Lucas Di Pentima over 3 years ago

  • Description updated (diff)
Actions #3

Updated by Lucas Di Pentima over 3 years ago

Fix at c8a640d52 - branch 16826-unlogged-attrs-fix
Test run: developer-run-tests: #2089

The problem was caused by the fact that the ActiveSupport::OrderedOptions class we use on the config.yml loading code always return keys as symbols even if they're assigned as strings:

2.5.5 :001 > require 'active_support'
 => true
2.5.5 :002 > opts = ActiveSupport::OrderedOptions.new
 => {}
2.5.5 :003 > opts['someKey'] = 'someValue'
 => "someValue" 
2.5.5 :004 > opts.keys
 => [:someKey]

This was tested on rails 5.0, 5.2 & 6.0

  • Added a test that confirms loaded config maps are OrderedOptions classes by checking Users.AutoSetupUsernameBlacklist that has predefined members by default, with a check that the keys are in fact being returned as symbols.
  • Set up the AuditLogs.UnloggedAttributes related tests with OrderedOptions instead of hashes to expose the bug.
  • Applied the fix proposed by the customer
  • Also set up other config map related tests accordingly to see if any test started to fail (no failures, fortunately)
Actions #4

Updated by Tom Clegg over 3 years ago

This LGTM, thanks.

I'm a little concerned that it's too easy to repeat the same mistake (assigning regular hashes to config) in future. Maybe restore_configuration() in source:services/api/test/test_helper.rb could do a recursive "symbol keys" test on Rails.configuration to catch that?

Actions #5

Updated by Lucas Di Pentima over 3 years ago

Tom Clegg wrote:

I'm a little concerned that it's too easy to repeat the same mistake (assigning regular hashes to config) in future. Maybe restore_configuration() in source:services/api/test/test_helper.rb could do a recursive "symbol keys" test on Rails.configuration to catch that?

Good idea!!

Updates at f4d0ea9ca
Test run: developer-run-tests: #2092

  • Adds config checks on TestCase.teardown.
  • Moves restore_configuration() to TestCase.setup to avoid additional test failures after a config check assertion fail.
  • Fixes illegal test config settings discovered by the new check.
  • Fixes API.DisabledAPIs auto-setting depending on Containers.JobsAPI.Enable's value.
Actions #6

Updated by Tom Clegg over 3 years ago

Great, more hidden bugs revealed & squashed! LGTM, thanks.

Actions #7

Updated by Anonymous over 3 years ago

  • Status changed from In Progress to Resolved
Actions

Also available in: Atom PDF