Bug in 3.5 BusinessBase IsValid()?

Bug in 3.5 BusinessBase IsValid()?

Old forum URL: forums.lhotka.net/forums/t/4478.aspx


skagen00 posted on Friday, March 07, 2008

public virtual bool IsValid

{

get { return ValidationRules.IsValid && (_fieldManager != null || FieldManager.IsValid()); }

}

The simplified object model I'm dealing with is an individual which contains a collection of individual names.

FieldManager.IsValid is false, but it doesn't matter because _fieldManager is not null. So when an individual name is invalid, individual is still returning that it's valid.

Certainly it only looks like _fieldManager is assigned if FieldManager is ever first referenced, so I assume this should be == null?

Chris

 

 

 

skagen00 replied on Friday, March 07, 2008

Please note that IsDirty has this logic as well.

skagen00 replied on Monday, March 10, 2008

*Bump*

I just wanted to make sure that Rocky notices this as I saw him swipe by with a few posts yesterday. I checked the SVN in advance but it's still unchanged -- just want to make sure this doesn't get lost in case it's a bug as a suspect it is.

 

RockfordLhotka replied on Wednesday, March 12, 2008

Yes, I think you are right - there's a bug. Here's a truth table that shows a problem (if I did it right - been a long time...):

T & (T | T) = T
T & (F | T) = T (but would blow up)
T & (T | F) = T (WRONG)
F & (T | T) = F
F & (F | T) = F
F & (T | F) = F

I think this is what it should be:

T & (T & T) = T
T & (F & T) = F
T & (T & F) = F
F & (T & T) = F
F & (F & T) = F
F & (T & F) = F

So if I am doinig this right, the problem is the OR should be an AND.

For IsDirty it would be:

T | (T & T) = T
T | (F & T) = T
T | (T & F) = T
F | (T & T) = T
F | (F & T) = F
F | (T & F) = F

Do you agree?

skagen00 replied on Wednesday, March 12, 2008

So it's currently:

ValidationRules.IsValid && (_fieldManager != null || FieldManager.IsValid());

You're proposing:

ValidationRules.IsValid && (_fieldManager != null && FieldManager.IsValid());

I initially suggested (and think is probably the right answer):

ValidationRules.IsValid && (_fieldManager == null || FieldManager.IsValid());

I initially suggested what I did because I wasn't entirely clear if _fieldManager got initialized when dealing with a class with no properties. I figured if it's the case that users who don't use managed properties don't get this initialized, _fieldManager == null would be a "valid" supporting condition.

It looked like there were many cases in the code where you were hitting FieldManager only if _fieldManager was not null - that is, it looked like perhaps you only wanted the property setting/getting to establish a FieldManager - kind of an on-demand thing.

There were a couple cases in code where however it was going right to FieldManager without checking if _fieldManager was null - effectively triggering an instance for _fieldManager - below are two:

Child_ListChanged

Child_PropertyChanged

I don't quite follow why you would change it to an "and" to be honest.... maybe I'm missing something?

Chris

 

ajj3085 replied on Wednesday, March 12, 2008

You're right, the problem is that if _fieldManger is null, _fieldManager != null returns false, so that entire expression is false, so the return will always be false when it should return just the evaluatin of ValidationRules.IsValid.

ajj3085 replied on Wednesday, March 12, 2008

RockfordLhotka:

Yes, I think you are right - there's a bug. Here's a truth table that shows a problem (if I did it right - been a long time...):

T & (T | T) = T
T & (F | T) = T (but would blow up)
T & (T | F) = T (WRONG)
F & (T | T) = F
F & (F | T) = F
F & (T | F) = F

I think this is what it should be:

T & (T & T) = T
T & (F & T) = F 

Shouldn't this be True.  There's no field manager to check, so it should = T... (F & T) can't exist, it will always blow up.


RockfordLhotka replied on Wednesday, March 12, 2008

Chris,

The problem with using OR is that it can blow up (and it is wrong if my table is correct). If the first part of OR returns false then the second part gets evaluated - which is exactly what we don't want!

Andy,

That bolded statement (T & (F | T)) is both False and safe. It is False because the inner expression returns False (F&T = F), and it is safe because the OR short-circuits and the second part is never evaluated because the first is False.

ajj3085 replied on Wednesday, March 12, 2008

You're right, it won't blow up, but shouldn't IsValid still return true if ValidationRules.IsValid is true and the fieldManger is null? 

( _fieldManager != null && fieldManager.IsValid ) always returns false if _fieldManager is null, so the entire expression will be false.   So the object will always say its not valid if _fieldManager is null if you use T & ( F & T ).  Or am I missing something?


RockfordLhotka replied on Wednesday, March 12, 2008

I see what you are saying, that's correct. Perhaps this can't be done in a single line. It should probably be this:

bool result = false;
if (ValidationRules.IsValid)
{
  if (fieldManager == null)
    result = true;
  else if (FieldManager.IsValid)
    result = true;
}
return result;

 

RockfordLhotka replied on Thursday, March 13, 2008

OK, so I think the original suggestion was right (change != to ==).

 

This isn’t that hard, but I’m so busy I’m only getting a minute here and there to think about it, which is really not fun…

 

Rocky

 

 

From: ajj3085 [mailto:cslanet@lhotka.net]
Sent: Wednesday, March 12, 2008 10:14 AM
To: rocky@lhotka.net
Subject: Re: [CSLA .NET] Bug in 3.5 BusinessBase IsValid()?

 

You're right, it won't blow up, but shouldn't IsValid still return true if ValidationRules.IsValid is true and the fieldManger is null? 

( _fieldManager != null && fieldManager.IsValid ) always returns false if _fieldManager is null, so the entire expression will be false.   So the object will always say its not valid if _fieldManager is null if you use T & ( F & T ).  Or am I missing something?




skagen00 replied on Thursday, March 13, 2008

Thank you Rocky, no hurries, just didn't want it to get lost.

RockfordLhotka replied on Saturday, March 15, 2008

I have checked in a fix for the IsValid issue. IsDirty was already correct, and oddly enough so was the VB code on both counts. But the fix is in, so things should be good now.

skagen00 replied on Wednesday, March 12, 2008

Actually if _fieldManager is not null (i.e. _fieldManager == null is false) don't you want to test IsValid for FieldManager?

I must be still missing something.

ajj3085 replied on Wednesday, March 12, 2008

NT

skagen00 replied on Wednesday, March 12, 2008

I like it but w/ _fieldManager :)

Chris

 

McManus replied on Wednesday, March 12, 2008

Andy,

This one looks OK, but I don't see the functional difference with Rocky's last solution.

Cheers,
Herman

ajj3085 replied on Wednesday, March 12, 2008

Well the problem with the last code snippet was that it didn't check _fieldManager.IsValid if _fieldManager wasn't null. 

McManus replied on Wednesday, March 12, 2008

If ValidationRules.IsValid is false, why bother checking _fieldManager.IsValid, since the end result will be false anyway!

 

skagen00 replied on Wednesday, March 12, 2008

**nevermind**

Copyright (c) Marimer LLC