sarbian / ModuleManager

176 stars 96 forks source link

[Bug] Index operator has no effect when used in conjunction with `:HAS` clause on a node #168

Closed al2me6 closed 3 years ago

al2me6 commented 3 years ago

This section of the ModuleManager documentation suggests that using indexing in conjunction with :HAS clauses is supported (emphasis mine):

For nodes, all options are available to select the node, including indexes, * index, HAS, and wildcards in the name.

However, it appears that using :HAS overrides any indexing operators and instead has the same behavior as if ,* were present.

Reproduction:

The below series of MM patches:

// These three cases do not work as expected:

TEST_CASE_1
{
    Foo
    {
        bar = 1
    }
    Foo
    {
        bar = 2
    }
    Foo
    {
        bar = 3
    }
}
@TEST_CASE_1
{
    // Intention: patch the first matching `Foo`, which is the second `Foo`.
    @Foo:HAS[#bar[>1]],0
    {
        baz = true
    }
}

TEST_CASE_2
{
    Foo
    {
        bar = 1
    }
    Foo
    {
        bar = 2
    }
    Foo
    {
        bar = 3
    }
}
@TEST_CASE_2
{
    // Intention: patch the second matching `Foo`, which is the third foo.
    @Foo:HAS[#bar[>1]],1
    {
        baz = true
    }
}

TEST_CASE_3
{
    Foo
    {
        bar = 1
    }
    Foo
    {
        bar = 2
    }
    Foo
    {
        bar = 3
    }
}
@TEST_CASE_3
{
    // Intention: patch all matching `Foo`s, i.e., the second and third ones.
    @Foo:HAS[#bar[>1]],*
    {
        baz = true
    }
}

// However, indexing without `:HAS` clauses does work as expected:

TEST_CASE_4
{
    Foo
    {
        bar = 1
    }
    Foo
    {
        bar = 2
    }
    Foo
    {
        bar = 3
    }
}
@TEST_CASE_4
{
    // This should edit the first `Foo`, since no index is specified.
    @Foo
    {
        baz = true
    }
}

TEST_CASE_5
{
    Foo
    {
        bar = 1
    }
    Foo
    {
        bar = 2
    }
    Foo
    {
        bar = 3
    }
}
@TEST_CASE_5
{
    // This should only edit the second `Foo`.
    @Foo,1
    {
        baz = true
    }
}

produce the following nodes in ModuleManager.ConfigCache:

UrlConfig
{
    parentUrl = /mmtest
    TEST_CASE_1
    {
        Foo
        {
            bar = 1
        }
        Foo
        {
            bar = 2
            baz = true
        }
        // ERROR: Only the second `Foo` should have `baz`, since that is the first `Foo` where `bar > 1` is true.
        Foo
        {
            bar = 3
            baz = true 
        }
    }
}
UrlConfig
{
    parentUrl = /mmtest
    TEST_CASE_2
    {
        Foo
        {
            bar = 1
        }
        // ERROR: We specifically indexed the second matching `Foo`, so the first one should not be patched.
        Foo
        {
            bar = 2
            baz = true
        }
        Foo
        {
            bar = 3
            baz = true
        }
    }
}
UrlConfig
{
    parentUrl = /mmtest
    TEST_CASE_3
    {
        Foo
        {
            bar = 1
        }
        // Note that wildcard has the same behavior as explicit indexing.
        Foo
        {
            bar = 2
            baz = true
        }
        Foo
        {
            bar = 3
            baz = true
        }
    }
}
UrlConfig
{
    parentUrl = /mmtest
    TEST_CASE_4
    {
        // As expected, only the first `Foo` is patched.
        Foo
        {
            bar = 1
            baz = true
        }
        Foo
        {
            bar = 2
        }
        Foo
        {
            bar = 3
        }
    }
}
UrlConfig
{
    parentUrl = /mmtest
    TEST_CASE_5
    {
        Foo
        {
            bar = 1
        }
        // As expected, only the second `Foo` is patched.
        Foo
        {
            bar = 2
            baz = true
        }
        Foo
        {
            bar = 3
        }
    }
}

It appears that this bug is breaking RealPlume's support for having multiple PLUMEs of the same kind, which was first introduced in this commit.

This bug also impacts the WIP ROE-Waterfall patchset, of which I am a contributor. Specifically, this patch, which relies on the same logic as the RealPlume patches linked above, is not functioning.

This was tested on KSP version 1.10.1 and ModuleManager version 4.1.4.

Curiously, this bug also exists when using KSP 1.10.1 and ModuleManager version 4.0.2, which appears to have been the most recent version of MM when the RealPlume patches above were committed.

CC @vevladdd.