Support for properties renamed with [DataMember] in OData models

[brainboost]

1. Put the [DataContract] and [DataMember] attributes on the OData model

    [DataContract]
    public class Contact
    {
        [DataMember]
        public int ContactId { get; set; }
        [DataMember(Name = "first_name")]
        public string FirstName { get; set; }
        [DataMember]
        public string LastName { get; set; }
        [DataMember]
        public string Email { get; set; }
        [DataMember]
        public string Phone { get; set; }
        [DataMember]
        public List<Address> Addresses { get; set; }
    }

2. Prepare the unit test expecting the property renamed in the contract

    public class DefaultModelTypeBuilderTest
    {
     ...
        [Fact]
        public void substituted_type_should_have_renamed_with_attribute_properties_from_original_type()
        {
            // arrange
            var modelBuilder = new ODataConventionModelBuilder();
            modelBuilder.EntitySet<Contact>( "Contacts" );

            var context = NewContext( modelBuilder.GetEdmModel() );
            var originalType = typeof( Contact );

            // act
            var substitutedType = originalType.SubstituteIfNecessary( context );

            // assert
            substitutedType.Should().HaveProperty<string>(nameof( Contact.FirstName ) );
        }

3. Get the failed this test and some others, as the property FirstName is disappeared from the type as well as from the Swagger generated model documentation.

In order to fix this behavior I made changes in DefaultModelTypeBuilder, particularly in lines 106-115, method GenerateTypeIfNeeded.

 if ( !structuralProperties.TryGetValue( property.Name, out var structuralProperty ) )
 {
     var name = GetNameFromAttribute( property );
     if ( string.IsNullOrEmpty( name ) || !structuralProperties.TryGetValue( name, out structuralProperty ) )
     {
         clrTypeMatchesEdmType = false;
         continue;
     }
 }

...
        static string GetNameFromAttribute( MemberInfo property )
        {
            return property.GetCustomAttribute<System.Runtime.Serialization.DataMemberAttribute>()?.Name;
        }

I'd be happy to provide PR with the working solution as I did my changes in the forked repo. Should I?

[brainboost]

PR #512

[commonsensesoftware]

Interesting... I thought this was only for deserialization. I guess it can be used for serialization too.

Supporting [DataContract] and [DataMember] is not unreasonable; however, relying on reading the custom attributes is undesirable. The goal and job of the IModelTypeBuilder is to construct a type based on exactly what the EDM says. That simplifies a lot of problems.

It would seem there is a current limitation that is affecting you where the CLR property name is expected to case-insensitively match the EDM property name, which may not always be the case. The more appropriate way to address this would be to match the CLR property to the EDM property directly as a fallback mechanism (as matching by name is relatively cheap). This is achieved by querying the annotation applied to the EDM structural property. If ClrPropertyInfoAnnotation.ClrPropertyInfo equals the current CLR property, then you know you've matched the correct EDM structural property, regardless of name.

Overall, you're on the right track, but it's important that everything derives directly from the EDM because this is how things will be transmitted over the wire. We also don't want to duplicate logic and should avoid making assumptions about how the EDM achieved it's mapping.

I hope that helps you continue. I'll take a deeper look and provide some feedback as I have time this week. Thanks.

[commonsensesoftware]

I could have just made the require changes and pushed it up, but I wanted to give you the PR credit since you already started doing the working. I've reviewed your PR and added appropriate comments. I've given you the additional code required to map EDM properties to CLR properties regardless of name, which will cover the scenario of using an alternate name, including the use of [DataMember]. Most everything else appears to be just be spacing, formatting, or other code analysis stuff that should be quick and easy to fix. If you're using Visual Studio, simply running the Format Document command should address most issues based on the .editorconfig policies.

[brainboost]

Shouldn't it be closed with #520? It had #512 and 520 is overriding it, isn't it?

[commonsensesoftware]

Yeah... It's probably just because it didn't have the correct metatag in the commit. Not a huge deal. I just added a few other minor changes that about to merge. It contains all these changes as well, plus the release notes. Once the release build is done and ready for publishing, we can close this; manually, if necessary.

[commonsensesoftware]

The packages have been pushed and you should see them appear after validation and indexing within the next 15 minutes or so. Thanks.