The multiool, core sarif, and validation test binaries now all pass

[michaelcfanning]

No description provided.

[michaelcfanning]

IList [](start = 23, length = 5)

Needs to be virtual due to converting skimmer base to derive from Rule (as part of ongoing effort to extirpate IRule) #Pending

[ghost]

That makes sense. But I'm surprised this the code generator is not marking this property as virtual. Every other property is virtual. What's special about this one? (he asked rhetorically).

---

In reply to: 248136339 [](ancestors = 248136339)

[michaelcfanning]

It could very well be for all I know, Rule.cs is getting picked up from NotYetAutogenerated for some other reason (or unnecessarily). I will take a look and get you a good answer, won't block here.

---

In reply to: 248368566 [](ancestors = 248368566,248136339)

[michaelcfanning]

code gen isn't emitting these as virtual. we may have broken the connection between IRule/Rule that would result in this (as part of our partial extraction process). in which case, ok, expected. or maybe there's a jschema issue. i won't file a bug, because we will remember to go examine and attempt to review the notyetgenerated content as part of fixing other bugs, and we'll fix the blame then where it needs to be.

---

In reply to: 248377693 [](ancestors = 248377693,248368566,248136339)

[michaelcfanning]

_fileLocationKeyToIndexMap [](start = 16, length = 26)

I removed necessary functionality earlier. The prerelease transformer depends on the old prerelease v2 file location key to perform some fixups. The result matching update will require index updates that match current v2 file locations. We'll need to update this class further then (or provide better separation of purpose for v2 update vs. current v2 matching. #Resolved

[michaelcfanning]

fileLocationKeyToIndexMap [](start = 105, length = 25)

your bug. nice catch, thanks. #Closed

[michaelcfanning]

forceUpdate [](start = 20, length = 11)

real issue here is that we weren't honoring the formatting param. added the forceUpdate/modifiedLog checks just to minimize code execution for certain cases #Closed

[michaelcfanning]

Rule [](start = 51, length = 4)

Rule derives from PropertyBag. The big problem with not using interfaces, of course, is that you need a single, sensible base class chain. We barely are able to accomplish this to eliminate IRule (thanks to the SARIF design change to make everything have a property bag). #ByDesign

[michaelcfanning]

Verify [](start = 21, length = 6)

I couldn't debug this test easily. So made a slight improvement where the relevant locations of unexpected conditions were isolated/more obvious. #ByDesign

[michaelcfanning]

files [](start = 7, length = 5)

for this file, had to spot-inject the v2 pattern, otherwise this non-transformed file doesn't deserialize. what a frankenfile! or i guess it's actually frankenstein's file
#Resolved

[ghost]

Yes, that's similar to the issue I mentioned in my comment on the other Invalid.sarif file that you changed. The "expected" file has a couple of instances of "message": "blah" instead of "message": { "text": "blah" }. And the PreReleaseCompatibilityTransformer is skipping the core transformations because it's called with forceUpdate: false, so those bad messages are left alone, so the file won't deserialize as v2.

---

In reply to: 248137193 [](ancestors = 248137193)

[michaelcfanning]

fileLocation [](start = 11, length = 12)

I had to update this entire file to SARIF v2 because otherwise the prerelease transformer would manufacture a new (correct) uri for the file. Rather than try to special-case the helper just to provoke testing of invalid content, just updated the file. #ByDesign

[ghost]

If I understand what's going on, the problem centers on the call to PrereleaseCompatibilityTransformer.UpdateToCurrentVersion in FileDiffingTests.RunTest. This call was fixing the invalid uri property in this fileLocation object. By updating this file to the most current pre-release version, you avoided the file update altogether -- and that works because RunTest calls PrereleaseCompatibilityTransformer.UpdateToCurrentVersion with forceUpdate: false -- right?

---

In reply to: 248137397 [](ancestors = 248137397)

[michaelcfanning]

Exactly right.

---

In reply to: 248374537 [](ancestors = 248374537,248137397)

[ghost]

In that case I'm going to have to revisit one of the fixes in my branch. One of the unit tests of SarifVersionOneToCurrentVisitor was failing because the pre-release transformer was not updating one of the test files -- and I fixed it in my branch by changing forceUpdate to true in PrereleaseCompatibilityTransformer.UpdateToCurrentVersion! (Remember I sent you a message last night saying that I'd fixed a "bug" in PrereleaseCompatibilityTransformer.UpdateToCurrentVersion :-))

So I'll change it back and change the test file instead.

---

In reply to: 248377880 [](ancestors = 248377880,248374537,248137397)

[michaelcfanning]

AbsoluteUri_ReversesRebasedURIs [](start = 20, length = 31)

made this thing slightly more debuggable. we now iterate over each log and block on discovering problem.

this test is just brain dead, though. it produces two kinds of uris only, for files table and results. the randomness is simply around the count of logs, runs, files, results. Dumb. let's just do a better job of writing this test later so that it operates against comprehensive data. this thought is why i didn't push a test pattern improvement elsewhere, just updated the failing test (which now passes). #ByDesign

[michaelcfanning]

    public void RebaseUriVisitor_VisitFileData_RebasesAllTheThings()

Horrible-to-maintain test champion. For the record, I authored this thing. Nerts. Trust me, I hand validated all the constant value changes below. We need a better test strategy here. It will be simple, actually, just need to produce the transformed sarif to disk, diff as usual. i'll do this soon. #Resolved

---

Refers to: src/Sarif.UnitTests/Visitors/RebaseUriVisitorTests.cs:178 in bd80b54. [](commit_id = bd80b54, deletion_comment = False)

[michaelcfanning]

@lgolding, the favor of your review is requested. #Closed

[ghost]

.OrderBy(loc => loc) [](start = 119, length = 20)

Since you're making a HashSet, there's no reason to order the list. Same thing on the next line. #Closed

[michaelcfanning]

Got it. FYI, i have updated this PR to merge files-array, will push an update in a moment. #Closed

[ghost]

unexpectedlyAbsentResultLocations [](start = 12, length = 33)

This whole thing can be done with a couple of LINQ calls. Also, a suggested variable rename for consistency:

IEnumerable<string> unexpectedNewResultLocations = actualResultLocations.Except(expectedResults.ResultLocationPointers);
unexpectedNewResultLocations.Count().Should().Be(0);

IEnumerable<string> unexpectedlyAbsentResultLocations = expectedResults.ResultLocationPointers.Except(actualResultLocations);
unexpectedlyAbsentResultLocations.Count().Should().Be(0);
``` #WontFix

[michaelcfanning]

I considered this but I don't like leaving O(N^2) comparisons hanging around. The code as authored is much less compact/elegant but is clear and performant.

---

In reply to: 248362142 [](ancestors = 248362142)

[ghost]

Would you at least do the variable rename?

---

In reply to: 248384220 [](ancestors = 248384220,248362142)

[ghost]

l [](start = 40, length = 1)

I get that by eliminating the LINQ implicit loop over logs you're making it a little easier to find the problem. But why did you reverse (actually, not quite reverse, because it's still && instead of ||) the sense of this condition?

[ghost]

RelativeOrAbsolute [](start = 62, length = 18)

Isn't it just plain relative? #ByDesign

[michaelcfanning]

Not according to .NET. :) which says, 'i can't make a determination'. leading slashes are problematic, in implementation in .NET, if not in the URI spec.

---

In reply to: 248385717 [](ancestors = 248385717)

[ghost]

• [](start = 100, length = 1)

While we're in here, "-0" => "0" #Closed

[ghost]

blah [](start = 55, length = 4)

Let's also assert that run.Files[0].FileLocation.UriBaseId.Should().Be("SRCROOT"). #Closed

[ghost]

Why did you add this line? #ByDesign

[michaelcfanning]

I corrected the file index, which was wrong. have we addressed the matter of how the file index is populated for a file location within the files table itself? i.e., that it can either accurately reflect the actual index of its container or that it can be -1 (which isn't valid, the default, meaning it isn't expressed). or is this sufficiently clear in the spec, you think?

---

In reply to: 248386981 [](ancestors = 248386981)

[ghost]

Sorry, I misread the diff, didn't realize this line was already there.

---

In reply to: 248387734 [](ancestors = 248387734,248386981)

[ghost]