SetHasOneForeignKeyValue detects string as non-nullable type and throws exception

[PiotrBrzezianski]

Description

I have a 0..1 to 1 relationship using foreign key of type string. As string can be null so it should be fine to use it as a foreign key in this scenario but I'm getting an exception thrown from SetHasOneForeignKeyValue method:

"Cannot set required relationship identifier '{hasOneAttr.IdentifiablePropertyName}' to null because it is a non-nullable type."

I believe this is because the method uses this code for the check: Nullable.GetUnderlyingType(foreignKeyProperty.PropertyType)
which returns null for type string.

That is because GetUnderlyingType only works as expected here for types derived from Nullable which string is not.
https://referencesource.microsoft.com/#mscorlib/system/nullable.cs,132
...

Environment

• JsonApiDotNetCore Version: 4.0.0-alpha3

[maurei]

Thanks for pointing this out!

In which scenario are you encountering this issue? I'll need a little reproduction case to be able to address this issue further. You could make a PR with a test that exposes this bug or share a repo with an example app that exposes the bug.

[PiotrBrzezianski]

Thanks for a quick reply @maurei !
Apologies for not following the process but it is end of day for me. I can add more details tomorrow but if that helps in the meantime here is the bit of code (modified by me) in JsonApiDeSerializer I suspect to be the cause.

private void SetHasOneForeignKeyValue(object entity, HasOneAttribute hasOneAttr, PropertyInfo foreignKeyProperty, ResourceIdentifierObject rio)
        {
            var foreignKeyPropertyValue = rio?.Id ?? null;
            if (foreignKeyProperty != null)
            {
                // in the case of the HasOne independent side of the relationship, we should still create the shell entity on the other side
                // we should not actually require the resource to have a foreign key (be the dependent side of the relationship)

                // e.g. PATCH /articles
                // {... { "relationships":{ "Owner": { "data": null } } } }
                bool foreignKeyPropertyIsNullableType = Nullable.GetUnderlyingType(foreignKeyProperty.PropertyType) != null 
                    || foreignKeyProperty.PropertyType == typeof(string);
                if (rio == null && !foreignKeyPropertyIsNullableType)
                    throw new JsonApiException(400, $"Cannot set required relationship identifier '{hasOneAttr.IdentifiablePropertyName}' to null because it is a non-nullable type.");

                var convertedValue = TypeHelper.ConvertType(foreignKeyPropertyValue, foreignKeyProperty.PropertyType);
                foreignKeyProperty.SetValue(entity, convertedValue);
                _jsonApiContext.RelationshipsToUpdate[hasOneAttr] = convertedValue;
            }
        }

I think adding string as a special case might be enough as I can't think what else anyone could use that is not int?, guid? or string.

bool foreignKeyPropertyIsNullableType = Nullable.GetUnderlyingType(foreignKeyProperty.PropertyType) != null 
                    || foreignKeyProperty.PropertyType == typeof(string);

[maurei]

Just wondering: since you're using FKs of type string, what kind of identifiers are you using?

[PiotrBrzezianski]

Hi @maurei - the PR is there with a new test which should hopefully explain the scenario better.
And to answer your question there are two main cases where I've seen use of string keys:

1. I'm using string IDs generated from EventFlow library. They put the entity name in front of the guid which helps with debugging a bit but it also helps to make sure you cannot pass author guid as book id by accident. Conveniently they also make sure the guids are sequential which helps with sql indexing...
2. Sometimes it makes sense to use a "natural key" if the resource data already includes a string that can uniquely identify the resource (and it should never change), e.g. some national identification number for a person or an email for a user.

[maurei]

Okay I was wondering whether you were doing some bad practise with respect to your primary keys but this seems like a totally valid usecase! I will review your PR shortly.

[maurei]

We'll release this in alpha4 which I'm expecting be published by the end of next week