RelationType has a new signature in 8.6

[skttl]

I am seeing this issue on Umbraco version: 8.6

Reproduction

Bug summary

I am building a component that autorelates some nodes in different scenarios. For this I need to be able to create relation types on the fly.

After upgradring from 8.5.5 to 8.6 my code broke, because of a new signature for RelationType

You can see the signature change here:
ae64fe4#diff-4e2dd39e5212f69a9fd9fea7f3b8f425

I create my RelationType like this

            var relationType = _relationService.GetRelationTypeByAlias(relationAlias);
            if (relationType == null)
            {
                relationType = new RelationType(UmbracoObjectTypes.Document.GetGuid(), UmbracoObjectTypes.Document.GetGuid(), relationAlias, relationName);
                _relationService.Save(relationType);
            }

After upgrading to 8.6, this should be

            var relationType = _relationService.GetRelationTypeByAlias(relationAlias);
            if (relationType == null)
            {
                relationType = new RelationType(relationAlias, relationName, false, UmbracoObjectTypes.Document.GetGuid(), UmbracoObjectTypes.Document.GetGuid());
                _relationService.Save(relationType);
            }

Expected result

I would expect that some kind of backwards compatibility would be introduced with this.

Actual result

I get YSODs all over the place, and need to have two versions of my code depending on the Umbraco versions.

[ronaldbarendse]

Looks like this TODO was added, but not fixed before releasing 8.6.0:

Umbraco-CMS/src/Umbraco.Core/Models/RelationType.cs

Line 20 in a1769dd

//TODO: Should we put back the broken ctors with obsolete attributes?

[nul800sebastiaan]

Hmm, I checked that method signature was put back and I swear I saw it before release... Not sure how I saw that

We'll fix for 8.6.1.

[skttl]

Thanks :)

[nul800sebastiaan]

"Fixed" in 055f18d

[ronaldbarendse]

@nul800sebastiaan "Partially fixed", as it's still missing one of the old constructors (containing the name parameter and that's actually used in the example above):

Umbraco-CMS/src/Umbraco.Core/Models/RelationType.cs

Lines 20 to 38 in ed90e71

	public RelationType(Guid childObjectType, Guid parentObjectType, string alias)
	{
	if (alias == null) throw new ArgumentNullException(nameof(alias));
	if (string.IsNullOrWhiteSpace(alias)) throw new ArgumentException("Value can't be empty or consist only of white-space characters.", nameof(alias));
	_childObjectType = childObjectType;
	_parentObjectType = parentObjectType;
	_alias = alias;
	Name = _alias;
	}
	public RelationType(Guid childObjectType, Guid parentObjectType, string alias, string name)
	: this(childObjectType, parentObjectType, alias)
	{
	if (name == null) throw new ArgumentNullException(nameof(name));
	if (string.IsNullOrWhiteSpace(name)) throw new ArgumentException("Value can't be empty or consist only of white-space characters.", nameof(name));
	Name = name;
	}

The obsolete constructor also uses the obsolete ArgumentNullOrEmptyException that was replaced with a seperate ArgumentNullException and ArgumentException as part of PR #6599.

The new constructors also have the name and alias swapped between the two, so adding the isBidrectional, parentObjectType and childObjectType parameters to the most basic constructor will result in using the name as alias and vice-versa:

Umbraco-CMS/src/Umbraco.Core/Models/RelationType.cs

Lines 21 to 33 in 055f18d

	public RelationType(string alias, string name)
	: this(name, alias, false, null, null)
	{
	}
	public RelationType(string name, string alias, bool isBidrectional, Guid? parentObjectType, Guid? childObjectType)
	{
	_name = name;
	_alias = alias;
	_isBidrectional = isBidrectional;
	_parentObjectType = parentObjectType;
	_childObjectType = childObjectType;
	}

Besides having the old, now obsolete, constructors, the new constructor could be simplified using optional parameters, so we end up with the following constructors:

public RelationType(string name, string alias, bool isBidrectional = false, Guid? parentObjectType = null, Guid? childObjectType = null)
{
    if (name == null) throw new ArgumentNullException(nameof(name));
    if (string.IsNullOrWhiteSpace(name)) throw new ArgumentException("Value can't be empty or consist only of white-space characters.", nameof(name));

    if (alias == null) throw new ArgumentNullException(nameof(alias));
    if (string.IsNullOrWhiteSpace(alias)) throw new ArgumentException("Value can't be empty or consist only of white-space characters.", nameof(alias));

    _name = name;
    _alias = alias;
    _isBidrectional = isBidrectional;
    _parentObjectType = parentObjectType;
    _childObjectType = childObjectType;
}

[Obsolete("Use the constructor with all parameters instead.")]
public RelationType(Guid childObjectType, Guid parentObjectType, string alias)
    : this(alias, alias, false, childObjectType, parentObjectType)
{ }

[Obsolete("Use the constructor with all parameters instead.")]
public RelationType(Guid childObjectType, Guid parentObjectType, string alias, string name)
    : this(name, alias, false, childObjectType, parentObjectType)
{ }

Fixing this will introduce another breaking change though 😢 Keep in mind that even making the parameters optional will result in a binary breaking change: breaking all code compiled against version 8.6.0 and require recompiling, which completely breaks backwards (and forwards) compatibility for all packages using RelationType!

[nul800sebastiaan]

Ah sure, I'll add that. It's all quite academic at this point I guess since it's "too late" for proper package support.

[ronaldbarendse]

Yes, for version 8.6.0 it's indeed too late now... So lets make sure it's properly fixed in the next (patch) release! 😉 I can create a quick PR for this (if you don't mind).

[nul800sebastiaan]

Oh no.. I've asked @Shazwazza to have a look at that one since I'm not sure if changing:

public RelationType(string alias, string name)
            : this(name, alias, false, null, null)

to

public RelationType(string alias, string name)
            : this(alias, name, false, null, null)

would be a breaking change between 8.6.0 and 8.6.1 😬

The other ctor has been added back in 0d7b0b9

[Shazwazza]

I've fixed this up in rev: b72abd5

Here's what the ctors look like now

[Obsolete("This constructor is no longer used and will be removed in future versions, use one of the other constructors instead")]
public RelationType(string alias, string name)
    : this(name, alias, false, null, null)
{
}

public RelationType(string name, string alias, bool isBidrectional, Guid? parentObjectType, Guid? childObjectType)
{
    _name = name;
    _alias = alias;
    _isBidrectional = isBidrectional;
    _parentObjectType = parentObjectType;
    _childObjectType = childObjectType;
}

[Obsolete("This constructor is no longer used and will be removed in future versions, use one of the other constructors instead")]
public RelationType(Guid childObjectType, Guid parentObjectType, string alias)
{
    if (alias == null) throw new ArgumentNullException(nameof(alias));
    if (string.IsNullOrWhiteSpace(alias)) throw new ArgumentException("Value can't be empty or consist only of white-space characters.", nameof(alias));

    _childObjectType = childObjectType;
    _parentObjectType = parentObjectType;
    _alias = alias;
    Name = _alias;
}

[Obsolete("This constructor is no longer used and will be removed in future versions, use one of the other constructors instead")]
public RelationType(Guid childObjectType, Guid parentObjectType, string alias, string name)
    : this(childObjectType, parentObjectType, alias)
{
    if (name == null) throw new ArgumentNullException(nameof(name));
    if (string.IsNullOrWhiteSpace(name)) throw new ArgumentException("Value can't be empty or consist only of white-space characters.", nameof(name));

    Name = name;
}

[ronaldbarendse]

@Shazwazza That looks great, but just to verify: is it correct that both name and alias can be null or empty in the new constructors? If not, those checks could be moved to the RelationType(string name, string alias, bool isBidrectional, Guid? parentObjectType, Guid? childObjectType) constructor and all obsolete ones should call this (and have no code in the body).

[Shazwazza]

thanks @ronaldbarendse good catch and have updated