Skip to content

Refactoring ValidatorBase#534

Merged
eddynaka merged 8 commits into
mainfrom
users/ednakamu/new-implementation
Aug 14, 2021
Merged

Refactoring ValidatorBase#534
eddynaka merged 8 commits into
mainfrom
users/ednakamu/new-implementation

Conversation

@eddynaka

Copy link
Copy Markdown
Collaborator

No description provided.

@eddynaka eddynaka changed the title Adding new implementation Refactoring ValidatorBase Aug 13, 2021
@michaelcfanning

michaelcfanning commented Aug 14, 2021

Copy link
Copy Markdown
Member
    public static bool TryGetNonEmptyValue<TKey>(this Dictionary<TKey, FlexMatch> dictionary, TKey key, out FlexMatch value)

Are you sure you require these helpers now? Trying removing them. i.e., any Dictionary instance should resolve to the IDictionary helpers below, I believe.


In reply to: 898930832


In reply to: 898930832


Refers to: Src/Plugins/Security/Utilities/DictionaryExtensions.cs:12 in 3b14c3c. [](commit_id = 3b14c3c, deletion_comment = False)


var sb = new StringBuilder();

var httpAuhorizationRequestHeaderValidator = new HttpAuthorizationRequestHeaderValidator();

@michaelcfanning michaelcfanning Aug 14, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

httpAuhorizationRequestHeaderValidator

This is misspelled now.

S/be httpAuthorizationRequestionHeaderValidator #Closed

"ValidationFingerprintHash/v1": "a82a08b8f8cd5a7eabf30386974158e85fa1f0982b13d273aa94373c2638eb42",
"AssetFingerprint/v2": "{\"host\":\"example.com\",\"id\":\"lastname\"}",
"ValidationFingerprint/v2": "{\"host\":\"example.com\",\"id\":\"lastname\"}"
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

Why has this stuff gone away?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we weren't using ValidatorBase, so duplicated results would appear.
With my current change, we will ask for the static or dynamic validator class, removing duplications

context: TestHelper.ContextSetup());

ValidateResult(string.Empty, result, runCount: 1, resultCount: 2, FailureLevel.Note, ignoreRegionContent: true);
ValidateResult(string.Empty, result, runCount: 1, resultCount: 1, FailureLevel.Note, ignoreRegionContent: true);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ValidateResult

Why did this test result change?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this change, AdoPat validator didn't implemented the ValidatorBase generating duplicated results.
With my current change, we will remove duplicates


namespace Microsoft.CodeAnalysis.Sarif.PatternMatcher.Sdk
{
public abstract class StaticValidatorBase

@michaelcfanning michaelcfanning Aug 14, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StaticValidatorBase

Nice rename! Good choice. #Closed

groups.Add("scanTargetFullPath", new FlexMatch() { Value = "GitHitPatTest" });

IEnumerable<ValidationResult> validationResults = GitHubPatValidator.IsValidStatic(groups);
var gitHubPatValidator = new GitHubPatValidator();

@michaelcfanning michaelcfanning Aug 14, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var

in other rules, you don't introduce a vertical line after instantiating the rule. :) Style nit! #Closed

"locations": [
{
"physicalLocation": {
"artifactLocation": {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

t

why did this go away? In general, it's good to explicitly account for results that disappear.

We have either introduced new false negatives or you have potentially dropped false positives, right?

Where's the description, if so, in ReleaseHistory.md?

@eddynaka eddynaka Aug 14, 2021

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we weren't using ValidatorBase, so duplicated results would appear.
With my current change, we will ask for the static or dynamic validator class, removing duplications

@michaelcfanning michaelcfanning left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@eddynaka eddynaka merged commit 686f9b3 into main Aug 14, 2021
@eddynaka eddynaka deleted the users/ednakamu/new-implementation branch August 14, 2021 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants