SOLVED

Problem with Foreach loop with multiple if statements

Contributor

I have this script setting the user's uidNumber and unixHomeDirectory without issue. It's just not setting the uid attribute. I'm not getting any errors when I run the script so I'm at a loss as to what the issue is. Hoping someone can point me in the right direction. 

 

$allusers = Get-ADUser -Filter "employeeID -ge 0" -Properties *

foreach ($user in $allusers)
{
    [int]$empid = (Get-ADUser $user -Property EmployeeID).EmployeeID

    if ($user.uidNumber -eq $null)
        {
            $uidnum = "1"+'{0:d5}' -f $empid
            Set-ADUser -Identity $user.SamAccountName -Replace @{uidNumber=$uidnum}
        }

    if ($user.uid -eq $null)
        {
            $uid = $user.SamAccountName
            Set-ADUser -Identity $user.SamAccountName -Replace @{uid=$uid}
        }

    if ($user.unixHomeDirectory -eq $null)
        {
            $unixhome = "/home/" + $user.SamAccountName
            Set-ADUser -Identity $user.SamAccountName -Replace @{unixHomeDirectory=$unixhome}
        }
}

 

 

9 Replies

@Baron164 Changed your script a little bit, the $null comparison doesn't always work. I changed it for the ui attribute, [string]::IsNullOrWhitespace($user.uid) does work :) 

 

foreach ($user in Get-ADUser -Filter "employeeID -ge 0" -Properties *) {
    [int]$empid = (Get-ADUser $user -Property EmployeeID).EmployeeID

    if ($null -eq $user.uidNumber) {
        $uidnum = "1" + '{0:d5}' -f $empid
        Set-ADUser -Identity $user.SamAccountName -Replace @{uidNumber = $uidnum }
    }

    if ([string]::IsNullOrWhitespace($user.uid)) {
        $uid = $user.SamAccountName
        Set-ADUser -Identity $user.SamAccountName -Replace @{uid = $uid }
    }

    if ($null -eq $user.unixHomeDirectory ) {
        $unixhome = "/home/" + $user.SamAccountName
        Set-ADUser -Identity $user.SamAccountName -Replace @{unixHomeDirectory = $unixhome }
    }
}

  

best response confirmed by Baron164 (Contributor)
Solution

@Baron164 

 

@Harm_Veenstra's reply contains three important new elements:

 

  1. The first is the one he called out in relation to [string]::IsNullOrWhitespace;
  2. The second is repositioning the $null checks from the format of "<something> -eq $null" to "$null -eq <something>";
  3. The third is getting rid of the original line 1, where the entire result set is being stored in an array (the $allusers variable.)

 

Harm's already explained point 1, so we'll start at point 2.

 

Point 2

The original format of "<something> -eq $null" is fine for single-valued data types like [string], but breaks on multivalued types (including collections, which make up a lot of things in PowerShell.)

 

The reason for this is that when a multi-valued object is on the left-hand side and $null is on the right-hand side, PowerShell filters the multi-valued object and emits empty objects into the pipeline just as if you had used the pipe symbol followed by a Where-Object. Here's an example.

 

LainRobertson_0-1658982373542.png

 

As you can see, this doesn't produce a Boolean result at all - it's something entirely different.

 

Conversely, if you use $null on the left-hand side, you get the Boolean response you expected.

 

LainRobertson_1-1658982512774.png

 

Point 3

If you're working with large data sets of any kind, you do not want to assign such massive things to a variable. Instead, you want to process objects directly across the pipeline.

 

Let's compare your original approach to Harm's across 400,000 objects.

 

Currently, you're:

 

  • Reading 400,000 into a variable, hitting your local resources hard;
  • You've done no processing whatsoever yet;
  • The .NET garbage collector cannot reclaim any memory as you have not yet finished using the variable (since the use of $allusers in the foreach() statement ensures it lives for the lifetime of the entire script.)

Harm's approach:

 

  • Reads a much smaller set of objects (equal to the page size);
  • Processes this smaller set according to the logic inside the foreach() loop;
  • Releases the objects as they are now out of the script's scope, which allows the .NET garbage collector to free the memory;
  • Repeats this "small batch" process until all objects have been processed.

 

Basically, what this translates to is that Harm's version will scale up to handle results of practically unlimited size with next to no performance hit while yours will struggle as the result set grows.

 

Does this matter?

 

In smaller environments, no, probably not. But it was worth calling out as forming good habits is always a good idea no matter the environment - particularly when it takes no more effort.

 

Additional observations

Performance

This is just a quick callout that you don't want to be calling Set-ADObject for every individual attribute change as that can - with larger data sets - also harm performance - not just locally but also potentially on the domain controller.

 

This use case is very simple meaning you can safely store all the attribute changes in a hash table and call Set-ADUser a single time at the end.

 

Exception handling

I've dropped an example below that adds some better exception handling to the process since PowerShell has a default disposition for throwing an exception yet continuing to execute any following statements, which can yield unexpected/undesirable outcomes, and that's something you want to avoid when you're playing around with people's accounts!

 

Anyhow, happy coding!

 

foreach ($user in (Get-ADUser -Filter { (employeeID -like "*") } -Properties employeeID, objectGUID, sAMAccountName, uidNumber)) {
    try
    {
        # Filter on employeeID = 0, since that will also capture the $null scenario (since integer types cannot equal null). Additionally, if employeeID contains non-numeric characters, an exception will be thrown - which should be handled properly handled with a try/catch.
        if ([int]$user.employeeID -ne 0)
        {
            # Using a hash table means we don't waste resources repeatedly calling Get-/Set-ADUser on a per attribute basis.
            $Hash = [hashtable]::new();

            #region Attribute: uidNumber
            $uidNumber = "1{0:d5}" -f $user.employeeID;

            if (-not ($user.uidNumber -and ($uidNumber -eq $user.uidNumber)))
            {
                $Hash.Add("uidNumber", $uidNumber);
            }
            #endregion

            #region Attribute: uid
            if ([string]::IsNullOrWhitespace($user.uid))
            {
                $Hash.Add("uid", $user.sAMAccountName);
            }
            #endregion

            #region Attribute: unixHomeDirectory
            if ([string]::IsNullOrWhitespace($user.unixHomeDirectory))
            {
                $Hash.Add("unixHomeDirectory", "/home/$($user.sAMAccountName)");
            }
            #endregion

            # Now we can call Set-ADUser to implement the values from $Hash.
            if ($Hash.Count -gt 0)
            {
                Set-ADUser -Identity ($user.objectGUID) -Replace $Hash;
            }
        }
    }
    catch
    {
        Write-Warning -Message "Failed to update user with sAMAccountName = $($user.sAMAccountName).";
        throw;
    }
}

 

Cheers,

Lain

Good explanation and addition!

One belated explanation:

 

I used "objectGUID" in the final call to Set-ADUser so that if you ever need to rename the sAMAccountName, you can do so without breaking the script.

 

I didn't choose it just to be cryptic!

 

Cheers,

Lain

@LainRobertson 

Thank you for the extensive write up, this is very informative and helpful.

As a test I cleared out the uid/uidNumber/unixHomeDirectory from a test account and ran your script and it threw this error:

Baron164_0-1659017450938.png

 

@Baron164 

 

No problem.

 

I'd made a basic error on line 11 and have resolved that through updating lines 11 and 12, so try again when you're able.

 

Cheers,

Lain

@LainRobertson 

Still not working.

Baron164_0-1659458634058.png


Also, this section doesn't make sense to me. Shouldn't it be parsing the EmployeeID and not the uidNumber? So instead of using $uidNumber I would expect that to be something like $EmployeeID.

            #region Attribute: uidNumber
            if ($user.uidNumber -and [int32]::TryParse($user.uidNumber, [ref] ([int]$uidNumber = 0)))
            {
                $Hash.Add("uidNumber", "1{-:d5}" -f $uidNumber);
            }
            else
            {
                Write-Warning -Message "Skipping user with employeeID = $($user.employeeID). uidNumber is either missing or equals 0.";
                continue;
            }
            #endregion

 Then it would only skip a user if the EmployeeID was blank, not the uidNumber which is what it should be setting.

@Baron164 

 

The error was simply from my fat-fingering of the regular expression. On line 13, I had:

 

"1{-:d5}"

 

Which should have been:

 

"1{0:d5}"

 

I've fixed that typo now in-place in the earlier  post.

 

And no, on line 11, we're testing for the presence of uidNumber, not employeeID.

 

employeeID is checked back on line 5 and the user is skipped if employeeID is either null or equal to 0.

 

That said, your final sentence is still correct for a different reason, as I had completely misunderstood the intent with uidNumber. I've updated the script based on your comment.

 

I've also taken the opportunity to add a quick check prior to the Set-ADUser so it isn't called needlessly.

 

Cheers,

Lain

Thank you for the assistance and clarification, it's working now.