Instantiating different kinds of error records for a database operation

Question

I've written code to be called from Powershell like so:

Rename-DatabaseUser -Name 'someuser' -NewName 'somenewuser'

I'm wondering if there's another way I can approach this. I want to create objects of the same type with slightly differing property values.

Here's the code I'm currently using now (RenameDatabaseUserCmdlet.cs):

namespace Company.Powershell.Module.Cmdlets
{
    using System.Management.Automation;

    [Cmdlet(VerbsCommon.Rename, "DatabaseUser")]
    [Alias("rndbu")]
    public class RenameDatabaseUserCmdlet : PSCmdlet
    {
        [Parameter(ParameterSetName = "Object")]
        public DatabaseUser User { get; set; }

        [Parameter(ParameterSetName = "String")]
        public string Name { get; set; }

        [Parameter(ParameterSetName = "Object")]
        [Parameter(ParameterSetName = "String")]
        public string NewName { get; set; }

        protected override void ProcessRecord()
        {
            DatabaseUser user;

            if (ParameterSetName == "String")
            {
                var ps = PowerShell.Create(RunspaceMode.CurrentRunspace);
                ps.AddCommand("Get-DatabaseUser")
                  .AddParameter("Name", Name);

                foreach (var result in ps.Invoke())
                {
                    user = result?.BaseObject as DatabaseUser;
                    if (user == null)
                    {
                        // This is where I'm not sure I'm doing it right.
                        WriteError(DatabaseErrorRecord.UserNotFound(Name));
                        return;
                    }
                }
            }
            else
            {
                user = User;
            }

            // Still more to do here...
        }
    }
}

This is how I create the objects. In DatabaseErrorRecord I have multiple static methods which return an ErrorRecord object initialised slightly differently. These are some of the methods:

namespace Company.Powershell.Module
{
    using System;
    using System.Management.Automation;

    class DatabaseErrorRecord : ErrorRecord
    {
        // This is added just so it compiles, which is why I think I'm wrong.
        private DatabaseErrorRecord(Exception exception,
            string errorId,
            ErrorCategory errorCategory,
            object targetObject) : base(exception, errorId, errorCategory, targetObject)
        { }

        public static ErrorRecord UserNotFound(string userName)
        {
            return new ErrorRecord
            (
                new NullReferenceException($"The user '{userName}' could not be found in the database."),
                "UserNotFound",
                ErrorCategory.ObjectNotFound,
                null
            );
        }

        public static ErrorRecord DatabaseNotFound(string databaseName)
        {
            return new ErrorRecord
            (
                new NullReferenceException($"The database '{databaseName}' could not be found on the SQL Server instance."),
                "DatabaseNotFound",
                ErrorCategory.ObjectNotFound,
                null
            );
        }

        public static ErrorRecord PermissionConflict(string permissionA, string permissionB)
        {
            return new ErrorRecord
            (
                new InvalidOperationException($"Granting {permissionA} to this user would cause a conflict with {permissionB}."),
                "PermissionConflict",
                ErrorCategory.InvalidOperation,
                null
            );
        }
        // Other methods omitted...
    }
}

What is a good way to create objects of the same type, using slightly different arguments?

In each of these methods, when the ErrorRecord is created. The Exception, Error ID and Error Category will all be different. I would create these objects when I need them in code, but there's some instances where I would need the same object twice.

I'm still thinking this is wrong since I have to include the constructor despite the fact I will never use it.

Is there any other ways I could structure this?

Answer 1

There are no established scenarios for deriving from this class.

MSDN ErrorRecord

Which leads me to believe that there's no reason to derive a child class. Instead, change the name of DatabaseErrorRecord to DatabaseErrorRecordFactory and make it static. Do not derive from ErrorRecord. Instead, just have a static class with a bunch of static methods that return ErrorRecords. This will remove the need to have any constructor at all.

public static class DatabaseErrorRecord
{
    public static ErrorRecord UserNotFound(string userName)
    {
        return new ErrorRecord
        (
            new NullReferenceException($"The user '{userName}' could not be found in the database."),
            "UserNotFound",
            ErrorCategory.ObjectNotFound,
            null
        );
    }
    // Other methods omitted...
}

---

Side note: I really like that you're taking advantage of the new C# 6 features. I'm a big fan of the Elvis operator.

One other thing I'd consider changing is that NullReferenceException though. It doesn't feel appropriate. An argument exception might be okay, but I think I'd create my own custom exception for this one.

Answer 2

Targeting only the ProcessRecord() method.

You have a flaw here, because if the given Name is not found then the resulting Collection<PSObject> from the call to ps.Invoke() won't likely contain any items, hence the foreach won't iterate. This will result in user being null.
The other thing I am concerned is, if it is possible that the user's name isn't unique, the user variable will be overwritten for each iteration of the loop.

If the Name is unique, you shouldn't use a foreach at all and change the code to

        var result = ps.Invoke();
        if (result.count == 0)
        {
            // Database user not found
            // write your error

            return;
        }
        // assuming unique `Name`
        user = (DatabaseUser)result[0].BaseObject;

I have replaced the soft cast with a regular cast because IMO we can be sure that a DatabaseUser is returned at this point.