@Harm_Veenstra's reply contains three important new elements:
- The first is the one he called out in relation to [string]::IsNullOrWhitespace;
- The second is repositioning the $null checks from the format of "<something> -eq $null" to "$null -eq <something>";
- 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.
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.
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
@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:
@LainRobertson
Still not working.
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;
}
#endregionThen it would only skip a user if the EmployeeID was blank, not the uidNumber which is what it should be setting.