Skip to content

fix: setup_config accepts multiple values for list fields#562

Open
nishantxscooby wants to merge 4 commits intocertego:developfrom
nishantxscooby:main
Open

fix: setup_config accepts multiple values for list fields#562
nishantxscooby wants to merge 4 commits intocertego:developfrom
nishantxscooby:main

Conversation

@nishantxscooby
Copy link
Contributor

@nishantxscooby nishantxscooby commented Feb 7, 2026

🎯 Description

Fixes #499

The setup_config Django management command now correctly parses and accepts multiple values with spaces for list-type (ArrayField) configuration fields in a single invocation.

Problem

Previously, attempting to pass multiple values with spaces would fail:

# ❌ Required workaround (multiple commands)
./manage.py setup_config -a filtered_alerts_types=['New Device']
./manage.py setup_config -a filtered_alerts_types=['User Risk Threshold']
./manage.py setup_config -a filtered_alerts_types=['Anonymous IP Login']

Solution

Enhanced the parse_field_value() function with quote-aware CSV parsing and fixed validation logic to properly handle list values:

# ✅ Now works in a single command
./manage.py setup_config -a 'filtered_alerts_types=["New Device", "User Risk Threshold", "Anonymous IP Login"]'

🔧 Changes Made

Modified Files

  1. buffalogs/impossible_travel/management/commands/setup_config.py

    • Enhanced parse_field_value() with manual quote-aware CSV parsing
    • Supports single quotes, double quotes, and mixed formats
    • Always returns a list when brackets [...] are present
    • Fixed validation logic to validate the complete value instead of individual elements
    • Maintains 100% backward compatibility
  2. buffalogs/impossible_travel/tests/task/test_management_commands.py

    • Added 7 comprehensive test cases
    • Tests all three modes: append (-a), override (-o), remove (-r)
    • Covers edge cases: empty lists, single values, multiple values, quotes

✅ Testing

Test Results

✅ All 18 automated tests pass (100% success rate)
✅ All 7 manual integration tests pass
✅ Black formatting: PASSED
✅ isort: PASSED
✅ flake8: PASSED

Manual Test Coverage

  1. ✅ Multiple values with spaces in append mode
  2. ✅ Multiple values with spaces in override mode
  3. ✅ Multiple values with spaces in remove mode
  4. ✅ Single-element lists return list type (fixed validation error)
  5. ✅ Backward compatibility (values without spaces/quotes still work)
  6. ✅ Empty lists handled correctly
  7. The exact case from issue [REFACTOR] <Django command>: setup_config command does not accept multiple values for list fields #499 works perfectly

Test Commands

# Test 1: Append mode
./manage.py setup_config -a 'ignored_users=["admin user", "bot user"]'
# Result: ['admin user', 'bot user'] ✅

# Test 2: Override mode
./manage.py setup_config -o 'vip_users=["VIP One", "VIP Two", "VIP Three"]'
# Result: ['VIP One', 'VIP Two', 'VIP Three'] ✅

# Test 3: Remove mode
./manage.py setup_config -o 'enabled_users=["keep me", "remove 1", "remove 2"]'
./manage.py setup_config -r 'enabled_users=["remove 1", "remove 2"]'
# Result: ['keep me'] ✅

# Test 4: Backward compatibility
./manage.py setup_config -o 'allowed_countries=[Italy,Romania,France]'
# Result: ['Italy', 'Romania', 'France'] ✅

# Test 5: Issue #499 exact case
./manage.py setup_config -a 'filtered_alerts_types=["New Device", "User Risk Threshold", "Anonymous IP Login"]'
# Result: ['New Device', 'User Risk Threshold', 'Anonymous IP Login'] ✅

🔄 Backward Compatibility

100% backward compatible - all existing command syntax continues to work:

# All these still work exactly as before
./manage.py setup_config -a ignored_users=admin
./manage.py setup_config -o allowed_countries=[Italy,Romania]
./manage.py setup_config -r vip_users=user1

📚 Implementation Details

Quote-Aware CSV Parsing

The fix uses manual character-by-character parsing to handle quoted CSV values:

  • Tracks quote state (in/out of quotes, which quote character)
  • Splits on commas only when outside quotes
  • Strips quotes from final values
  • Handles edge cases (empty strings, mixed quotes, etc.)

Validation Fix

Fixed validation logic to validate the complete value for list fields instead of iterating through individual elements. ArrayField validators expect to receive the entire list, not individual strings.

Before (❌ Bug):

values_to_validate = value if is_list else [value]
for val in values_to_validate:  # Iterates individual elements
    validator(val)  # Fails: validator expects list, gets string

After (✅ Fixed):

for validator in getattr(field_obj, "validators", []):
    validator(value)  # Validates complete list

🎯 Impact

Before:

  • ❌ Multiple values with spaces required multiple commands
  • ❌ Poor user experience
  • ❌ Inconsistent with help text examples

After:

  • ✅ Single command handles all cases
  • ✅ Intuitive syntax matching documentation
  • ✅ Improved developer experience

📋 Checklist


🔧 UPDATE: Idempotent Append Mode (2026-02-07)

Issue Reported by @Lorygold

Running the command multiple times was appending duplicate values:

# First run
./manage.py setup_config -a "filtered_alerts_types=['New Device','User Risk Threshold']"
# Result: ['New Device', 'User Risk Threshold'] ✅

# Second run (same command)
./manage.py setup_config -a "filtered_alerts_types=['New Device','User Risk Threshold']"
# Result: ['New Device', 'User Risk Threshold', 'New Device', 'User Risk Threshold'] ❌

Fix Applied

Modified the append logic to filter out existing values before adding:

if mode == "append":
    # Only append values that don't already exist (make it idempotent)
    new_values = [v for v in value if v not in current]
    current += new_values

Benefits

  • Idempotent: Running the command multiple times produces the same result
  • Safe for automation: Can be used in CI/CD pipelines without risk of duplicates
  • Configuration management friendly: Compatible with tools like Ansible, Terraform
  • Tested: Added test_setup_config_append_idempotent (all 26 tests pass)

Test Results

# Run command 3 times
./manage.py setup_config -a "filtered_alerts_types=['User Risk Threshold','New Device']"
./manage.py setup_config -a "filtered_alerts_types=['User Risk Threshold','New Device']"
./manage.py setup_config -a "filtered_alerts_types=['User Risk Threshold','New Device']"

# Result: ['User Risk Threshold', 'New Device'] ✅ (no duplicates!)

- Enhanced parse_field_value() to handle quoted values with spaces
- Supports single quotes, double quotes, and mixed formats
- Maintains backward compatibility with unquoted values
- Added 6 comprehensive unit tests + 1 integration test
- All 25 tests pass (18 existing + 7 new)
- Code formatted with Black, isort, and flake8

Fixes certego#499
@nishantxscooby
Copy link
Contributor Author

Hello @Lorygold, sorry for the delay, I just got busy with my university's exam. I have made all the required changes, let me know if it's gtg.

Thankyou :)

@Lorygold
Copy link
Contributor

Lorygold commented Feb 7, 2026

@nishantxscooby some checks have to be added, 'cause if I run the command multiple times, also the already existing values are appended:

In the admin page:
image

and in the DB:
image

@Noble-47 fyi

nishantxscooby added a commit to nishantxscooby/BuffaLogs that referenced this pull request Feb 7, 2026
- Modified append logic to filter out existing values before adding
- Prevents duplicate entries when command runs multiple times
- Safe for automation and configuration management tools
- Added test_setup_config_append_idempotent to verify behavior
- All 26 tests pass (19 setup_config + 7 other commands)

Fixes issue reported by @Lorygold in PR certego#562
- Modified append logic to filter out existing values before adding
- Prevents duplicate entries when command runs multiple times
- Safe for automation and configuration management tools
- Added test_setup_config_append_idempotent to verify behavior
- All 26 tests pass (19 setup_config + 7 other commands)

Fixes issue reported by @Lorygold in PR certego#562
@nishantxscooby
Copy link
Contributor Author

@Lorygold Good catch! 🎯

I've added a fix to make the append mode idempotent. Now running the command multiple times won't create duplicates.

What changed:

The append logic now filters out existing values before adding:

new_values = [v for v in value if v not in current]
current += new_values

@Noble-47
Copy link
Contributor

Noble-47 commented Feb 7, 2026

Hi @nishantxscooby

Your fix works, but it also seems like exception handling misses invalid config values like "New Devace".

image image

The exception raised here is django.db.IntegrityError expected a CommandError

image

Is this the intended behaviour?

…yError

- Validate array field values against AlertDetectionType.choices before saving
- Raise clear CommandError with invalid values and valid choices list
- Prevents database IntegrityError from check constraint violation
- Addresses feedback from Noble-47 on PR certego#562
…yError

- Import AlertDetectionType from impossible_travel.constants
- Validate array field values against AlertDetectionType.choices before saving
- Raise clear CommandError with invalid values and valid choices list
- Prevents database IntegrityError from check constraint violation
- Addresses feedback from Noble-47 on PR certego#562
@nishantxscooby
Copy link
Contributor Author

@Noble-47 Hi! Sorry, I just realised that the exception handling was missing validation for invalid config values.

I've just pushed a fix that adds pre-save validation for filtered_alerts_types before the database save operation. Now invalid values (like typos such as "New Devace") will raise a clear CommandError instead of the cryptic IntegrityError.

Example output:

$ python3 manage.py setup_config -a filtered_alerts_types="INVALID_VALUE"

CommandError: Invalid values in 'filtered_alerts_types': ['INVALID_VALUE']. 
Valid choices are: ['New Device', 'Imp Travel', 'New Country', 'User Risk Threshold', 'Anonymous IP Login', 'Atypical Country']

@nishantxscooby
Copy link
Contributor Author

Hello, @Noble-47 @Lorygold - Just a reminder :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[REFACTOR] <Django command>: setup_config command does not accept multiple values for list fields

3 participants