Skip to content

refactor(ConditionGroup): IsValid() returns false for all group conditions — misleading contract #31

@HeyItsGilbert

Description

@HeyItsGilbert

Summary

ConditionGroup.IsValid() checks whether Property, Operator, and Value are all non-null — which is only true for flat leaf conditions. For group conditions (AllOf/AnyOf/Not), all three are $null, so IsValid() always returns $false regardless of whether the group is perfectly well-formed. Any caller using IsValid() to check group conditions will get a wrong result.

File

Gatekeeper/Classes/FeatureFlag.ps1:69-76

Current Code

[boolean]IsValid() {
    # comment says 'nested groups are not checked' but the method returns $false for them
    if ($null -ne $this.Property -and $null -ne $this.Operator -and $null -ne $this.Value) {
        return $true
    }
    return $false
}

Fix Options

Option A — Rename to IsLeaf() to accurately describe what it tests:

[boolean]IsLeaf() {
    return $null -ne $this.Property -and $null -ne $this.Operator -and $null -ne $this.Value
}

Option B — Extend IsValid() to also return $true when a group is correctly configured:

[boolean]IsValid() {
    if ($null -ne $this.AllOf -or $null -ne $this.AnyOf -or $null -ne $this.Not) { return $true }
    return $null -ne $this.Property -and $null -ne $this.Operator -and $null -ne $this.Value
}

Notes

  • Found by Jordan B. and Chip Torres

Metadata

Metadata

Assignees

No one assigned

    Labels

    refactorCode quality, structure, or naming

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions