Microsoft.SqlServer.Management.Smo.Database - .SetOwner() does not change .Owner without .Refresh()

[andreasjordan]

I've opened the issue here: microsoft/sqltoolsservice#1131

But maybe the issue has to be in this repo - not sure.

[shueybubbles]

this is the right forum , I will close the other one.

thx

[shueybubbles]

Andreas Jordan (@andreasjordan) is there any scenario where the user-provided string succeeds to set the owner, but fetching the value from SQL Server might return something different? For example, can one specify a Windows user or a Windows authentication login multiple ways to identify the same user such that the set accepts multiple formats but the get would always return a canonical format?

[andreasjordan]

Just did a test: You can only provide Windows logins in the form "DOMAIN\Account", the form "Account@Domain" is not allowed.

[jzabroski]

Just did a test: You can only provide Windows logins in the form "DOMAIN\Account", the form "Account@Domain" is not allowed.

It is true that the form "Account@Domain" is not allowed, but you can specify a SID when you create a login. The way SQL Server works, the SID is what is presented to Kerberos authentication to get the list of sys.login_tokens and associated permissions context. Once a SID exists in the principals tables (both server_principals and database_principals), you can change the name in Active Directory from DOMAIN\Acccount to DOMAIN\AccountFirstName.AccountLastName to follow a new naming convention, and SQL Server will still work. However, you can't drop the user or login using the name AD has. You have to drop it via its SQL Server database_principals or server_principals name.

So, in this sense, if you're using the AccountManagement API and SMO together, then I guess there is "scenario where the user-provided string succeeds to set the owner, but fetching the value from SQL Server might return something different", but not strictly from within SMO.

Hope that helps.

[andreasjordan]

Sorry, John Zabroski (@jzabroski) - the form of the login is just not relevant for the original problem. Just David Shiflet (@shueybubbles) asked about it and so I answered.

Let me copy the original problem here (sorry - I opend it in the wrong repo):

I found this issue while working on dbatools, so this is the related issue there:
dataplat/dbatools#7010

Here is my demo code (I hope it's ok to use dbatools to get the smo):

$server = Connect-DbaInstance -SqlInstance SRV1
$db = $server.Databases['Test101']
$db.Owner
$db.SetOwner('sa')
$db.Owner
$db.Refresh()
$db.Owner

And this is how it looks like:

If you need additional info, I would be happy to provide it.
If you could point me to the source code of that SetOwner method, I could also have a look myself.

[shueybubbles]

I don't know the origins of why Owner is a read-only property on Database. I'd have expected it to support being changed via Alter just like any other property, assuming you don't also want to drop the existing owner at the same time like the SetOwner method allows. But the current behavior is clearly a bug, the code is just missing a line to set the internal property dictionary's "Owner" key to the new value. I'll get it fixed at some point.

[jzabroski]

I don't know the origins of why Owner is a read-only property on Database.

Well, it's not read-only. It's just that the property only exposes a getter.

See: https://docs.microsoft.com/en-us/dotnet/api/microsoft.sqlserver.management.smo.database.owner?view=sql-smo-160#Microsoft_SqlServer_Management_Smo_Database_Owner

public string Owner { get; }

Andreas Jordan (@andreasjordan) Sorry, away from a keyboard, but:

1. what happens if you change the owner to a Windows AD user?
2. what happens if you change the owner to a Windows AD user w/ dropExistingUser: true

My thinking is that maybe it is this way due to someone not accounting for the scenario where the string loginName value is a valid AD User but the SQL Server server_principals table has a different login name for that AD User's SID?

I'd need to run some code to prototype that scenario, but this scenario HAPPENS A LOT in my career when I show up to a job.

I could be completely wrong and off course. Your code example, however, does not show my test case.

It's also possible it's just a bug and that before GitHub came along, we had very difficult time getting bugs submitted through Connect to engineers who can solve them like David Shiflet.

[andreasjordan]

As the code is now open source, let my try to work on this again. Either by solving or problem or by understanding the used concept.

It looks like this is the relevant code:

sqlmanagementobjects/src/Microsoft/SqlServer/Management/Smo/DatabaseBase.cs

Lines 1403 to 1472 in 9809360

	/// <summary>
	/// Implementation to change the owner of this database to the login with the specified name.
	/// </summary>
	/// <param name="loginName">The name of the login to be the new owner</param>
	/// <param name="dropExistingUser">Whether to drop existing non-dbo users before changing the owner</param>
	private void SetOwnerImpl(string loginName, bool dropExistingUser)
	{
	try
	{
	if (null == loginName)
	{
	throw new SmoException(ExceptionTemplates.InnerException, new ArgumentNullException("loginName"));
	}
	// Owner is a read-only property.
	// To stay true to the design principle in design mode that dictates that we keep the same access
	// pattern to objects, we can only set the owner by calling the setowner method.
	// The non-design mode code actually doesn't set the owner property, it only propagates the new
	// owner value to the server
	if (this.IsDesignMode)
	{
	Property owner = this.Properties.Get("Owner");
	owner.SetValue(loginName);
	owner.SetRetrieved(true);
	}
	else
	{
	var query = new StringCollection();
	InsertUseDb(index: 0, col:query, targetEngineType: this.DatabaseEngineType);
	//Check to see if login is already mapped to a non-dbo user in this database.
	//If it is and the caller has specified to drop any existing users then
	//we add a query to do so. If the user hasn't specified to drop
	//existing users then we skip this completely - though the query to change
	//the owner is likely to fail since you can't have a login mapped to more
	//one user in a db.
	if (dropExistingUser)
	{
	Login l = ((Server)ParentColl.ParentInstance).Logins[loginName];
	if (null == l)
	{
	throw new SmoException(ExceptionTemplates.InnerException,
	new ArgumentException(ExceptionTemplates.InvalidLogin(loginName)));
	}
	//Find the user that maps to our target login - if one exists. Script
	//the drop for that user if it's anything but dbo (since that can't be
	//dropped and means the login is already the owner)
	User user = this.Users.Cast<User>().FirstOrDefault(u => u.Login.Equals(loginName));
	if (user != null && !user.Name.Equals("dbo", StringComparison.Ordinal))
	{
	user.ScriptDrop(query, this.GetScriptingPreferencesForCreate());
	}
	}
	//Script the query to actually change the owner
	this.ScriptChangeOwner(query, loginName);
	this.ExecutionManager.ExecuteNonQuery(query);
	}
	}
	catch (Exception e)
	{
	FilterException(e);
	throw new FailedOperationException(ExceptionTemplates.SetOwner, this, e);
	}
	}

To me it looks like that this code from "in design mode" is the one that is missing in the other branch:

                if (this.IsDesignMode)
                {
                    Property owner = this.Properties.Get("Owner");
                    owner.SetValue(loginName);
                    owner.SetRetrieved(true);
                }

In the other branch of this "if" the actual T-SQL is executed, but the value is not set on the object.

[andreasjordan]

Let me add a screenshot that shows the current behaviour:

I think it is not correct if instance is changed, but SMO still has the old value.

And Alter() should be just needed in the other way: First change only SMO, and change instance when using Alter().

[shueybubbles]

If we change SetOwnerImpl I'd want to modify the query it uses to return the new owner after setting it. As John Zabroski (@jzabroski) mentioned, I think there are scenarios where the value passed to this function ends up being different than the value that would be returned after Refresh(). EG instead of ExecuteNonQuery it would use ExecuteScalar which returns a string.
Secondly, we'd need to examine the Database object for other properties that depend on referential integrity with the Owner. Maybe the requirement to call Refresh here was to make sure other properties that depend on the Owner are set correctly.

[jzabroski]

Agree, maybe start by writing some failing unit tests. I think there are some common workflows (multiple SUT calls) that are more "integration tests" than "unit tests" but nonetheless we need some way to test and validate.

The other question on my mind is whether this constitutes a major version bump or not. I would side towards yes, since it is a fairly major breaking change, even if I think the behavior is incorrect.

[andreasjordan]

What's the current status? I think it's still not fixed.

[andreasjordan]

Any updates?

[andreasjordan]

Still reproducible with SMO 18.100.1.19: after SetOwner('sa') the server reports sa while Database.Owner keeps the old value until Refresh(). Since the code is open source now, I have opened #231 with a fix: instead of storing the caller-provided name (which may not match the canonical name, as discussed above), it invalidates the cached Owner property so the next access fetches the canonical value from the server - the same pattern CleanObject already uses for the default language properties.

This was created by Claude and reviewed by Andreas Jordan.