Make all ACL categories explicit in JSON files

[dvkashapov]

Resolves #417

Script used for updating JSON files will be added as a separate comment.

[dvkashapov]

#!/usr/bin/env python3
"""
Script to make implicit ACL categories explicit in command JSON files.

Implicit ACL category rules:
1. WRITE flag - WRITE category
2. READONLY flag (and NOT SCRIPTING category) - READ category
3. ADMIN flag - ADMIN and DANGEROUS categories
4. PUBSUB flag - PUBSUB category
5. FAST flag - FAST category
6. BLOCKING flag - BLOCKING category
7. If NOT FAST category - SLOW category
"""

import json
import sys
from pathlib import Path
from typing import Dict, List, Set


def get_implicit_acl_categories(command_flags: List[str], existing_acl_categories: List[str]) -> Set[str]:
    implicit_categories = set()
    existing_set = set(existing_acl_categories)
    
    if "WRITE" in command_flags:
        implicit_categories.add("WRITE")
    
    if "READONLY" in command_flags and "SCRIPTING" not in existing_set:
        implicit_categories.add("READ")
    
    if "ADMIN" in command_flags:
        implicit_categories.add("ADMIN")
        implicit_categories.add("DANGEROUS")
    
    if "PUBSUB" in command_flags:
        implicit_categories.add("PUBSUB")
    
    if "FAST" in command_flags:
        implicit_categories.add("FAST")
    
    if "BLOCKING" in command_flags:
        implicit_categories.add("BLOCKING")
    
    all_categories = existing_set | implicit_categories
    if "FAST" not in all_categories:
        implicit_categories.add("SLOW")
    
    return implicit_categories - existing_set


def process_command(command_data: Dict):
    command_flags = command_data.get("command_flags", [])
    acl_categories = command_data.get("acl_categories", [])
    
    implicit_categories = get_implicit_acl_categories(command_flags, acl_categories)
    
    if "acl_categories" not in command_data:
        command_data["acl_categories"] = []
    
    command_data["acl_categories"].extend(sorted(implicit_categories))
    command_data["acl_categories"].sort()


def process_json_file(filepath: Path, dry_run: bool = False) -> tuple[bool, str]:
    with open(filepath, 'r', encoding='utf-8') as f:
        data = json.load(f)
    
    if not data:
        raise ValueError(f"{filepath.name}: Empty JSON file")
    
    command_name = list(data.keys())[0]
    command_data = data[command_name]   
    
    process_command(command_data)
    
    if not dry_run:
        with open(filepath, 'w', encoding='utf-8') as f:
            json.dump(data, f, indent=4, ensure_ascii=False)
            f.write('\n')
        
        return True, f"Updated {filepath.name}: added ACL categories"
    else:
        acl_categories = command_data.get("acl_categories", [])
        return True, f"Would update {filepath.name}: {acl_categories}"


def main():
    script_dir = Path(__file__).parent
    commands_dir = script_dir
    
    dry_run = "--dry-run" in sys.argv
    
    json_files = sorted(commands_dir.glob("*.json"))
    
    if not json_files:
        print(f"No JSON files found in {commands_dir}")
        return 1
        
    changed_count = 0
    
    for filepath in json_files:
        changed, message = process_json_file(filepath, dry_run)
        
        if changed:
            print(f"{message}")
            changed_count += 1

    
    print("Summary:")
    print(f"  Files processed: {len(json_files)}")
    print(f"  Files changed: {changed_count}")
        
    return 0


if __name__ == "__main__":
    sys.exit(main())

[dvkashapov]

Replication fail in module API tests are strange, I was unable to reproduce them locally with or without ASAN, will try to understand from crash report.

[enjoy-binbin]

The CI failure is because of #2886.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.40%. Comparing base (4a0e20b) to head (4365bbe).
⚠️ Report is 1 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #2887      +/-   ##
============================================
- Coverage     72.44%   72.40%   -0.05%     
============================================
  Files           128      128              
  Lines         70487    70477      -10     
============================================
- Hits          51066    51028      -38     
- Misses        19421    19449      +28     

Files with missing lines	Coverage Δ
src/commands.def	100.00% <ø> (ø)

... and 15 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[zuiderkwast]

OK, explicit is better than implicit. It looks good in general. The actual change is not huge if we ignore the generated JSON changes.

Have you checked that the ACL categories are the same before/after this change? For example comparing the reply of COMMAND?

How is this change affecting commands defined using ValkeyModule_CreateCommand? Do they use implicit ACL rules?

[dvkashapov]

@zuiderkwast Thank you for feedback! Applied review recommendations, PTAL

Have you checked that the ACL categories are the same before/after this change? For example comparing the reply of COMMAND?

Yes, no changes in COMMAND output.

How is this change affecting commands defined using ValkeyModule_CreateCommand? Do they use implicit ACL rules?

Current module code does not assume anything about ACL categories for new commands, so no need to handle implicit, we set implicit categories only for commands from JSON files.
Do we want same validation as for core commands for module commands?
I think we should but currently VM_CreateCommand and VM_SetCommandACLCategories are decoupled, so that may be counterintuitive to return error which is not related to arguments of VM_SetCommandACLCategories. I may be incorrect about module code flow, we need real expert to take a look!

[zuiderkwast]

Yes, no changes in COMMAND output.

Great!

Current module code does not assume anything about ACL categories for new commands, so no need to handle implicit, we set implicit categories only for commands from JSON files.

OK, good. It makes things easier.

Do we want same validation as for core commands for module commands?
I think we should but currently VM_CreateCommand and VM_SetCommandACLCategories are decoupled, so that may be counterintuitive to return error which is not related to arguments of VM_SetCommandACLCategories. I may be incorrect about module code flow, we need real expert to take a look!

I believe you're right. I read the code and I couldn't see any code path from the module code to implicit ACL categories.

No, let's not add any validation for modules. It might break existing modules. Let's keep the scope of this PR small and without breaking changes.

[dvkashapov]

@zuiderkwast so I triple checked everything by locally reverting setImplicitACLCategories() and instead of setting them I did serverAssert(c->acl_categories & ACL_CATEGORY); . I believe everything is correct currently and we're good to go, do we need anything else?

[zuiderkwast]

I believe everything is correct currently and we're good to go, do we need anything else?

Yes, I think so too. I wanted to compare the output of valkey-cli COMMAND locally, but COMMAND's reply is not sorted so it was more difficult than I expected. Then, I took the list of all commands using COMMAND LIST and compared the output of COMMAND INFO for each of the commands, one by one. All of them match, except some commands with subcommands (CLUSTER, CLIENT, ACL, FUNCTION), because their subcommands are not sorted. (Valkey on the unstable branch running on port 11111.)

for c in $(./valkey-cli command list) ; do
    diff <(./valkey-cli -p 11111 command info "$c") \
         <(./valkey-cli command info "$c") || echo "MISMATCH: $c"
done

This chows mismatch for CLUSTER, CLIENT, ACL, FUNCTION. Then, I tried to just sort the output line by line before comparing. Then everything matches:

for c in $(./valkey-cli command list) ; do
    diff <(./valkey-cli -p 11111 command info "$c"|sort) \
         <(./valkey-cli command info "$c"|sort) || echo "MISMATCH: $c"
done

So it's safe to say that this PR does not change the ACL categories of any command.

[dvkashapov]

Thank you! I had to use some frankenstein regexes to handle COMMAND's not sorted reply, your check is definitely more clever xD