-
Notifications
You must be signed in to change notification settings - Fork 77
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix and simplify DirectoryEntry validation #181
Fix and simplify DirectoryEntry validation #181
Conversation
ba57c3a
to
877ba99
Compare
@ironfede Looks to me like the root should also optionally throw if ValidationExceptionEnabled is set? i.e.
WDYT? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeremy-visionaid I think thats named used in methods are not 100% representative or their function. For example SiblingIsValid seems to imply it checks for "validity" but it really checks for "existance" of sibling AND do validation. So I think it should be renamed "HasSiblings" and internally throws for validity exception.
I think it should throw. |
Ensure both the root and left sibilings are considered when checking for duplicate SIDs. Also, remove redundant lookups.
Hmm, I might take it further and say that the method actually isn't even aware of sibilings - only the caller is, so "HasSiblings" is also arguably misleading. I think perhaps I could just rewrite it to avoid having a boolean return at all. |
877ba99
to
493f0d5
Compare
@ironfede I've rewriten to avoid having a bool method, and added another commit on top that fixes the case where an exception was not thrown if the root entry was invalid too. |
493f0d5
to
e7919e3
Compare
Ensure both the root and left sibilings are considered when checking for duplicate SIDs.
Also, remove redundant lookups.