` _CsvMgr.get_rule_key() ` is supplied with incorrect argument

Ma1h01 commented 4 weeks ago

Describe the bug

The above code snippet is from csv-to-oscal-cd.py. The highlighted portion has a potential bug. The get_rule_key() expects component_title, but component_description is passed in. But, this error somehow doesn't generate any serious bug whether when component_title has the same value as component_description(in the majority cases) or when they are different. However, the expected key is not in the _csv_rules_map dict.

To Reproduce

Steps to reproduce the behavior:

Refer to the below test case:

def test_execute_different_component_title_and_description(tmp_path: pathlib.Path) -> None:
    """Test execute missing value."""
    _, section = _get_config_section_init(tmp_path, 'test-csv-to-oscal-cd.config')
    # inject error
    rows = _get_rows('tests/data/csv/ocp4-user.v2.csv')
    row = rows[2]
    assert row[7] == 'OSCO'
    assert row[8] == 'OSCO'
    row[8] = 'IAM'
    assert row[8] == 'IAM'
    component_title = row[7]
    component_description = row[8]
    component_type = row[5]
    rule_id = row[1]
    with mock.patch('trestle.tasks.csv_to_oscal_cd.csv.reader') as mock_csv_reader:
        mock_csv_reader.return_value = rows
        expected_key = (component_title, component_type, rule_id)
        wrong_key = (component_description, component_type, rule_id)
        tgt = csv_to_oscal_cd.CsvToOscalComponentDefinition(section)
        retval = tgt.execute()
        assert retval == TaskOutcome.SUCCESS
        assert wrong_key in tgt._csv_mgr.get_rule_keys()
        assert expected_key in tgt._csv_mgr.get_rule_keys()

Expected behavior

A clear and concise description of what you expected to happen.

Screenshots / Logs.

If applicable, add screenshots to help explain your problem.

Environment

OS: [e.g. iOS]
Python version:
Installed packages:

degenaro commented 4 weeks ago

If I understand this correctly, the current mainline code is broken, resulting in this test case failing due to the expected key not present. In that case, checking for the wrong key is superfluous and should be removed.

A correct mainline code fix will cause this failing test case to succeed without any change to the test case itself.

On Thu, Jun 6, 2024 at 1:58 PM Ma1h01 @.***> wrote:

Describe the bug image.png (view on web) https://github.com/oscal-compass/compliance-trestle/assets/78010183/a7126df8-fcba-4b15-820f-c555101a60f7 The above code snippet is from csv-to-oscal-cd.py. The highlighted portion has a potential bug. The get_rule_key() expects component_title, but component_description is passed in. But, this error somehow doesn't generate any serious bug whether when component_title has the same value as component_description(in the majority cases) or when they are different. However, the expected key is not in the _csv_rules_map dict. To Reproduce

Steps to reproduce the behavior:

Refer to the below test case:

def test_execute_different_component_title_and_description(tmppath: pathlib.Path) -> None: """Test execute missing value.""" , section = _get_config_section_init(tmp_path, 'test-csv-to-oscal-cd.config')

inject error
rows = _get_rows('tests/data/csv/ocp4-user.v2.csv')
row = rows[2]
assert row[7] == 'OSCO'
assert row[8] == 'OSCO'
row[8] = 'IAM'
assert row[8] == 'IAM'
component_title = row[7]
component_description = row[8]
component_type = row[5]
rule_id = row[1]
with mock.patch('trestle.tasks.csv_to_oscal_cd.csv.reader') as mock_csv_reader:
    mock_csv_reader.return_value = rows
    expected_key = (component_title, component_type, rule_id)
    wrong_key = (component_description, component_type, rule_id)
    tgt = csv_to_oscal_cd.CsvToOscalComponentDefinition(section)
    retval = tgt.execute()
    assert retval == TaskOutcome.SUCCESS
    assert wrong_key in tgt._csv_mgr.get_rule_keys()
    assert expected_key in tgt._csv_mgr.get_rule_keys()
Expected behavior

A clear and concise description of what you expected to happen. Screenshots / Logs.

If applicable, add screenshots to help explain your problem. Environment

OS: [e.g. iOS]

Python version:

Installed packages:

— Reply to this email directly, view it on GitHub https://github.com/oscal-compass/compliance-trestle/issues/1576, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAD66XOQG7WKGIQFR5UAKL3ZGCPNVAVCNFSM6AAAAABI5GZR62VHI2DSMVQWIX3LMV43ASLTON2WKOZSGMZTQOBVHA3DGMA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

Ma1h01 commented 4 weeks ago

Would the mainline code fix be simply replacing component_description with component_title?

degenaro commented 4 weeks ago

In theory, yes!

On Thu, Jun 6, 2024 at 3:41 PM Ma1h01 @.***> wrote:

Would the mainline code fix be simply replacing component_description with component_title?

— Reply to this email directly, view it on GitHub https://github.com/oscal-compass/compliance-trestle/issues/1576#issuecomment-2153277789, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAD66XPIKOZWEJ6TJX6BMKTZGC3OJAVCNFSM6AAAAABI5GZR62VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJTGI3TONZYHE . You are receiving this because you commented.Message ID: @.***>

oscal-compass / compliance-trestle