Runtime: Port more of System.ComponentModel.DataAnnotations types

[marek-safar]

in System.ComponentModel.Annotations

https://msdn.microsoft.com/en-us/library/system.componentmodel.dataannotations.associatedmetadatatypetypedescriptionprovider(v=vs.110).aspx

[karelz]

@divega @weshaggard can you chime in why we skipped this type?
@marek-safar why do you think it is needed?

[marek-safar]

It's part of public .NET API and Xamarin APIs, it's not obsolete so not sure why it's not part of corefx

[karelz]

In general: @weshaggard removed ad-hoc some parts of the Desktop and Xamarin intersection (based on complexity/usage/usefulness) - you might want to sync up with him on details about the criteria used.
cc @danmosemsft

[weshaggard]

Yes I did that as part of the netstandard work however System.ComponentModel.DataAnnotations is not part of netstandard so it wasn't included in my analysis. We still need to do further upstack comparisons for bringing APIs back.

[danmosemsft]

What kind of upstack comparison do you have in mind?

[weshaggard]

Any library that we ship that is above netstandard, like Registry (which was recently done) and DataAnnotations, we should probably look at everything else that is above netstandard that ships inbox on .NET Framework.

[danmosemsft]

Do you plan to do this :)

[weshaggard]

No it isn't on my list of things to do.

[ajcvickers]

@weshaggard Who is responsible for doing this across all the .NET libraries?

[weshaggard]

@danmosemsft has been looking at this more generally but ultimately it is up to the library owners to ensure they have the correct surface area for their library.

[ajcvickers]

@weshaggard @danmosemsft How are we defining "correct" at this point? We dropped stuff that we didn't think was useful; I don't think that analysis changes. But if the goal is now as much compat as possible, then we should bring them all back. Happy to do either.

[weshaggard]

.NET Core 2.0 is definitely taking a big pivot towards compat over pure cleanliness. So I would suggest you add all the APIs back unless there is a strong reason to keep some of them out.

[ajcvickers]

@weshaggard Will do.
@lajones @divega Putting this in 2.0 and updating it to indicate that we will bring all APIs back.

[lajones]

I would imagine this covers dotnet/corefx#18075 as well.

[ajcvickers]

We have re-evaluated this based on what other teams are doing in similar "peripheral" assemblies. At this time we will consider bringing API's back where we feel that there is a real-world customer-based scenario that is broken by not having the API present. We won't do the work to bring back an API just because it is missing. However, we will be more likely to bring back an API if there is a customer PR to do so.

Marking this for re-triage and giving @marek-safar a chance to comment on how the API would be used and submit a PR if desired.

[harsimranb]

Microsoft's RESTier ODATA Library is currently reliant on this API. I was trying to port it to .Net Core 2.0, but have been unable to. I am not too familier with how this API is being using in the OData library. We were looking to use OData in one of projects.

[harsimranb]

@ajcvickers I have been able to successfully import the code from Microsoft ReferenceSource and compile. However, I am not sure what unit tests to write, as there aren't any specs. I realize Mono's implementation has unit tests, but in order to use that, I'd have to keep the file header.

Any suggestions?

[danmosemsft]

@harsimranb it is OK to take code from Mono to use for implementation/tests in CoreFX. You adjust the header as follows:

1. Keep the existing copyright headers in place
    ○ Note: If you are porting just portions of the file over, you still need to bring over all the existing copyright headers.
2. Add the following copyright headers – NOTE this is missing the middle line of the ‘regular’ CoreFx copyright headers:

// Licensed to the .NET Foundation under one or more agreements.
// See the LICENSE file in the project root for more information.

example: https://github.com/dotnet/corefx/blob/master/src/System.ComponentModel.TypeConverter/tests/ContextStackTests.cs

[harsimranb]

Thank you. I've ported tests for AssociatedMetadataTypeTypeDescriptionProvider. There are three additional clases I've added for this API for which I cannot find unit tests: AssociatedMetadataTypeTypeDescriptor, MetadataPropertyDescriptorWrapper, and MetadataTypeAttribute.

That being said, I can add unit tests for MetadataTypeAttribute, it's pretty straightfoward.

[ysmoradi]

@harsimranb Any news? Thanks in advance.

[danmosemsft]

As noted above/in the dupe, we focused on porting S.CM.* that were in System.dll (mostly into our S.CM.dll I guess), not more types from S.CM.DA.dll (which would go into our S.CM.Annotations.dll). It would be great to now port more of S.CM.DA.dll and contributions would be welcome.

[ysmoradi]

I'd like to port following classes of S.CM.DA to .net core from either mono or .net framework.

• AssociatedMetadataTypeTypeDescriptionProvider
• ScaffoldTableAttribute
• MetadataTypeAttribute
• BindableTypeAttribute

As a matter of fact, I need AssociatedMetadataTypeTypeDescriptionProvider only, but I think it's a good idea to make S.CM.DA complete.

I read their codes & tests on both mono & referenceSource.com

Now I've there questions:

1- AssociatedMetadataTypeTypeDescriptionProvider needs AssociatedMetadataTypeTypeDescriptor and that class needs ConcurrentDictionary. What should I do about that?
2- Do I need to follow http://aka.ms/api-review ? or this implicit allow is enough?
3- What should I do if I prefer to add those classes to .net standard?

[karelz]

1 - ConcurrentDictionary is part of .NET Core. What's the problem?
2 - If you just port existing API surface from .NET Framework, then API review is not needed. Just don't alter the original API shape.
3 - You cannot add classes to .NET Standard. .NET Standard versions independently when all platforms catch up with required API surface. Don't worry about it at all at this moment.

@maryamariyan @safern should be able to help with the port.

[ysmoradi]

@karelz Sorry, you're right. My Visual Studio (15.7.3 Prev) wasn't able to add using for ConcurrentDictionary "using System.Collections.Concurrent"
I added that manually to my cs file and now everything works fine.
So, stay tuned for my PR (:

[ysmoradi]

Following is MetadataTypeAttribute's codes copied from reference source:

[AttributeUsage(AttributeTargets.Class, AllowMultiple = false)]
    public sealed class MetadataTypeAttribute : Attribute { 

        private Type _metadataClassType; 

        public Type MetadataClassType {
            get {
                if (_metadataClassType == null) {
                    throw new InvalidOperationException(DataAnnotationsResources.MetadataTypeAttribute_TypeCannotBeNull);
                }

                return _metadataClassType;
            }
        } 

        public MetadataTypeAttribute(Type metadataClassType) {
            _metadataClassType = metadataClassType;
        } 

    }

This is mine:

    [AttributeUsage(AttributeTargets.Class, AllowMultiple = false)]
    public sealed class MetadataTypeAttribute : Attribute
    {
        public Type MetadataClassType { get; }

        public MetadataTypeAttribute(Type metadataClassType)
        {
            MetadataClassType = metadataClassType ?? throw new ArgumentNullException(SR.MetadataTypeAttribute_TypeCannotBeNull);
        }
    }

I think my code is better, because it throws an exception for null constructor parameter earlier, instead of raising an exception later while you get property's value without any condition.

I'd like to apply my own code, but this results into different behavior between DotNetCore & DotNetFull.

What should I do?

[ysmoradi]

I've committed changes to my own fork in one commit. But I'm not going to create a PR for this.
When I try to build using .\build.cmd command, everything works fine. But when I try to run .\build.cmd -allconfigurations to achieve packages to test them on my own app, I receive following errors:

Build FAILED.

System.ComponentModel.Annotations.cs(11,96): error CS0234: The type or namespace name 'TypeDescriptionProvider' does no
t exist in the namespace 'System.ComponentModel' (are you missing an assembly reference?) [D:\bit-foundation\corefx\src
\System.ComponentModel.Annotations\ref\System.ComponentModel.Annotations.csproj]
System.ComponentModel.Annotations.cs(15,47): error CS0234: The type or namespace name 'ICustomTypeDescriptor' does not
exist in the namespace 'System.ComponentModel' (are you missing an assembly reference?) [D:\bit-foundation\corefx\src\S
ystem.ComponentModel.Annotations\ref\System.ComponentModel.Annotations.csproj]
System.ComponentModel.Annotations.cs(15,69): error CS0115: 'AssociatedMetadataTypeTypeDescriptionProvider.GetTypeDescri
ptor(Type, object)': no suitable method found to override [D:\bit-foundation\corefx\src\System.ComponentModel.Annotatio
ns\ref\System.ComponentModel.Annotations.csproj]
    0 Warning(s)
    3 Error(s)

Time Elapsed 00:00:50.27
Command execution failed with exit code 1.

After I found out what's a problem, I will create a correct one in single commit.

[karelz]

What to do? ... In general think/list how the attributes are used, what is the impact on people who pass null (is it their mistake, or is it based on app input?) - will they be broken? Is your "better" solution worth breaking them?

[ysmoradi]

@karelz I appreciate your comment. What about build error, what should I do?

[karelz]

It looks like there is no reference from S.CM.Annotations to S.CM.TypeConvertor where the missing types live. I would try to change the .csproj of the modified ref.

[ysmoradi]

Thanks. Let me know when you managed to make it work :-)

[ysmoradi]

By following csproj config, it works

  <ItemGroup Condition="'$(TargetGroup)' == 'netfx'">
    <Reference Include="mscorlib" />
    <Reference Include="System.ComponentModel.DataAnnotations" />
    <Reference Include="System" />
  </ItemGroup>
  <ItemGroup Condition="'$(TargetGroup)' == 'netcoreapp' OR '$(TargetGroup)' == 'netcoreapp2.0'">
    <Reference Include="System.ComponentModel.TypeConverter" />
  </ItemGroup>

Instead of

<ItemGroup Condition="'$(TargetGroup)' == 'netfx'">
    <Reference Include="mscorlib" />
    <Reference Include="System.ComponentModel.DataAnnotations" />
  </ItemGroup>

Is it ok?

[karelz]

That sounds reasonable - although I wonder why the condition is netcoreapp OR netcoreapp2.0. Shouldn't just one of them be sufficient?
Anyway, it can be discussed as part of code review.

[bjthomson]

@karelz ,@danmosemsft Glad to see this has been done, when will we be seeing it in a new release?

[danmosemsft]

@bjthomson you can get it in 3.0 previews right now, or wait for final release a little later in the year.

[bjthomson]

Thanks @danmosemsft , but our solution is already in pieces on the floor due to various breaking changes, OpenID connect etc.
I was just investigating .net standard to see if we should start using it for general libraries. I've just spent 2 days discovering MetadataTypeAttribute doesn't exist, nor CloudConfigurationManager, so not quite there yet.
Without MetadataType, how would we attach metadata to partial classes relating to Entity Framework?
I found this on Stack Overflow https://stackoverflow.com/questions/34576921/asp-net-core-metadatatype-attribute-not-working
but I really dont want to pollute a domain library with references to AspnetCore.mvc
I'll wait for the final release.

[ImGonaRot]

I too need the MetadataTypeAttribute for partial classes that are being used in a Xamarin client with OData calls.
Any idea when this will be available for Xamarin?