NHV-119 - Handle multiple attributes #17

AlexanderKot · 2018-04-16T16:31:11Z

NHV-119 - Handle multiple attributes, fix & tests.
For many attributes, enable AllowMultiple.
If several attributes of the same type are defined on a property, their constraint injection in NHibernate mapping is disabled.
If several attributes of the same type are defined (as attribute or using fluent configuration) with overlapping tags (or without tags at all) on a property, the last defined attribute override all previous ones.

For many attributes AllowMultiple=true; If several attributes with different tags defined on property no Nhib mappings changes; If several attribute with same tag defined for property as attribute or using fluent configuration - last defined attribute override all previous with same tag;

fredericDelaporte · 2018-04-19T10:37:34Z

I have reworded your description for having plain sentences, but for "no NHib mappings changes", I am not making sens of it. The first part of the sentence looks to me as the thing that is asked by NHV-119, so "no changes" looks a bit contradictory. What is a change that would change nothing?

Please also prioritize the most meaningful information in the commit: the first line should directly tell what the change is about. "Fix & tests" is quite unhelpful, we are left to lookup the ticket. "Handle multiple attributes" (what I have edited in the PR description and title) tells more about the subject. You can put the "Fix & tests" at the very end of the commit message just to let know if you wish, but this information could also be dropped. That looks to me as the default for a PR/commit.

fredericDelaporte · 2018-04-19T10:55:17Z

src/NHibernate.Validator.Tests/Utils/AttributeUtilsFixture.cs

 			Assert.AreEqual(true, (AttributeUtils.AttributeAllowsMultiple(patternAttribute)));
 		}

 		[Test]
 		public void AttributeCannotBeMultiplied()
 		{
-			LengthAttribute lenghtAttribute = new LengthAttribute();
+			var lenghtAttribute = new IBANAttribute();


The variable name should be changed too.

fredericDelaporte · 2018-04-19T11:00:10Z

src/NHibernate.Validator/Constraints/RangeAttribute.cs

+			if (AttributeUtils.AttributeUsedMultipleTimesOnProperty(property, GetType()))
+				return;
+
+


Double blank line.

fredericDelaporte · 2018-04-19T11:01:31Z

src/NHibernate.Validator/Engine/ClassValidator.cs

@@ -207,6 +207,9 @@ private void InitValidator(System.Type clazz, IDictionary<System.Type, IClassVal
 					ValidateClassAtribute(classAttribute);
 				}

+
+				var membersToValidateFor1Memeber = new List<Member>();


Typo in variable name. And well, in fact those Member are more validator definition than members. A more meaningful name for that variable could then be memberValidators.

fredericDelaporte · 2018-04-19T12:07:47Z

src/NHibernate.Validator/Engine/ClassValidator.cs

+				membersToValidateFor1Memeber.RemoveAll(x => x.ValidatorDef.Validator.GetType() == newAttr.GetType()
+				                                            && ((x.ValidatorDef.Tags == null && newAttr.TagCollection == null)
+				                                                || x.ValidatorDef.Tags.Intersect(newAttr.TagCollection).Any()
+				                                            ));


This need renaming, rewriting and reformatting, for maintainability and correctness. Please use semantic naming (what is it meant to do, rather than what it technically does).

By example:

private void RemoveOverridenValidators(System.Type clazz, List<Member> memberValidators, ITagableRule newValidator) { var newValidatorType = newValidator.GetType(); var newValidatorIsUntagged = newValidator.TagCollection.Count == 0; var removedCount = memberValidators .RemoveAll( v => v.ValidatorDef.Validator.GetType() == newValidatorType && ( (v.ValidatorDef.Tags == null || v.ValidatorDef.Tags.Count == 0) && newValidatorIsUntagged || v.ValidatorDef.Tags != null && !newValidatorIsUntagged && v.ValidatorDef.Tags.Intersect(newValidator.TagCollection).Any())); if (removedCount > 0) { #if NETFX log.Debug( "Validators of the same type having some tags in common (or both without tags) found on a " + "member, keeping only the last defined: class " + clazz.FullName + ", member " + v.Getter.Name + ", last validator " + newValidator); #else Log.Debug( "Validators of the same type having some tags in common (or both without tags) found on a " + "member, keeping only the last defined: class {0}, member: {1}, last validator {2}", clazz.FullName, v.Getter.Name, newValidator); #endif } }

I have removed the null check on TagCollection since it is never checked everywhere else and that current implementations never yields null. I also have added a log for helping diagnosing rule overrides.

But shouldn't it check AllowsMultipleWithIntersectingTags before removing?

fredericDelaporte · 2018-04-19T12:25:03Z

src/NHibernate.Validator/Constraints/RangeAttribute.cs

+			if (AttributeUtils.AttributeUsedMultipleTimesOnProperty(property, GetType()))
+				return;
+
+
 			IEnumerator ie = property.ColumnIterator.GetEnumerator();
 			ie.MoveNext();
 			var col = (Column)ie.Current;


GetEnumerator yields an IEnumerator<ISelectable> which is disposable, and not disposed of... This kind of old code should be modernized when met, for using extension methods like First` instead.

fredericDelaporte · 2018-04-19T12:48:29Z

src/NHibernate.Validator/Util/AllowsMultipleWithIntersectingTagsAttribute.cs

+namespace NHibernate.Validator.Util
+{
+	/// <summary>
+	/// Specify if constraint attribute can be used multiple times with intersecting tags. (No tags, or same tags)


Parenthesis is misleading I think. It should be `(No tags, or some tags in common.)

fredericDelaporte · 2018-04-19T12:50:16Z

src/NHibernate.Validator/Util/AttributeUtils.cs

+		public static bool AttributeAllowsMultipleWithIntersectingTags(Attribute attribute)
+		{
+			var attr = Attribute.GetCustomAttribute(attribute.GetType(), typeof(AllowsMultipleWithIntersectingTagsAttribute));
+			return AttributeAllowsMultiple(attribute) && attr != null;


Revert condition order: when using short-circuiting boolean operators, the less costly operation should be done first.

fredericDelaporte · 2018-04-19T12:52:44Z

src/NHibernate.Validator/Util/AttributeUtils.cs

+		}
+
+		/// <summary>
+		/// Return true if same attribute applied to property or field more then 1 time. It can happens when attribute used with different tags


Many typo and syntax troubles.
Return true if the same attribute is applied to the property or field more than one time. It can happen when the attribute is used with different tags.

fredericDelaporte · 2018-04-19T12:57:58Z

src/NHibernate.Validator/Util/AttributeUtils.cs

+		public static bool AttributeUsedMultipleTimesOnProperty(Property property, System.Type attributeType)
+		{
+			//Next happens for component properties. Not possible to access component class metadata back from Property in NHib 5.0
+			//TODO: How to fix this?


See comment on Apply.

fredericDelaporte · 2018-04-19T13:02:22Z

src/NHibernate.Validator/Constraints/DecimalMaxAttribute.cs

@@ -98,6 +99,9 @@ public bool IsValid(object value, IConstraintValidatorContext validatorContext)

 		public void Apply(Property property)
 		{
+			if (AttributeUtils.AttributeUsedMultipleTimesOnProperty(property, GetType()))
+				return;


This seems to be done for all IPropertyConstraint with AllowMultiple. Then better move that check into ClassValidator, for not calling this method at all when constraints should not be generated (when the attribute allows multiple and is defined multiple times). By the way, this should allow to fix the component case, since ClassValidator has access to more data about the property.

fredericDelaporte · 2018-04-19T13:13:21Z

Now after reviewing I can understand a bit more "If several attributes with different tags defined on property no Nhib mappings changes", but it appears to be wrong. Tags are not checked anywhere for disabling the constraint "enrichment" that some attributes may do.

I am changing that to: If several attributes of the same type are defined on a property, their constraint injection in NHibernate mapping is disabled.

AlexanderKot · 2018-04-19T16:22:03Z

Hello
Thank you for review. I will improve all this.
About "… the last defined attribute override all previous ones. "
May be this need to be changed to opposite one, to be consistent with fluent definitions?
In OpenClassMapping.cs first definition is used and all later ignored with writing message to log, if attribute is not allowed to be used multiple times.

fredericDelaporte · 2018-04-19T17:34:33Z

May be this need to be changed to opposite one, to be consistent with fluent definitions?
In OpenClassMapping.cs first definition is used and all later ignored with writing message to log, if attribute is not allowed to be used multiple times.

Yes then, invert it.

fredericDelaporte · 2018-04-19T17:55:10Z

src/NHibernate.Validator/Mappings/OpenClassMapping.cs

 			if (found == null || AttributeUtils.AttributeAllowsMultiple(attribute))
 			{
+				constraints.Add(attribute); 
+				//Not possible to stop adding constraints here based on intersecting tags, because Tags not yet assigned when this code is executed


Can you clarify what happens in that case? Is the fluent configuration checked and "cleaned up" later anyway? Or does this cause the overriding to not occur in this case, letting all attributes applied even if contradictory?

AlexanderKot added 2 commits April 16, 2018 19:07

Lost comment added

de41f44

fredericDelaporte changed the title ~~NHV-119 fix & tests;~~ NHV-119 - Handle multiple attributes Apr 19, 2018

fredericDelaporte reviewed Apr 19, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NHV-119 - Handle multiple attributes #17

NHV-119 - Handle multiple attributes #17

AlexanderKot commented Apr 16, 2018 •

edited by fredericDelaporte

Loading

fredericDelaporte commented Apr 19, 2018

fredericDelaporte Apr 19, 2018

fredericDelaporte Apr 19, 2018

fredericDelaporte Apr 19, 2018

fredericDelaporte Apr 19, 2018

fredericDelaporte Apr 19, 2018

fredericDelaporte Apr 19, 2018

fredericDelaporte Apr 19, 2018

fredericDelaporte Apr 19, 2018

fredericDelaporte Apr 19, 2018

fredericDelaporte Apr 19, 2018

fredericDelaporte commented Apr 19, 2018

AlexanderKot commented Apr 19, 2018

fredericDelaporte commented Apr 19, 2018

fredericDelaporte Apr 19, 2018

		if (AttributeUtils.AttributeUsedMultipleTimesOnProperty(property, GetType()))
		return;

NHV-119 - Handle multiple attributes #17

Are you sure you want to change the base?

NHV-119 - Handle multiple attributes #17

Conversation

AlexanderKot commented Apr 16, 2018 • edited by fredericDelaporte Loading

fredericDelaporte commented Apr 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredericDelaporte commented Apr 19, 2018

AlexanderKot commented Apr 19, 2018

fredericDelaporte commented Apr 19, 2018

Choose a reason for hiding this comment

AlexanderKot commented Apr 16, 2018 •

edited by fredericDelaporte

Loading