(adobereader) Using /IgnoreInstalled reports matched packages multiple times

pauby commented 1 month ago

There is a bug in # loop over each found installation and ignore it if it matches a value in '/IgnoreInstalled' that multiplies the found installs by the number of not matching IgnoreInstalled, e.g 8 matching installs instead of 2 (and will also prevent reaching 1 matchig installs after processing other configurations). I defects the result for all multiple IgnoreInstalled.

    /IgnoreInstalled package parameter was passed with these software names:
    - Adobe Acrobat X Pro*
    - Adobe Acrobat XI Pro
    - Adobe Acrobat DC (2015)
    - Adobe Acrobat 2017
    Keeping 'Adobe Acrobat 2020' as it does not match 'Adobe Acrobat X Pro*)'
    Keeping 'Adobe Acrobat 2020' as it does not match 'Adobe Acrobat XI Pro)'
    Keeping 'Adobe Acrobat 2020' as it does not match 'Adobe Acrobat DC (2015))'
    Keeping 'Adobe Acrobat 2020' as it does not match 'Adobe Acrobat 2017)'
    Keeping 'Adobe Acrobat (64-bit)' as it does not match 'Adobe Acrobat X Pro*)'
    Keeping 'Adobe Acrobat (64-bit)' as it does not match 'Adobe Acrobat XI Pro)'
    Keeping 'Adobe Acrobat (64-bit)' as it does not match 'Adobe Acrobat DC (2015))'
    Keeping 'Adobe Acrobat (64-bit)' as it does not match 'Adobe Acrobat 2017)'
    After processing, we will use this list of installed software:
    - Adobe Acrobat 2020
    - Adobe Acrobat 2020
    - Adobe Acrobat 2020
    - Adobe Acrobat 2020
    - Adobe Acrobat (64-bit)
    - Adobe Acrobat (64-bit)
    - Adobe Acrobat (64-bit)
    - Adobe Acrobat (64-bit)
    8 matching installs of Adobe Acrobat Reader DC found!
    To prevent accidental data loss, this install will be aborted.

Maybe modify the code to something like the following?

    # loop over each found installation and ignore it if it matches a value in '/IgnoreInstalled'
    $key = for ($i = 0; $i -lt $installation.count; $i++) {
    $KEEP = $true
    for ($j = 0; $j -lt $matchInstallation.count; $j++) {
    if ($installation[$i].DisplayName -notlike $matchInstallation[$j]) {
    Write-Verbose "Keeping '$($installation[$i].DisplayName)' as it does not match '$($matchInstallation[$j]))'"
    }
    else {
    $KEEP = $false
    Write-Verbose "Removing '$($installation[$i].DisplayName)' from list of found software, as it matches '$($matchInstallation[$j]))'"
    }
    }
    If ($KEEP) {
    $installation[$i]
    }
    }

github-actions[bot] commented 1 month ago

Thanks for raising this issue!

The packages within this repository are maintained in my spare time. My spare time, like yours is important. Please help me not to waste it.

To help me, and to have the issue resolved more quickly, please see CONTRIBUTING for how to raise a pull request to resolve the issue yourself.

Thank you.

sf0-k46 commented 1 month ago

Suggestion:

# Since -notlike does not compare a single String to an array of patterns use -notmatch.
# Convert the $matchInstallation-array from a -like- to a -match-syntax in a single string to make it sematically identical by ...
#   - replacing '*' with '.*'
#   - frame the elements with '^' and '$'
#   - delimit elements with '|'
# E.g. 'Adobe Acrobat X Pro*', 'Adobe Acrobat XI Pro'
#   -> '^Adobe Acrobat X Pro.*$|^Adobe Acrobat XI Pro$'
$matchInstallationPattern = ('^' + (($matchInstallation -replace '\*','.*') -join '$|^') + '$')

$key = $installation `
| Where-Object `
    -FilterScript {
        $_.DisplayName -notmatch $matchInstallationPattern
    }

pauby commented 1 month ago

What would be the purpose of using that code over what is there?

sf0-k46 commented 1 month ago

The code whats is there

Outputs the current $installation for each $matchInstallation that is not like it. When using at least two different elements in $matchInstallation this does lead to never removing any element of $installation since every element is outputted at least one time.
The Elements get outputted multiple times so ($key.Count -eq 1) in line 70 can never be true unless only one element is given in /IgnoreInstalled and can really ignore an entry.

So the code what is there does not ignore installations when multiple elements are given in /IgnoreInstalled despite the purpose of that block doing so.

The suggested code does # loop over each found installation and ignore it if it matches a value in '/IgnoreInstalled'. Looping is done by $installation | Where-Object, ignoring by comparing the DisplayName with each of the values in /IgnoreInstalled by using -notmatch and a pattern that combines the values by using | to OR them.

sf0-k46 commented 1 month ago

The following code tests the results of the three code variants on a common installation set with a common /IgnoreInstalled configuration.

# <https://github.com/pauby/ChocoPackages/issues/230>

# Iterate over the three code versions
@(1, 2, 3) | ForEach-Object {

    # set up the used variables for each version

    # existent typed variables can influence the result
    Remove-Variable -Name @('installation', 'matchInstallation', 'key')

    # Test case: Current parallel installation
    [array]$installation = @(
        [PSCustomObject]@{
            DisplayName = 'Adobe Acrobat (64-bit)'
        }
        [PSCustomObject]@{
            DisplayName = 'Adobe Acrobat 2020'
        }
    )

    # Test case: More than one software to ignore
    $matchInstallation = @(
        'Adobe Acrobat 2020'
        'SomeOtherProduct'
    )

    "Running variant $_"

    switch ($_) {

        # the code that is there
        # <https://github.com/pauby/ChocoPackages/blob/4184359f216e4126ec054a84047c6451bad406c9/automatic/adobereader/tools/chocolateyinstall.ps1#L41>
        1 {
            # loop over each found installation and ignore it if it matches a value in '/IgnoreInstalled'
            $key = for ($i = 0; $i -lt $installation.count; $i++) {
                for ($j = 0; $j -lt $matchInstallation.count; $j++) {
                    if ($installation[$i].DisplayName -notlike $matchInstallation[$j]) {
                        $installation[$i]
                        Write-Verbose "Keeping '$($installation[$i].DisplayName)' as it does not match '$($matchInstallation[$j])'"
                    }
                    else {
                        Write-Verbose "Removing '$($installation[$i].DisplayName)' from list of found software, as it matches '$($matchInstallation[$j])'"
                    }
                }
            }
        }

        # Disqus-comment
        # <http://disq.us/p/2zlyuib>
        2 {
            # loop over each found installation and ignore it if it matches a value in '/IgnoreInstalled'
            $key = for ($i = 0; $i -lt $installation.count; $i++) {
                $KEEP = $true
                for ($j = 0; $j -lt $matchInstallation.count; $j++) {
                    if ($installation[$i].DisplayName -notlike $matchInstallation[$j]) {
                        Write-Verbose "Keeping '$($installation[$i].DisplayName)' as it does not match '$($matchInstallation[$j])'"
                    }
                    else {
                        $KEEP = $false
                        Write-Verbose "Removing '$($installation[$i].DisplayName)' from list of found software, as it matches '$($matchInstallation[$j])'"
                    }
                }
                If ($KEEP) {
                    $installation[$i]
                }
            }

            Write-Verbose "After processing, we will use this list of installed software:"
            $key | ForEach-Object {
                Write-Warning "- $($_.DisplayName)"
            }
        }

        # GitHub-issue
        # <https://github.com/pauby/ChocoPackages/issues/230#issuecomment-2237235529>
        3 {
            # Since -notlike does not compare a single String to an array of patterns use -notmatch.
            # Convert the $matchInstallation-array from a -like- to a -match-syntax in a single string to make it sematically identical by ...
            #   - replacing '*' with '.*'
            #   - frame the elements with '^' and '$'
            #   - delimit elements with '|'
            # E.g. 'Adobe Acrobat X Pro*', 'Adobe Acrobat XI Pro'
            #   -> '^Adobe Acrobat X Pro.*$|^Adobe Acrobat XI Pro$'
            $matchInstallationPattern = ('^' + (($matchInstallation -replace '\*','.*') -join '$|^') + '$')

            [array]$key = $installation `
            | Where-Object `
                -FilterScript {
                    $_.DisplayName -notmatch $matchInstallationPattern
                }
        }
    }

    $key
    "Type: $($key.GetType().BaseType.Name)"
    "Count: $($key.Count)"

    if ($key.Count -eq 1) {
        "CONTINUE"
    } else {
        'ABORT'
    }

    ''

}

Variant 1 does not filter correctly, always aborts:

Running variant 1

DisplayName           
-----------           
Adobe Acrobat (64-bit)
Adobe Acrobat (64-bit)
Adobe Acrobat 2020    
Type: Array
Count: 3
ABORT

Variant 2 was a minimal quick-fix that filters correctly but test shows that it outputs the wrong type, so that it always aborts. Maybe this is due to the test code but as it mainly reordered the output of variant 1 that could be an indication that variant 1 is not type safe also. An explicit type case would correct this:

Running variant 2
Adobe Acrobat (64-bit)
Type: Object
Count: 
ABORT

Variant 3 is a code refacturing and does filter correctly. But to make it type safe an explicit typecast to [array] has been added compared to the previous suggestion:

Running variant 3
Adobe Acrobat (64-bit)
Type: Array
Count: 1
CONTINUE

pauby / ChocoPackages

(adobereader) Using /IgnoreInstalled reports matched packages multiple times #230