Add support for derived type constraint annotation - #1977
Conversation
| /// </summary> | ||
| public ActionConfiguration AddDerivedTypeConstraint<TSubClass>() where TSubClass : class | ||
| { | ||
| base.AddDerivedTypeConstraintToReturnTypeImplementation(typeof(TSubClass)); |
There was a problem hiding this comment.
base [](start = 12, length = 4)
"base. " is unnecessary? #Resolved
| /// </summary> | ||
| public CollectionPropertyConfiguration AddDerivedTypeConstraint<TSubClass>() where TSubClass : class | ||
| { | ||
| this.AddDerivedTypeConstraintImpl(typeof(TSubClass)); |
There was a problem hiding this comment.
this [](start = 12, length = 4)
"this." is unnecessary? #Resolved
There was a problem hiding this comment.
| /// <summary> | ||
| /// Adds TSubClass to the list of derived type constraints. | ||
| /// </summary> | ||
| public SingletonConfiguration AddDerivedTypeConstraint<TSubClass>() where TSubClass : class |
There was a problem hiding this comment.
AddDerivedTypeConstraint() [](start = 38, length = 37)
AddDerivedTypeConstraint() [](start = 38, length = 37)
For design perspective, add a non-generic method to support adding TypeConstraint.
Call the non-generic method in the generic method. #Resolved
There was a problem hiding this comment.
There was a problem hiding this comment.
somthing likes public SingletonConfiguration AddDerivedTypeConstraint(params Type type) {}
In reply to: 358416861 [](ancestors = 358416861,358416790)
|
| private readonly Dictionary<PropertyInfo, IEdmProperty> _properties = new Dictionary<PropertyInfo, IEdmProperty>(); | ||
| private readonly Dictionary<IEdmProperty, QueryableRestrictions> _propertiesRestrictions = new Dictionary<IEdmProperty, QueryableRestrictions>(); | ||
| private readonly Dictionary<IEdmProperty, ModelBoundQuerySettings> _propertiesQuerySettings = new Dictionary<IEdmProperty, ModelBoundQuerySettings>(); | ||
| private readonly Dictionary<IEdmProperty, PropertyConfiguration> _propertyConfigurations = new Dictionary<IEdmProperty, PropertyConfiguration>(); |
There was a problem hiding this comment.
_propertyConfigurations [](start = 73, length = 23)
We can't use the "_properties" field? #Resolved
There was a problem hiding this comment.
We need PropertyConfiguration. _properties does not have any configuration.
In reply to: 358418941 [](ancestors = 358418941)
| if (types.Count == 0) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
switch line 689 and ling 690 #Resolved
| } | ||
| else | ||
| { | ||
| throw Error.InvalidOperation("Cannot find EdmType corresponding to {0}", type); |
There was a problem hiding this comment.
Cannot find EdmType corresponding to {0} [](start = 50, length = 40)
use the SRSources for the error message? #Resolved
| return; | ||
| } | ||
|
|
||
| List<EdmStringConstant> collectionConstants = new List<EdmStringConstant>(); |
There was a problem hiding this comment.
List collectionConstants = new List(); [](start = 12, length = 76)
shall we make sure no duplicated existed? #Resolved
There was a problem hiding this comment.
We do that through the ISet. At this point in time, there should be no duplicates.
In reply to: 358420621 [](ancestors = 358420621)
| using System.Diagnostics.CodeAnalysis; | ||
| using System.Diagnostics.Contracts; | ||
| using System.Linq; | ||
| using System.Reflection; |
| _bindingParameter = new BindingParameterConfiguration(name, bindingParameterType); | ||
| } | ||
|
|
||
| internal virtual void AddDerivedTypeConstraintToReturnTypeImplementation(Type t) |
There was a problem hiding this comment.
t [](start = 85, length = 2)
rename the parameter name #Resolved
| _bindingParameter = new BindingParameterConfiguration(name, bindingParameterType); | ||
| } | ||
|
|
||
| internal virtual void AddDerivedTypeConstraintToReturnTypeImplementation(Type t) |
There was a problem hiding this comment.
virtual [](start = 17, length = 7)
I don't think it's virtual method, right? #Resolved
| { | ||
| if (ReturnType.ClrType == null) | ||
| { | ||
| throw Error.InvalidOperation("No CLR Type specified for this configuration."); |
There was a problem hiding this comment.
No CLR Type specified for this configuration. [](start = 46, length = 45)
centralize the error messages #Resolved
|
|
||
| internal virtual void AddDerivedTypeConstraintToReturnTypeImplementation(Type t) | ||
| { | ||
| if (ReturnType.ClrType == null) |
There was a problem hiding this comment.
ReturnType [](start = 16, length = 10)
ReturnType maybe "null", right? #Resolved
| _bindingParameter = new BindingParameterConfiguration(name, bindingParameterType); | ||
| } | ||
|
|
||
| internal virtual void AddDerivedTypeConstraintToReturnTypeImplementation(Type t) |
There was a problem hiding this comment.
Type t [](start = 81, length = 6)
t maybe null #Resolved
|
|
||
| if (TypeConfiguration.ClrType == null) | ||
| { | ||
| throw Error.InvalidOperation("No CLR Type specified for this configuration."); |
There was a problem hiding this comment.
No CLR Type specified for this configuration. [](start = 46, length = 45)
centralize the error messages #Resolved
|
|
||
| DerivedTypeConstraints.Add(t); | ||
| return this; | ||
| } |
There was a problem hiding this comment.
It seems similar method repeated a lot, maybe consider to create a helper method #Resolved
|
|
||
| [Fact] | ||
|
|
||
| public async Task VerifyMetaDataIsGeneratedCorrectly() |
There was a problem hiding this comment.
VerifyMetaDataIsGeneratedCorrectly() [](start = 26, length = 36)
Add test cases in "MetadataControllerTest.cs" to verify the annotation showing in the xml #Resolved
There was a problem hiding this comment.
need E2E test cases to verify:
- Uri parsing
- Serialization
In reply to: 358425347 [](ancestors = 358425347)
| } | ||
|
|
||
| var collectionExpression = new EdmCollectionExpression(collectionConstants); | ||
| EdmVocabularyAnnotation annotation = new EdmVocabularyAnnotation(target, term, collectionExpression); |
There was a problem hiding this comment.
EdmVocabularyAnnotation annotation = new EdmVocabularyAnnotation(target, term, collectionExpression); [](start = 12, length = 101)
Discussion: Maybe need configuration to set the annotation location (Inline or outofLine) #Resolved
| public QueryConfiguration QueryConfiguration { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Adds TSubClass to the list of derived type constraints. |
There was a problem hiding this comment.
TSubClass [](start = 17, length = 9)
TSubClass [](start = 17, length = 9)
resentence the comments #Resolved
| /// </summary> | ||
| internal PropertyConfiguration AddDerivedTypeConstraintImpl(Type t) | ||
| { | ||
| if (RelatedClrType == null) |
There was a problem hiding this comment.
RelatedClrType [](start = 16, length = 14)
why do we test this? It should be tested on the constructor, right? #Resolved
| { | ||
| if (RelatedClrType == null) | ||
| { | ||
| throw Error.InvalidOperation("No CLR Type specified for this configuration."); |
There was a problem hiding this comment.
No CLR Type specified for this configuration. [](start = 46, length = 45)
add the errors into resource manage #Resolved
There was a problem hiding this comment.
| /// <summary> | ||
| /// Adds TSubClass to the list of derived type constraints. | ||
| /// </summary> | ||
| internal PropertyConfiguration AddDerivedTypeConstraintImpl(Type t) |
There was a problem hiding this comment.
t [](start = 73, length = 1)
rename the parameter name #Resolved
| /// </summary> | ||
| public SingletonConfiguration AddDerivedTypeConstraint<TSubClass>() where TSubClass : class | ||
| { | ||
| this.AddDerivedTypeConstraintImpl(typeof(TSubClass)); |
There was a problem hiding this comment.
this. [](start = 12, length = 5)
don't need "this." #Resolved
| /// </summary> | ||
| internal void AddDerivedTypeConstraintImpl(Type t) | ||
| { | ||
| if (ClrType == null) |
There was a problem hiding this comment.
if (ClrType == null) [](start = 12, length = 20)
we don't need to test this, right?
It's verify at the constuctor #Resolved
| /// </summary> | ||
| public NavigationPropertyConfiguration AddDerivedTypeConstraint<TSubClass>() where TSubClass : class | ||
| { | ||
| this.AddDerivedTypeConstraintImpl(typeof(TSubClass)); |
| /// <summary> | ||
| /// Adds TSubClass to the list of derived type constraints. | ||
| /// </summary> | ||
| public EnumPropertyConfiguration AddDerivedTypeConstraint<TSubClass>() where TSubClass : class |
There was a problem hiding this comment.
EnumPropertyConfiguration [](start = 15, length = 25)
do we have the enum property derived type scenario? #Resolved
| /// </summary> | ||
| public EntitySetConfiguration AddDerivedTypeConstraint<TSubClass>() where TSubClass : class | ||
| { | ||
| this.AddDerivedTypeConstraintImpl(typeof(TSubClass)); |
There was a problem hiding this comment.
this. [](start = 12, length = 5)
remove this. #Resolved
| /// </summary> | ||
| public ComplexPropertyConfiguration AddDerivedTypeConstraint<TSubClass>() where TSubClass : class | ||
| { | ||
| this.AddDerivedTypeConstraintImpl(typeof(TSubClass)); |
There was a problem hiding this comment.
this. [](start = 12, length = 5)
remove this. #Resolved
| public DerivedTypeConstraintSet(Type baseType) | ||
| : base() | ||
| { | ||
| ClrBaseType = baseType; |
There was a problem hiding this comment.
ClrBaseType = baseType; [](start = 12, length = 23)
no config for the Location? #Resolved
| /// <summary> | ||
| /// Initializes a DerivedTypeConstraintSet instance without setting a base type. | ||
| /// </summary> | ||
| public DerivedTypeConstraintSet() |
There was a problem hiding this comment.
DerivedTypeConstraintSet [](start = 15, length = 24)
maybe call other ctor with arguments #Resolved
| public DerivedTypeConstraintSet(Type baseType) | ||
| : base() | ||
| { | ||
| ClrBaseType = baseType; |
There was a problem hiding this comment.
ClrBaseType = baseType; [](start = 12, length = 23)
check the baseType nullable?
| { | ||
| ValidateAndAddSingleConstraint(ClrBaseType); | ||
| return; | ||
| } |
There was a problem hiding this comment.
if the input is empty, add the ClrBaseType ? Why don't we have this strange logic?
There was a problem hiding this comment.
I had two options; One was to throw an error and the other was to consider that as an indication to allow only base types. Consider that this is called from HasDerivedTypeConstraints(). What should be the behavior?
In reply to: 394506704 [](ancestors = 394506704)
| if (ClrBaseType == null) | ||
| { | ||
| throw Error.InvalidOperation(SRResources.NoClrTypeSpecified); | ||
| } |
There was a problem hiding this comment.
A DerivedTypeConstraintSet should have a ClrBaseType configuired already, why should we check this? #ByDesign
There was a problem hiding this comment.
An operation can be created without a ClrBaseType. Think about actions with no return type. #ByDesign
| throw Error.InvalidOperation(SRResources.ConstraintAlreadyExists, typeToAdd.Name); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I'd like to copy/paste my assumption:
I don't want to use my name, but, for the "Add" logic, we can make it simple as blow
/// <summary>
///
/// </summary>
public class DerivedTypeConstraintConfiguration
{
private ISet<Type> _derivedTypes;
/// <summary>
///
/// </summary>
/// <param name="baseType"></param>
public DerivedTypeConstraintConfiguration(Type baseType)
{
ClrBaseType = baseType;
_derivedTypes = new HashSet<Type>();
Location = EdmVocabularyAnnotationSerializationLocation.OutOfLine;
}
/// <summary>
///
/// </summary>
public ISet<Type> DerivedTypes { get { return _derivedTypes; } }
/// <summary>
///
/// </summary>
public Type ClrBaseType { get; private set; }
/// <summary>
///
/// </summary>
/// <typeparam name="TDerived"></typeparam>
public void Add<TDerived>()
{
Add(typeof(TDerived));
}
/// <summary>
///
/// </summary>
public EdmVocabularyAnnotationSerializationLocation Location { get; set; }
/// <summary>
///
/// </summary>
/// <param name="derivedType"></param>
public void Add(Type derivedType)
{
if (derivedType == ClrBaseType)
{
return;
}
if (_derivedTypes.Contains(derivedType))
{
return;
}
if (!ClrBaseType.IsAssignableFrom(derivedType))
{
throw new Exception("blabla");
}
_derivedTypes.Add(derivedType);
}
}
There was a problem hiding this comment.
The main difference that I see is that you don't support adding enumerables. I think it makes the implementation a lot cleaner if you can add enumerables to the set.
In reply to: 394509903 [](ancestors = 394509903)
| /// <summary> | ||
| /// Set of derived type constraints. | ||
| /// </summary> | ||
| public class DerivedTypeConstraintSet : HashSet<Type> |
There was a problem hiding this comment.
: HashSet [](start = 42, length = 15)
I am not fan for either "derived" or "Composition"
However, i'd like to mention:
- Derived "HasSet" , we can't change later for this type owing breaking changes.
- Composition and make it private means we can change it any time without any breaking changes.
#Resolved
| _returnType = value; | ||
| if (value != null) | ||
| { | ||
| DerivedTypeConstraints.ClrBaseType = value.ClrType; |
There was a problem hiding this comment.
DerivedTypeConstraints.ClrBaseType = value.ClrType; [](start = 20, length = 51)
That's weird and change/add the configuration even no derived type configuration happens in customer scenarios
There was a problem hiding this comment.
The option was to lazy initialize and save extra allocations or initialize with the constructor. Model builder not being the most performance critical piece, I decided to initialize with the constructor. Since the return type does not have to be specified in the operation configuration initialization,I need to update it for the derivedTypeConstraint whenever it gets set. Will change to lazy initialize for operations.
In reply to: 394512858 [](ancestors = 394512858)
| /// </summary> | ||
| /// <param name="subtypes">The subtypes for which the constraint needs to be added.</param> | ||
| /// <returns>Updated configuration object.</returns> | ||
| public ActionConfiguration HasDerivedTypeConstraints(params Type[] subtypes) |
There was a problem hiding this comment.
HasDerivedTypeConstraints [](start = 35, length = 25)
I can't understand which derived constraint happens if customer call this method.
It's for the Binding type constraint, or return type constraint.
Be caution, return type and binding type could be the same type.
There was a problem hiding this comment.
It is for the return type, I changed the name to be more explicit. I don't think we should give customers two ways of doing one thing. The can always do function.BindingParameter.HasDerivedTypeConstraint().
| /// <summary> | ||
| /// Represents an <see cref="Attribute"/> that can be placed on a property or placed on a class to specify the derived type constraints. | ||
| /// </summary> | ||
| [AttributeUsage(AttributeTargets.Property)] |
There was a problem hiding this comment.
[AttributeUsage(AttributeTargets.Property)] [](start = 4, length = 43)
It's only for the property, we can't put it on the class?
My assumption is like this:
[DerivedTypeConstraint(typeof(Human)]
[DerivedTypeConstraint(typeof(Dog)]
....
public class Creature
{
....
}
It support to multiple config, each for one. (I am also ok to use param[])
There was a problem hiding this comment.
If you start putting it on Classes, user will expect to put constraints on entity types which we don't want to do. And Params seems cleaner.
In reply to: 394517334 [](ancestors = 394517334)
There was a problem hiding this comment.
SO, we don't have the C# attribute for entity set?
can we do like this:
And a string argument into the Constructor of "DerivedTypeConstraintAttribute", use it to set the entity set or singleton or navigation property built from this type?
[DerivedTypeConstraint(typeof(Human), "Creatures"]
[DerivedTypeConstraint(typeof(Dog)]
....
public class Creature
{
....
}
In reply to: 399119699 [](ancestors = 399119699,394517334)
| public DerivedTypeConstraintAttribute() | ||
| { | ||
| DerivedTypeConstraints = new HashSet<Type>(); | ||
| } |
There was a problem hiding this comment.
Required by the compiler. Need to have at least one constructor with CLS compliant parameters for these classes.
In reply to: 394517978 [](ancestors = 394517978)
| { | ||
| throw Error.InvalidOperation(SRResources.NoClrTypeSpecified); | ||
| } | ||
| else if (!ClrBaseType.IsAssignableFrom(typeToAdd) && typeToAdd != ClrBaseType) |
There was a problem hiding this comment.
&& typeToAdd != ClrBaseType [](start = 62, length = 27)
I don't think you need this second check; if typeToAdd == ClrBaseType, then ClrBaseType.IssAssignableFrom(typeToAdd) would be true. #Resolved
| /// <summary> | ||
| /// Initializes a DerivedTypeConstraintConfiguration instance without setting a base type. | ||
| /// </summary> | ||
| internal DerivedTypeConstraintConfiguration() |
There was a problem hiding this comment.
internal [](start = 8, length = 8)
why it's internal?
There was a problem hiding this comment.
We don't want people to create a DerivedTypeConstraintConfiguration without a base type.
In reply to: 401810329 [](ancestors = 401810329)
| ReturnTypeConstraints = new DerivedTypeConstraintConfiguration(ReturnType.ClrType); | ||
| } | ||
|
|
||
| ReturnTypeConstraints.AddConstraint<TDerivedType>(); |
There was a problem hiding this comment.
why don't you move all these codes into base class OperationConfiguration?
There was a problem hiding this comment.
To avoid the cast while using the fluent APIs. I can have an internal implementation on operation configuration but it is only 3 lines of code.
In reply to: 401813407 [](ancestors = 401813407)
| ReturnTypeConstraints = new DerivedTypeConstraintConfiguration(ReturnType.ClrType); | ||
| } | ||
|
|
||
| ReturnTypeConstraints.AddConstraints(subtypes); |
There was a problem hiding this comment.
why don't you move all these codes into base class OperationConfiguration? #Resolved
There was a problem hiding this comment.
To avoid the cast while using the fluent APIs. I can have an internal implementation on operation configuration but it is only 3 lines of code.
In reply to: 401813541 [](ancestors = 401813541)
| } | ||
|
|
||
| /// <summary> | ||
| /// Initializes a DerivedTypeConstraintConfiguration instance without setting a base type. |
There was a problem hiding this comment.
without setting a base type [](start = 70, length = 27)
copy/paste - this overload doe sspecify a base type
address pr comments update method
6e44b49 to
9d4efb5
Compare
|
I think we'll ultimately want to put the validation as a rule in EdmModel.Validate -- there may be scenarios where we (or the user) want to turn off this validation. But okay with it here for now, since it's internal, unless we have to make additional changes for other reasons. |
| } | ||
| } | ||
|
|
||
| internal ISet<Type> ConstraintSet { get; } |
There was a problem hiding this comment.
internal ISet ConstraintSet { get; } [](start = 8, length = 42)
cannot be private?
* Everything except properties. Fix tests Address PR comments Remove Inline change due to a bug * Address PR comments and take sam's input parity between actions and functions self review Update tests fix final set of issues * Address PR comments address pr comments update method
* Everything except properties. Fix tests Address PR comments Remove Inline change due to a bug * Address PR comments and take sam's input parity between actions and functions self review Update tests fix final set of issues * Address PR comments address pr comments update method
Issues
This pull request enables support for derived type constraint annotation.
Description
Non-convention model builder now has APIs and thus support for adding out of line annotations for derived type constraint.
Checklist (Uncheck if it is not completed)