Update version to 2.0.0 and target ODL 8 and .NET 8.0

[habbes]

Issues

The version constraint excludes ODL 8.

This causes the following warning when you install Microsoft.OData.ModelBuilder alongside Microsoft.OData.Edm 8.0.0.

Description

The PR changes the target framework to .NET 8 and ODL version to ODL 8.0.0.

It also made the following changes:

• Update test to expect the default scale value to be null instead of 0. The default scale value was actually changed to null in ODL 7.11.0. But ModelBuilder was still targeting ODL 7.9.0, so this issue was never caught.
• Replaced calls to the deprecated EntityType() extension method with the EntityType property
• Improved mocking logic to fix some failing tests that resulted from a breaking change in GetCustomAttributes that was introduced in .NET 7. This is a bit involved, so I'll explain that in detail below.

Notes on property attribute mocking and failing tests

After I changed the target framework to .NET 8.0.0, 24 tests failed with an InvalidCastException error:

The exception was thrown from this statement in .NET

This codepath is reached when we call a method like propertyInfo.GetCustomAttribute<SomeAttributeType>() or propertyInfo.GetCustomAttributes<SomeAttributeType>().

And we're doing that a couple of places, like on ODataConventionModelBuilder class where we check presence of the ContainedAttribute to determine whether a navigation property is contained.

Why was the InvalidCastException being thrown?

The exception was being thrown because it so happens that the element.GetCustomAttributes(attributeType, inherit) was return an empty array of type object[]. And .NET was trying to cast that to Attribute[]. This cast fails and throws the exception.

Why was the exception not thrown before changing the target to .NET 8.0.0?

Previously, our target frameworks were .NET standard 2.0 and .NET 6.0. Prior to .NET 7, the custom attribute code used to look like this

(element.GetCustomAttributes(attributeType, inherit) as Attribute[])!

Since this uses the as keyword is used for the cast, it returns null instead of throwing an exception when the cast fails.

2 years ago (i.e. .NET 7), this PR changed the implementation to:

(Attribute[])element.GetCustomAttributes(attributeType, inherit)

which throws an exception if the cast fails.

Why didelement.GetCustomAttributes(attributeType, inherit) return an object[] array instead of Attribute[] array?

It so happens that in the tests that fail, the property info was not an actual PropertyInfo, but a mock. We use helper methods in [MockType.cs]((https://github.com/OData/ModelBuilder/blob/main/test/Microsoft.OData.ModelBuilder.Tests/Commons/MockType.cs) to mock Type and PropertyInfo instances for use in tests.

The MockType.Property() method adds a mock property info to a type and is supposed to mock the inherited MemberInfo.GetCustomAttributes() method as well.

It so happens this method has two overloads:

• object[] GetCustomAttributes(bool inherit) which is supposed to return all attributes regardless of the attribute type
• object[] GetCustomAttributes(Type attributeType, bool inherit) which should return only attributes of the specified type

Note that the declared return type of these methods is object[]. Despite the return type being so open, the excepted return type should be Attribute[] for the first overload and an array of the target attribute type for the other method.

It also so happens that only the first overload was being mocked:

The second overload was not being mocked. If a method is not mocked, I think Moq (the mocking library we use) generates a mock method that returns a default instance of the declared return type, which in this case would be an empty array of type object[] (and not Attribute[]).

Coincidentally, the GetCustomAttribute<T>() and GetCustomAttributes<T>() extension methods called by ModelBuilder call the second MemberInfo.GetCustomAttributes(Type attributeType, bool inherit) overload. Which in the case of this mock, returns an empty object[] array, which the implementation attempts to cast to Attribute[] resulting in the InvalidCastException.

If the overload was no mocked, then how did tests that tested against custom attributes like RequiredAttribute pass without issues before?

Yes, we do have some tests that verify that specific attributes are handled properly by the model builder, like this one

It so happens that some parts of model builder call propertyInfo.GetCustomAttributes() and then perform a filter on the result (e.g. GetCustomAttributes().OfType<T>(), and other parts call proptertyInfo.GetCustomAttribute<T>(). Code that use the first variant was not affected because they used the overload that was already mocked.

I also noticed that after introducing the fix, I didn't get failing tests that tested the ContainedAttribute. Yet this was one of the code paths leading to the failing tests. I realized that existing tests that tested the ContainedAttribute did not use the mock approach, but used real types that had the Contained attribute applied to some properties. Since they did not use the mock, they were not affected by the breaking change.

To make the sure the fix to the mocking logic works, I added a test that verifies the behaviour of the Contained attribute using mocks.

But my main take away is that we should avoid relying on mocks for types that are used in ways we do not control or that can be changed in ways we cannot predict.

Checklist (Uncheck if it is not completed)

• [x] Test cases added
• [x] Build and test with one-click build and test script passed

[xuzhg]

why don't update the version to 2.0.0? and target to .NET 8 also?

[habbes]

@xuzhg that's an option we can pursue, I didn't want to make a major breaking change unless it was necessary to support ODL 8. In the case of ModelBuilder, it's not necessary. But I can go ahead and make that change since .NET 6.0 is going out of support soon.

[habbes]

Build pipeline succeeded: https://identitydivision.visualstudio.com/OData/_build/results?buildId=1346886&view=logs&j=12f1170f-54f2-53f3-20dd-22fc7dff55f9

[habbes]

The nightly pipeline needs to be fixed. Pipelines should also be ported to 1ES.