Reduced files array build #1191

michaelcfanning · 2018-12-29T20:13:56Z

Trying to factor this large change into pieces. As a result, the overall build will not function. Tests will not pass. Once I get the overall build functioning, test updates will start to come online and new tests.

This first change restores compilation of the Sarif and Sarif.Converters binaries. It demonstrates a core pattern for working with file data objects: there is a helper on the Run object that will retrieve the requested file data object on the file location which serves as the effective key. This helper understands that it should only use the Uri and UriBaseId members to locate the associated file data object. This helper opportunistically updates the file location instance passed to it, populating its fileIndex property and also normalizing the Uri representation.

[btw - this approach of placing a helper on the run object may be useful for logical locations as well, will ponder this.]

While dropping in required updates to the converters code, I finally was compelled to DRY out all the shared results writing code into the base class. Having completed this, there is sufficient work to review.

Finally, as is inevitable, I ran into some existing rough edges that are either fixed or documented in bugs. We have a long-standing desire to zap IRule completely (it only exists due to being entangled with other interfaces backing the Driver extensibility model). IResultLogWriter has a bug, WriteToolNotifications s/be replaced by WriteInvocations (the invocations object now contains both tool and configuration notifications). The FortifyFpr converter attempts to persist tool notifications. I suspect successfully traveling this code path today would produce invalid SARIF, though (since the writer hangs the data off the run object.

@EasyRhinoMSFT, your attention particularly appreciated on the FPR converter work. Do we have a data file that contains tool notifications we can test?

* Add fileIndex property to file object (#1186) * Fix up tests * PR feedback to improve schema comment * Logical locations notes (#1185) (#1187) * Respond to a small # of PR comments related to recent logical locations change. * Fix visibility on helper * Fix up v1 transformation with keys that collide * Preserve decorated name data * Rebaseline test for decorated name propagation * Respond to PR feedback. Update tests to close test holes. * Rebaseline updated test * Test key collision in annotated code locations. * Update baseline

…oft/sarif-sdk into reduced-files-array-build

…an array.

michaelcfanning · 2018-12-29T20:15:34Z

src/Sarif/Sarif.csproj

+  </ItemGroup>
+
+  <ItemGroup>
+    <Compile Remove="Visitors\SarifCurrentToVersionOneVisitor.cs" />


SarifCurrentToVersionOneVisitor [](start = 30, length = 31)

Temporarily removed so I can isolate the v1/v2 transformation into a discrete change #ByDesign

Ok.

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

michaelcfanning · 2018-12-29T20:18:33Z

src/Sarif/SarifUtilities.cs

+
+        internal static bool UnitTesting = false;
+
+        internal static void DebugAssert(bool conditional)


DebugAssert [](start = 29, length = 11)

Random thing I'm throwing out there. Debug.Assert statements are great, except that they aren't handled well while unit testing (the tests resolve as inconclusive). This pattern would converter a failed assert into an exception, assuming tests set the UnitTesting conditional. Note that this would also allow us to effectively inject the assert into retail builds (Debug.Assert will compile away entirely of course). This is a nice value, unit testing can provoke some assertion coverage in release testing that would otherwise never get executed.

Want to start a general conversation on this, as we'll all want to be in sync on this approach assuming we proceed. #Closed

I like the idea of getting better unit test results. I suggest adding a message or reason string argument. You can use it as the message argument to InvalidOperartionException and as the second argument to Debug.Assert.

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

A fantastic idea.

In reply to: 244542519 [](ancestors = 244542519,244507788)

michaelcfanning · 2018-12-29T20:26:34Z

src/Sarif/Core/Run.cs

@@ -13,6 +14,109 @@ public partial class Run
        private static Invocation EmptyInvocation = new Invocation();
        private static LogicalLocation EmptyLogicalLocation = new LogicalLocation();

+        private IDictionary<FileLocation, int> _fileToIndexMap;
+
+        public int GetFileIndex(


GetFileIndex [](start = 19, length = 12)

Unit tests pending for this helper #Pending

ghost · 2018-12-29T23:45:32Z

src/Sarif.Converters/AndroidStudioConverter.cs

            var run = new Run()
            {
-                Tool = tool,
-                ColumnKind = ColumnKind.Utf16CodeUnits
+                Tool = new Tool { Name = ToolFormat.AndroidStudio },


Tool [](start = 27, length = 4)

Did you also mean to remove the local variable tool? #Closed

This is marked "Resolved" but the local variable tool on Line 67 is still there.

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

ghost · 2018-12-29T23:48:09Z

src/Sarif.Converters/ToolFileConverterBase.cs

+            var run = new Run()
+            {
+                Tool = new Tool { Name = toolName },
+                ColumnKind = ColumnKind.Utf16CodeUnits


ColumnKind.Utf16CodeUnits [](start = 29, length = 25)

Since this occurs several times, maybe there's a good place for a global constant ColumnKind DefaultColumnKind = ColumnKind.Utf16CodeUnits. #WontFix

ghost · 2018-12-29T23:58:17Z

src/Sarif.Converters/ToolFileConverterBase.cs

+            output.CloseResults();
+
+            // TODO: should we move these to be emitted at the beginning of the file? What order fo we prefer generaally
+            if (run.Resources?.Rules != null)


if [](start = 12, length = 2)

A tool that emits results as it produces them would naturally put files and logicalLocations at the end, because doesn't know until the end the set of locations it will encounter. Such a tool would also put resources at the end if it chose to emit only those rules that actually triggered and those messages it actually emitted; otherwise it could put resources anywhere.

Also, a user probably wants to get to the results ASAP, without paging through dozens thousands of files before getting to the good stuff.

So IMO all of files, logicalLocations, and resources should go after results. #Resolved

Just to be clear, we're only discussing the converters scenario, not tools that emits results as they produce them. Now that we've DRYed out this code, we have the possibility of making all converter behaviors consistent. I'll put everything afterwards as you say.

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

but btw one consequence of this is that our next converter update will produce wildly divergent content that will be impossible to diff effectively manually. Even if you are Laurence Golding, esq.

In reply to: 244545376 [](ancestors = 244545376,244520670)

ghost · 2018-12-30T00:02:19Z

src/Sarif.Converters/ClangAnalyzerConverter.cs

-                output.OpenResults();
-                output.WriteResults(results);
-                output.CloseResults();
+                PersistResults(output, results, ToolFormat.ClangAnalyzer);


ClangAnalyzer [](start = 59, length = 13)

I don't think you should pass a ToolFormat to the toolName parameter (I see you used a file format as a tool name elsewhere, as well, as in AndroidStudioConverter). There are tools like FxCop and Fortify that can produce multiple formats. I think this confuses two concepts, the tool and its output format. #Closed

Only Fortify produces multiple formats but your point is taken.

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

ghost · 2018-12-30T00:06:16Z

src/Sarif.Converters/FortifyConverter.cs

@@ -102,16 +93,10 @@ public override void Convert(Stream input, IResultLogWriter output, OptionallyEm
                        Text = runDescription
                    }
                },
-                Tool = tool
+                Tool = new Tool {  Name = "Fortify" }


Fortify [](start = 43, length = 7)

Right, here's a case where you can't use the format for the tool name, because there are two formats. Since you can't do it always, I think you should never do it; it's more or less an accident if the format name happens to match the tool name. Besides, the tool's preferred name might not even be a valid identifier (e.g., "Clang Analyzer").

I'll stop harping on this now. #Resolved

ghost · 2018-12-30T00:11:01Z

src/Sarif.Converters/FortifyFprConverter.cs

@@ -116,7 +114,8 @@ public override void Convert(Stream input, IResultLogWriter output, OptionallyEm
                    InstanceId = _automationId + "/"
                },
                Tool = tool,
-                Invocations = new[] { _invocation }
+                Invocations = new[] { _invocation },
+                Resources = new Resources {  Rules = _ruleDictionary}


Rules [](start = 45, length = 5)

Doesn't this mean you'll emit

"resources": { "rules": {} }

if the dictionary is empty, rather than omitting the "rules" object? (In fact, rather than omitting "resources" entirely?) How about:

var run = new Run { ... }; if (_ruleDictionary.Any()) { run.Resources = new Resources { Rules = _ruleDictionary }; } ``` #WontFix

our deserialization logic can elide the resources object in cases where its contained strings and rules are empty, is my thought. this keeps the initialization logic in one place. eventually, we should consider having the OM initialize core objects automatically, to make object creation more concise. will require building out the ShouldSerializeXXX helpers exhaustively. All this is one reason I built that helper that automatically hydrates the OM with all possible items. This will help comprehensively test the helpers described above.

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

ghost · 2018-12-30T00:11:21Z

src/Sarif.Converters/ToolFileConverterBase.cs

@@ -75,5 +75,54 @@ internal static string GetLogicalLocationName(string parentKey, string fullyQual

            return logicalName;
        }
+
+        protected static Run PersistResults(IResultLogWriter output, IList<Result> results, string toolName)


PersistResults [](start = 29, length = 14)

A big +1 for DRYing this out. #ByDesign

ghost · 2018-12-30T00:11:52Z

src/Sarif.Converters/FxCopConverter.cs

            if (rules.Count > 0)
            {
-                IDictionary<string, IRule> rulesDictionary = new Dictionary<string, IRule>();
+                IDictionary<string, Rule> rulesDictionary = new Dictionary<string, Rule>();


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

Yes! Step by step... #ByDesign

ghost · 2018-12-30T00:15:16Z

src/Sarif.Converters/PREFastConverter.cs

-                    output.WriteFiles(fileDictionary);
-                }
+                    Tool = new Tool { Name = ToolFormat.PREfast, FullName = "PREfast Code Analysis" },
+                    ColumnKind = ColumnKind.Utf16CodeUnits


ColumnKind = ColumnKind.Utf16CodeUnits [](start = 20, length = 38)

You could DRY this out by adding

static Run Create() { return new Run { ColumnKind = ColumnKind.Utf16CodeUnits; } }

... to the Run.cs partial class in Sarif\Core. #WontFix

... It's true that then you couldn't use property initialization syntax to fill the rest of the properties, so... whatever you like.

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

We should make a pass over converter run creation in the future to ensure we fully hydrate things like full name.

In reply to: 244521261 [](ancestors = 244521261,244521253)

ghost · 2018-12-30T00:16:34Z

src/Sarif.Converters/PREFastConverter.cs

-                {
-                    output.WriteLogicalLocations(LogicalLocations);
-                }
+                run.LogicalLocations = LogicalLocations;


LogicalLocations [](start = 39, length = 16)

Any reason not to include this in the property initialization syntax above? #Closed

ghost · 2018-12-30T00:19:05Z

src/Sarif.Converters/SemmleQlConverter.cs

+                visitor.VisitResult(result);
+            }
+
+            if (run.Files != null && run.Files.Count > 0)


run.Files != null && run.Files.Count > 0 [](start = 16, length = 40)

if (run.Files?.Count > 0) or if (run.Files?.Any()) (my favorite expression for this)...

... but the base class already does this. #Resolved

run.Files.Any() works because the extension handles the null 'this' case. still, this is unnecessary as you've pointed out, we should be calling the base helper. Simply failed to DRY here.

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

ghost · 2018-12-30T00:26:20Z

src/Sarif.Converters/SemmleQlConverter.cs

@@ -67,9 +67,6 @@ public override void Convert(Stream input, IResultLogWriter output, OptionallyEm

            var results = GetResultsFromStream(input);

-			var fileInfoFactory = new FileInfoFactory(MimeType.DetermineFromFileExtension, dataToInsert);
-			Dictionary<string, FileData> fileDictionary = fileInfoFactory.Create(results);
-
            var tool = new Tool
            {
                Name = "Semmle QL"


"Semmle QL" [](start = 23, length = 11)

I said I'd stop harping on it, but -- here's the other case I mentioned, where you can't use ToolFormat because the tool's preferred name isn't a valid identifier.

Actually this relates to a comment you made in my review -- you asked why I used separate identifiers for constants that held the property name DefaultValueHandling and the enumerated type DefaultValueHandling. It's the same answer: I had two different things that both happened to have the same string value, and I wanted to distinguish them, rather than rely on the happy accident that they happened to be the same string.

I know, I'm pontificating.

(I actually saw a news story the other day that characterized the Pope as "pontificating", and I couldn't decide if the author was trying to be funny...) #ByDesign

'Pons' means bridge, 'facere' means 'to make' and so a pontificator is a bridge builder. YMMV being on the receiving end of it.

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

ghost · 2018-12-30T00:27:24Z

        output.CloseResults();

It looks like you neglected to replace all of lines 80 -- 97 with a call to PersistResults. #Resolved

Refers to: src/Sarif.Converters/SemmleQlConverter.cs:97 in 346bc1d. [](commit_id = 346bc1d, deletion_comment = False)

ghost · 2018-12-30T00:29:15Z

src/Sarif/Writers/ResultLogJsonWriter.cs

@@ -150,23 +150,23 @@ public void Initialize(Run run)
        /// after the results, as the full list of scanned files might not be known until
        /// all results have been generated.
        /// </summary>
-        /// <param name="fileDictionary">
+        /// <param name="files">
        /// A dictionary whose keys are the URIs of scanned files and whose values provide


A dictionary [](start = 12, length = 12)

"A list containing information about the relevant files." #Resolved

ghost · 2018-12-30T00:29:54Z

    /// Write information about scanned files to the log. This information may appear

"relevant" (run.files can contain so much more than scanned files!) #Closed

Refers to: src/Sarif/Writers/ResultLogJsonWriter.cs:149 in 346bc1d. [](commit_id = 346bc1d, deletion_comment = False)

ghost · 2018-12-30T00:32:50Z

src/Sarif/Writers/SarifLogger.cs

+                    fileData.FileLocation = fileLocation;
+
+                    // This call will insert the file object into run.Files if not already present
+                    fileData.FileLocation.FileIndex = run.GetFileIndex(fileData.FileLocation, addToFilesTableIfNotPresent: true);


addToFilesTableIfNotPresent [](start = 94, length = 27)

Could we avoid this by starting out with a hash set? Well, I suppose that won't work because our ValueComparer includes fileIndex...

Hey! I could add a new type of code gen hint that would let you omit properties from the value comparer, what do you think? #Pending

I don't see how your comment is relevant here. 'addToFilesTableIfNotPresent' simply tells the underlying helper whether we are simply querying for the existing index or if it should be added. That underlying code maintains a dictionary of key to the relevant index. So not sure how a hash set helps.

However, yes, the general improvement you're pointing to is quite interesting: how do we capture relevant identity semantics in our complex format in a way that's broadly helpful. What we need here is a special value comparer that's used strictly for the files table. The code gen could then usefully emit a backing Dictionary<FileLocation, int> item that we'd consult in order to manage/populate this thing. We could perform similar tricks for rules and logical locations table (that latter table should have a unique array as well, it's a bug IMO that it isn't expressed this way today)

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

ghost · 2018-12-30T00:34:11Z

src/Sarif/Core/Message.cs

@@ -1,8 +1,6 @@
 // Copyright (c) Microsoft.  All Rights Reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.

-using System;
-using System.Linq;


Linq [](start = 13, length = 4)

Always happy to see this. Ultra nit: extra blank line. #Closed

That's a Roslyn refactoring bug. :)

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

ghost · 2018-12-30T00:35:20Z

src/Sarif/Schemata/sarif-schema.json

@@ -1543,8 +1543,9 @@

        "files": {
          "description": "An array of file objects relevant to the run.",


array [](start = 29, length = 5)

Hah! The comment used to be wrong and is now right. #Closed

I fixed the comment in an early rough check-in (without updating the type details). it wasn't wrong in any shipped version. :)

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

ghost · 2018-12-30T00:40:42Z

src/Sarif/Core/Run.cs

@@ -32,13 +136,13 @@ public bool ShouldSerializeColumnKind()
            return true;
        }

-        public bool ShouldSerializeFiles() { return this.Files != null && this.Files.Values.Any(); }
+        public bool ShouldSerializeFiles() { return this.Files.Any(); }


. [](start = 62, length = 1)

Here and on Line 145, you're assuming that these array-valued properties will be non-null (that is, that they will always be present, even if empty). That means that we'll unnecessarily serialize empty arrays, right? Shouldn't we let them be null, and the use this.TheArrayValuedProperty?.Any()? #Pending

I don't think so. We should only serialize in cases where Any() returns true, meaning that there is at least one item in a non-null collection.

Also, I am not assuming that this.Files is non-null. I am assuming that the Any extension method will handle a null 'this' arg and return false in that case.

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

hm, my assumption may be wrong on looking at relevant source. will test and update appropriately. handling a null 'this' pointer is a cute trick but non-obvious is you're not tracking that the relevant method is an extension method. so it may be verboten in CLR API as breaking principle of least surprise. will follow up.

In reply to: 244604442 [](ancestors = 244604442,244521595)

ghost · 2018-12-30T00:45:33Z

src/Sarif/Core/Run.cs

+            FileLocation fileLocation, 
+            bool addToFilesTableIfNotPresent = true,
+            OptionallyEmittedData dataToInsert = OptionallyEmittedData.None,
+            System.Text.Encoding encoding = null)


System.Text [](start = 12, length = 11)

Just curious how you decide when to inline this and when to add a using. I suppose your criterion is "only used once." And maybe that's better than my practice of always adding a using. It fits with the goal of reducing cognitive load ("Where is Encoding defined?"). #Resolved

My personal rule: inlined fully qualified type name is permissible when used a single time or when required to resolve a type name collision. A using statement is fine as well (except in cases when it requires creating an alias, in which case I generally prefer the fully qualified name).

Somewhere, there is a collision with the 'Encoding' term. I assumed I'd provoke it here but actually did not. Reverted to 'using.

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

ghost · 2018-12-30T00:46:19Z

src/Sarif/Core/Run.cs

+            OptionallyEmittedData dataToInsert = OptionallyEmittedData.None,
+            System.Text.Encoding encoding = null)
+        {
+            if (fileLocation == null) throw new ArgumentNullException(nameof(fileLocation));


throw [](start = 38, length = 5)

I am absolutely fine with this construct, but I think you've told me before that you prefer braces even if it's a single statement on the same line. #Closed

it's not my preference, it's my attempt to create a standard that balances the preferences of many.

i.e., i do not set the coding guidelines in this code base according to my own preferences but instead attempt to help negotiate a set of standards that usefully encompasses valid concerns raised by the range of developers who contribute.

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

ghost · 2018-12-30T00:47:47Z

src/Sarif/Core/Run.cs

+        {
+            if (fileLocation == null) throw new ArgumentNullException(nameof(fileLocation));
+
+            if (this.Files == null || this.Files.Count == 0)


this.Files == null || this.Files.Count == 0 [](start = 16, length = 43)

this.Files?.Any()... BUT! If Files is missing and addToFilesTableIfNotPresent is true, then wouldn't this be a good time to create Files? #Closed

Well, I guess I see what you're after -- if the producer doesn't initialize run.Files with the list of scanned files, maybe that means the producer didn't want a files array, so we shouldn't force it on him here. But in that case, maybe the producer just shouldn't call this method?

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

No. This is just a bad bug you've identified that the unit test authoring has closed. In the case of files being null and 'addFileToTableIfNotPresent' == true, that boolean value would not be honored. Basically, this method should never return -1 if that value is true

In reply to: 244521733 [](ancestors = 244521733,244521702)

ghost · 2018-12-30T00:50:56Z

src/Sarif/Core/Run.cs

+                _fileToIndexMap[fileLocation] = i;
+
+                // For good measure, we'll explicitly populate the file index property
+                this.Files[i].FileLocation.FileIndex = i;


FileIndex [](start = 43, length = 9)

This could be in the property initializer list. #Closed

ghost · 2018-12-30T00:52:30Z

src/Sarif.Converters/TSLintConverter.cs

@@ -21,41 +20,18 @@ public TSLintConverter()

        public override void Convert(Stream input, IResultLogWriter output, OptionallyEmittedData dataToInsert)
        {
-            input = input ?? throw new ArgumentNullException(nameof(input));
-
+            input = input   ?? throw new ArgumentNullException(nameof(input));


[](start = 25, length = 3)

Accidental extra space?

"throw expression" FTW. Nice language feature. #Closed

was actually intentional but you'd have to be in the code editor to see why i lined things up for readability. we don't have a lot of this sort of formatting as a standard practice (it's pervasive in most C++ code bases I've worked on). reverted.

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

Oh, right, lines up with "output". If I'd noticed that I wouldn't have commented.

In reply to: 244605570 [](ancestors = 244605570,244521769)

ghost · 2018-12-30T00:54:52Z

src/Sarif/Core/Run.cs

+            // When we perform a files table look-up, only the uri and uriBaseId
+            // are relevant; these properties together comprise the unique identity
+            // of the file object. The file index, of course, does not relate to the
+            // file identity. We consciously exclude the properties as well.


. We consciously exclude the properties as well [](start = 28, length = 47)

"..., so we exclude it" ? Or "..., so we exclude that property?" Not sure what to make of the plural here. #Closed

we exclude the properties bag.

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

ghost · 2018-12-30T00:59:21Z

src/Sarif/Core/Run.cs

+                {
+                    fileIndex = this.Files.Count;
+
+                    string mimeType = Writers.MimeType.DetermineFromFileExtension(filesTableKey.Uri.ToString());


ToString() [](start = 100, length = 10)

Uri.AbsolutePath is probably faster, and makes it clear where you're getting the extension from. #WontFix

Uri.AbsolutePath throws in cases where the URI isn't in fact, absolute. Uri.ToString() gets us where we want to be. Uri.OriginalString might also be sufficient though.

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

ghost · 2018-12-30T01:01:03Z

src/Sarif/Core/Run.cs

+                    {
+                        MimeType = mimeType,
+                        FileLocation = filesTableKey
+                    });


Shouldn't you be adding the var fileData that you just created? #Closed

yes. arg.

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

ghost · 2018-12-30T01:02:13Z

src/Sarif/Core/Run.cs

+                }
+            }
+
+            fileLocation.FileIndex = fileIndex;


fileIndex [](start = 37, length = 9)

If you didn't enter the if block on Line 62, then this is uninitialized. I'm surprised this compiles. #ByDesign

variable declared on line 61. variable is initialized on call on line 62 (where it is used as an 'out' param). the only way line 62 isn't entered is in the event of an exception, in which case line 93 will not execute. now, if this code was executing in a 'finally' clause, you'd see the compilation error.

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

ghost

ghost · 2018-12-30T18:49:15Z

I had several comments, but nothing that I'd push back on whatever you decide (except for that whole "tool format" vs. "tool name" thing -- I feel pretty strongly about that. So, "approved", and do as ye will.

ghost

ghost

ghost

ghost · 2018-12-31T19:26:01Z

When you DRY'd out to PersistResults in Iteration 12, I think you lost the tool notifications.

Refers to: src/Sarif.Converters/SemmleQlConverter.cs:102 in 5a29dd7. [](commit_id = 5a29dd7, deletion_comment = True)

* Fix up tests * Conversion to files array. WIP. Core SARIF component build complete except for SarifLogger tail. * Add fileIndex property to file object (#1186) * Fix up tests * PR feedback to improve schema comment * Logical locations notes (#1185) (#1187) * Respond to a small # of PR comments related to recent logical locations change. * Fix visibility on helper * Fix up v1 transformation with keys that collide * Preserve decorated name data * Rebaseline test for decorated name propagation * Respond to PR feedback. Update tests to close test holes. * Rebaseline updated test * Test key collision in annotated code locations. * Update baseline * Reduced files array build (#1191) * Sarif and Sarif.Converters now building * Files array (#1188) * Add fileIndex property to file object (#1186) * Fix up tests * PR feedback to improve schema comment * Logical locations notes (#1185) (#1187) * Respond to a small # of PR comments related to recent logical locations change. * Fix visibility on helper * Fix up v1 transformation with keys that collide * Preserve decorated name data * Rebaseline test for decorated name propagation * Respond to PR feedback. Update tests to close test holes. * Rebaseline updated test * Test key collision in annotated code locations. * Update baseline * DRY out converters to isolate shared code. * Restore essential change in schema that converts files dictionary to an array. * Simplify ShouldSerialize logic * Remove unused namespaces * Respond to PR feedback. * Respond to PR feedback * End-to-end build works. Now we can author core transformation and fix tests. (#1192) * Fix up merge from 'develop' branch. * Update supporting test code for processing checked in files. (#1202) * Update supporting test code for processing checked in files. * Update nested files test to contain single file. * Files array basic transform (#1205) * Update supporting test code for processing checked in files. * Update nested files test to contain single file. * WIP. Furhter progress * Fix up samples build * Fix up merge from basic transform branch * Mime type validation (#1206) * Fix up merge from basic transform branch * Fix up mime test * Start work on v1 <-> v2 transformation (#1208) * Restore TransformCommand and first (unaffected) unit test * Restore "minimal prerelease v2 to current v2" test. * estore "minimal v1 to current v2" test. * Restore remaining TransformCommand unit tests. * Uncomment v2 => v1 tests to observe failures. * Uncomment 'transform' command. * Restore MakeUrisAbsoluteVisitor tests (#1210) This change updates the visitor that expands URIs in the presence of `originalUriBaseIds`. Turns out there was technical debt here, because our tests provided `originalUriBaseIds` equivalents in the property bag (because we had no official SARIF slot for them). I did not notice this gap when we made the schema change to add `originalUriBaseIds`. * Get v2 -> v1 transform working with files array (#1211) Test failure count is down to 32; will be 28 when you merge your fix. There is not -- and never was -- a test case for fileLocations that use uriBaseId (never was one). I know for a fact that there is no code to support that case. You’ll see a comment to that effect in the code. I will take care of that next. Then I will move on to v1 -> v2 transform. As part of this change, the `SarifCurrentToVersionOneVisitorTests` are now based on the `RunTest` helper method from the base class `FileDiffingTests`. * Convert debug assert to exception to make test execution more deterministic (#1214) * Update insert optional data tests and update indices visitor. (#1212) * Update insert optional data tests and update indices visitor. * Delete speculatively needed file * Remove erroneous override of base visit method. * Rework summary comment on DeletesOutputsDirectoryOnClassInitializationFixture. * Update clang analyzer name. Flow converter log verification through JToken comparison. (#1213) * The multiool, core sarif, and validation test binaries now all pass (#1215) * The multiool, core sarif, and validation test binaries now all pass completely. * Remove unwanted assert that may fire during unit testing. * Merge from files-array * PR feedback. * PR feedback tweaks * Accept PR feedback from previous change. (#1216) Use LINQ IEnuemrable.Except in the unit test, which improves readability without compromising efficiency (because Except uses a Set to do its work in O(N) time). * Fix Sarif.Driver and Sarif.Functional tests. (#1217) * Fix Sarif.Driver and Sarif.Functional tests. * Restore test file * Fix Semmle tests and Fortify converter: all tests now pass. (#1218) * Sarif converters fixups (#1219) * Fix semmle tests and fority. * Final test fixups * Invoke appveyor for files-array branch.: (#1220) * Update SarifVersionOneToCurrentVisitor for run.files array (#1221) * Uncomment v1 -> v2 tests; 3/14 fail. * Move test data to locations expected by FileDiffingTests. * Fix up some IDE C#7 code cleanups. * Use FileDiffingTests helper. * Fix bug in FileDiffingTests that caused a test failure. * Remove default-valued argument from a call to RunTest. * Create basic files array Does not yet have fileIndex, parentIndex, or response file handling. * Revert incorrect change in FileDiffingTests. * Fix one unit test with spot fix to "expected" file. * Fix up some C#7 IDE warnings * Force update in FileDiffing tests to avoid deserialization errors from out of date "expected" files. * Fix missing "modified" flag sets in PreRelCompatTransformer. * Populate fileIndex in run.files array. * Fix unit test by fixing fileLocation creation. * Restore response file handling. * Populate fileIndex on fileLocations as appropriate. * Fix last test failure by reworking response file handling. * Feedback: Introduce transformer helper PopulatePropertyIfAbsent. * Feedback: Tighten platform-independent string compare. Also: - Reformat unit test lines. * Feedbakc: Revert FileDiffingTest change; downgrade affected test files to provoke transform * Basic rules transformation (except for v1 <-> v2 conversion) (#1223) * Basic rules transformation (except for v1 <-> v2 conversion) * Respond to very excellent PR feedback. * PR feedback * Add files array tests for nested files and uriBaseIds (#1226) * Add failing v1 -> v2 nested files test * Fix existing mis-handling of analysisTarget and resultFile. * Get nested files test case working. * Add failing v1 => v2 uriBaseId test. * Populate v2 uriBaseId. * Fix up expected v2 fileLocation.properties: test passes. * Enhance uriBaseIds test case. * Implement v2->v1 conversion for rules dictionary (#1228) * Notification rule index (#1229) * Add notification.ruleIndex and increase flatten messages testing * Notification message tests + add notification.ruleIndex to schema * Notification rule index (#1230) * Add notification.ruleIndex and increase flatten messages testing * Notification message tests + add notification.ruleIndex to schema * Missed feedback from previous PR (#1231) * Implement v1->v2 conversion for rules dictionary (#1232) * Partial implementation * Get last test working. * Somebody missed checking in a generated file. * Schema changes from TC #30 (#1233) * Add source language, fix rank default. * Adjust rank minimum to accommoodate default. * Fix broken test. * Remove unnecessary None items from project file. * PR feedback * Files array results matching (#1234) * WIP preliminary results matching * Restore results matching for files array * Add back autogenerated (unused) namespace directive

@fishoak

…#1264) * Fix tests that are broken in appveyor (#1134) * Properly persist run level property bags (#1136) * Fix #1138: Add validation rule: contextRegion requires region (#1142) Also: - Enhance the "new-style" verification so that we no longer require the file "Invalid_Expected.sarif". Each file can now contain a property that specifies the expected locations of all the validation results. * Prep for 2018-11-28 schema update. Remove run.architecture. (#1145) * Add run.newlineSequences to schema (#1146) * Mark result.message as required in the schema (#1147) * Mark result.message as required in the schema * Update release history with result.message breaking change. * Fix typo in testoutput. * Rename tool.fileVersion to tool.dottedQuadFileVersion (#1148) * Upgrade more validator functional tests (#1149) We apply the new functional test pattern to four more rules: - `EndColumnMustNotBeLessThanStartColumn` - `EndLineMustNotBeLessThanStartLine` - `EndTimeMustBeAfterStartTime` (which is misnamed, and in a future PR we will rename it to `EndTimeMustNotBeBeforeStartTime`) - `MessageShouldEndWithPeriod` In addition, we remove the test data for a rule that no longer exists, `HashAlgorithmsMustBeUnique` (which no longer applies because `file.hashes` is now an object keyed by the hash algorithm). Because there are so many properties of type `message`, exhaustively testing the rule `MessageShouldEndWithPeriod` revealed many holes in the visitor class `SarifValidationSkimmerBase`, which I plugged. As we have discussed, we should generate this class from the schema. After this, there are only two more rules to convert: - `UriBaseIdRequiresRelativeUri` - `UseAbsolutePathsForNestedFileUriFragments` ... but this PR is already large enough. * Remove 'open' from list of valid rule configuration default values. (#1158) * Emit column kind default explicitly for Windows SDK SARIF emit. (#1160) * Emit column kind default explicitly for Windows SDK SARIF emit. * Update release notes * More column kind test fixes * Change behavior to always serialize column kind. * Always serialize column kind * Finish validator functional test upgrade (#1159) * Rename rule EndTimeMustBeAfterStartTime => ...MustNotBeBefore... * Upgrade UriBaseIdRequiresRelativeUri tests. * Remove obsolete rule UseAbsolutePathsForNestedFileUriFragments. * Remove support for old test design. * Remove 'package' as a documented logical location kind in the schema. Documentation only change. (#1162) * Fortify FPR converter improvements + unit tests (#1161) * Improvements and corrections Populate originalUriBaseIds from <SourceBasePath> Populate tFL.kind from <Action type="..."> Add default node to result.locations * Add location annotation for Action elements with no type attribute * Support node annotations + uribasepath + test updates * Update FortifyTest.fpr.sarif * Add converter tests & assets + opportunistic code cleanup * PR feedback * Logical locations dictionaries to arrays (#1170) The core change here is the transformation of `run.logicalLocations` from a dictionary (which is keyed, generally, by the fully qualified name of a logical location) to an array of logical locations. Result locations now reference logical locations by a logical location index. This changes removes the necessity of resolving key name collisions for logical locations that differ only by type (a namespace that collides with the fully qualified name of a type being the classic example). In addition to making the core change, we have also authored a transformation that converts existing pre-release SARIF v2 files to the new design. We accomplish this by creating dictionaries, with value type comparison for keys, that are keyed by logical locations. This processing requires that any parent keys already exist in the array (so that a logical location can populate its parent logical location index, if any). In addition to the core functionality and any transformation of individual log files, result matching presents special complications. In a result matching scenario, the logical location index of a result is not relevant to its identify: only the contents of the logical location this index points to are relevant. Furthermore, when merging a baseline file (which can contain results that are exclusive to a single log file within the matching domain), logical location indices are subject to change and must be updated. For this scenario and at least one other, we use a visitor pattern to update indices. The reason is that locations are pervasive in the format and the visitor provides a convenient mechanism to put common location processing logical. This visitor uses puts additional pressure on the transformation logic, as it entails additional deserialization of the JSON. With more time/effort, we could have exhaustively updated all locations using the JParser/JObject/etc. API domain. Oh well. Finally, we must update the logical that transforms v1 to v2 and vice versa. Whew. If that was not already sufficiently intrusive, this work revealed some minor flaws in various converters (the ones that handle logical locations): AndroidStudio, FxCop and PREfast. This change is complex but valuable. Logical locations are now expressed as coherent objects in their table. In the main, I have preferred to leave `result.fullyQualifiedName` populated (in addition to `result.logicalLocationIndex`, to support easily looking up matching logical locations). * Add result.rank and ruleConfiguration.defaultRank (#1167) As we discussed offline with @fishoak, the design is good as it stands. The only change is that the default should be -1. I filed oasis-tcs/sarif-spec#303 for that, and put it on the agenda for TC #30. * Logical locations notes (#1184) * Respond to a small # of PR comments related to recent logical locations change. * Fix visibility on helper * Logical locations notes (#1185) * Respond to a small # of PR comments related to recent logical locations change. * Fix visibility on helper * Fix up v1 transformation with keys that collide * Preserve decorated name data * Rebaseline test for decorated name propagation * Respond to PR feedback. Update tests to close test holes. * Rebaseline updated test * Test key collision in annotated code locations. * Update baseline * Incorporate "provenance" schema changes and fix package licenses (#1193) * Add autogenerated RuleConfiguration.cs missed from earlier commit. * Upgrade to NuGet.exe 4.9.2 to handle new license tag. * Remove unused 'Owners' element from build.props. * Add package Title. * Use packageLicenseExpression to specify package license. * Suppress NU5105 (about SemVer 2.0.0 strings) for "dotnet pack" packages. NuGet.exe still warns for ".nuspec" packages. * Incorporate latest "provenance" schema changes. * Address PR feedback. * External property files (#1194) * Update spec for externalPropertiesFile object. * Add external property files to schema. * Finish merge of provenance changes. * Update release notes. * Remove vertical whitespace. * PR feedback. Fix 'properties' to refer to an external file not an actual properties bag. * Remove code gen hint that makes external property files a property bag holder. * Introduce missing brace. Fix up code emit for 'properties' property that isn't a property bag. * Incorporate schema changes for versionControlDetails.mappedTo and rule.deprecatedIds (#1198) * Incorporate "versionControlDetails.mappedTo" schema change. * Incorporate "rule.deprecatedIds" schema change. * Revert updates to comprehensive.sarif (to allow transformer to continue to use this as test content). * Array scrub part 1: everything but anyOf-or-null properties. (#1201) NOTE: For explicitness, I added schema attributes even when they matched the JSON schema defaults, namely: `"minItems": 0` and `"uniqueItems": false`. * Fix v1->v2 hash transformation (#1203) CreateHash must be called to handle algorithm names that aren't in our translation table. Also updated a unit test to cover this case. * Integrate jschema 0.61.0 into SDK (#1204) * Merging arrays transformations back into 'develop' branch (#1236) * Fix up tests * Conversion to files array. WIP. Core SARIF component build complete except for SarifLogger tail. * Add fileIndex property to file object (#1186) * Fix up tests * PR feedback to improve schema comment * Logical locations notes (#1185) (#1187) * Respond to a small # of PR comments related to recent logical locations change. * Fix visibility on helper * Fix up v1 transformation with keys that collide * Preserve decorated name data * Rebaseline test for decorated name propagation * Respond to PR feedback. Update tests to close test holes. * Rebaseline updated test * Test key collision in annotated code locations. * Update baseline * Reduced files array build (#1191) * Sarif and Sarif.Converters now building * Files array (#1188) * Add fileIndex property to file object (#1186) * Fix up tests * PR feedback to improve schema comment * Logical locations notes (#1185) (#1187) * Respond to a small # of PR comments related to recent logical locations change. * Fix visibility on helper * Fix up v1 transformation with keys that collide * Preserve decorated name data * Rebaseline test for decorated name propagation * Respond to PR feedback. Update tests to close test holes. * Rebaseline updated test * Test key collision in annotated code locations. * Update baseline * DRY out converters to isolate shared code. * Restore essential change in schema that converts files dictionary to an array. * Simplify ShouldSerialize logic * Remove unused namespaces * Respond to PR feedback. * Respond to PR feedback * End-to-end build works. Now we can author core transformation and fix tests. (#1192) * Fix up merge from 'develop' branch. * Update supporting test code for processing checked in files. (#1202) * Update supporting test code for processing checked in files. * Update nested files test to contain single file. * Files array basic transform (#1205) * Update supporting test code for processing checked in files. * Update nested files test to contain single file. * WIP. Furhter progress * Fix up samples build * Fix up merge from basic transform branch * Mime type validation (#1206) * Fix up merge from basic transform branch * Fix up mime test * Start work on v1 <-> v2 transformation (#1208) * Restore TransformCommand and first (unaffected) unit test * Restore "minimal prerelease v2 to current v2" test. * estore "minimal v1 to current v2" test. * Restore remaining TransformCommand unit tests. * Uncomment v2 => v1 tests to observe failures. * Uncomment 'transform' command. * Restore MakeUrisAbsoluteVisitor tests (#1210) This change updates the visitor that expands URIs in the presence of `originalUriBaseIds`. Turns out there was technical debt here, because our tests provided `originalUriBaseIds` equivalents in the property bag (because we had no official SARIF slot for them). I did not notice this gap when we made the schema change to add `originalUriBaseIds`. * Get v2 -> v1 transform working with files array (#1211) Test failure count is down to 32; will be 28 when you merge your fix. There is not -- and never was -- a test case for fileLocations that use uriBaseId (never was one). I know for a fact that there is no code to support that case. You’ll see a comment to that effect in the code. I will take care of that next. Then I will move on to v1 -> v2 transform. As part of this change, the `SarifCurrentToVersionOneVisitorTests` are now based on the `RunTest` helper method from the base class `FileDiffingTests`. * Convert debug assert to exception to make test execution more deterministic (#1214) * Update insert optional data tests and update indices visitor. (#1212) * Update insert optional data tests and update indices visitor. * Delete speculatively needed file * Remove erroneous override of base visit method. * Rework summary comment on DeletesOutputsDirectoryOnClassInitializationFixture. * Update clang analyzer name. Flow converter log verification through JToken comparison. (#1213) * The multiool, core sarif, and validation test binaries now all pass (#1215) * The multiool, core sarif, and validation test binaries now all pass completely. * Remove unwanted assert that may fire during unit testing. * Merge from files-array * PR feedback. * PR feedback tweaks * Accept PR feedback from previous change. (#1216) Use LINQ IEnuemrable.Except in the unit test, which improves readability without compromising efficiency (because Except uses a Set to do its work in O(N) time). * Fix Sarif.Driver and Sarif.Functional tests. (#1217) * Fix Sarif.Driver and Sarif.Functional tests. * Restore test file * Fix Semmle tests and Fortify converter: all tests now pass. (#1218) * Sarif converters fixups (#1219) * Fix semmle tests and fority. * Final test fixups * Invoke appveyor for files-array branch.: (#1220) * Update SarifVersionOneToCurrentVisitor for run.files array (#1221) * Uncomment v1 -> v2 tests; 3/14 fail. * Move test data to locations expected by FileDiffingTests. * Fix up some IDE C#7 code cleanups. * Use FileDiffingTests helper. * Fix bug in FileDiffingTests that caused a test failure. * Remove default-valued argument from a call to RunTest. * Create basic files array Does not yet have fileIndex, parentIndex, or response file handling. * Revert incorrect change in FileDiffingTests. * Fix one unit test with spot fix to "expected" file. * Fix up some C#7 IDE warnings * Force update in FileDiffing tests to avoid deserialization errors from out of date "expected" files. * Fix missing "modified" flag sets in PreRelCompatTransformer. * Populate fileIndex in run.files array. * Fix unit test by fixing fileLocation creation. * Restore response file handling. * Populate fileIndex on fileLocations as appropriate. * Fix last test failure by reworking response file handling. * Feedback: Introduce transformer helper PopulatePropertyIfAbsent. * Feedback: Tighten platform-independent string compare. Also: - Reformat unit test lines. * Feedbakc: Revert FileDiffingTest change; downgrade affected test files to provoke transform * Basic rules transformation (except for v1 <-> v2 conversion) (#1223) * Basic rules transformation (except for v1 <-> v2 conversion) * Respond to very excellent PR feedback. * PR feedback * Add files array tests for nested files and uriBaseIds (#1226) * Add failing v1 -> v2 nested files test * Fix existing mis-handling of analysisTarget and resultFile. * Get nested files test case working. * Add failing v1 => v2 uriBaseId test. * Populate v2 uriBaseId. * Fix up expected v2 fileLocation.properties: test passes. * Enhance uriBaseIds test case. * Implement v2->v1 conversion for rules dictionary (#1228) * Notification rule index (#1229) * Add notification.ruleIndex and increase flatten messages testing * Notification message tests + add notification.ruleIndex to schema * Notification rule index (#1230) * Add notification.ruleIndex and increase flatten messages testing * Notification message tests + add notification.ruleIndex to schema * Missed feedback from previous PR (#1231) * Implement v1->v2 conversion for rules dictionary (#1232) * Partial implementation * Get last test working. * Somebody missed checking in a generated file. * Schema changes from TC #30 (#1233) * Add source language, fix rank default. * Adjust rank minimum to accommoodate default. * Fix broken test. * Remove unnecessary None items from project file. * PR feedback * Files array results matching (#1234) * WIP preliminary results matching * Restore results matching for files array * Add back autogenerated (unused) namespace directive * Updated release notes for TC30 changes. (#1240) * Mention rules array change in release history. (#1243) * Baseline states (#1245) * Add 'updated' state to baselineState and rename 'existing' to 'unchanged' * Update prerelease transformer * Enable appveyor build + test. Correct version constant. * Update test. Respond to PR feedback. * Fix #1251 #1252 #1253 (#1254) * Fixes + test coverage + cleanup * Update SDK version * Update version in test assets * Fix multitool nuspec (#1256) * Revert unintentional change to BaselineState (#1262) The `develop` branch should match TC #30, but we inadvertently introduced a change from TC #31: replacing `BaselineState.Existing` with `.Unchanged` and `Updated`. I did not revert the entire change. Some things (like having AppVeyor build the `tc-31` branch instead of the defunct `files-array` branch, and some C# 7 updates to the `PrereleaseCompatibilityTransformer`) were good, and I kept them. Also: - Update the version to `2019-01-09` in preparation for merge to `master`. * Transfer Bogdan's point fix (analysisTarget handling) from master to develop (#1263) In preparation for merging `develop` to `master` for the publication of version 2019-01-09 (TC #30), we apply the recent changes in `master` to the `develop` branch. These changes fixed two bugs in the handling of `analysisTarget` in the v1-to-v2 converter (`SarifVersionOneToCurrentVisitor`). Now `develop` is completely up to date, and when we merge `develop` to `master`, we _should_ be able to simply take the "incoming" changes on all conflicting files. * Cherry-pick: v1 transformer analysis target region persistence fix. (#1238) * Mention NuGet publishing changes in release history. * Cherry pick: Don't emit v2 analysisTarget if there is no v1 resultFile. (#1247)

michaelcfanning added 6 commits December 28, 2018 16:17

Sarif and Sarif.Converters now building

e56f46e

DRY out converters to isolate shared code.

6de4aee

Merge from files array branch

c296771

Merge branch 'reduced-files-array-build' of https://github.com/micros…

9b7a6c4

…oft/sarif-sdk into reduced-files-array-build

Restore essential change in schema that converts files dictionary to …

b9b5a15

…an array.

michaelcfanning commented Dec 29, 2018

View reviewed changes

michaelcfanning added 2 commits December 29, 2018 12:20

Simplify ShouldSerialize logic

7ec751b

Remove unused namespaces

346bc1d

michaelcfanning commented Dec 29, 2018

View reviewed changes

ghost reviewed Dec 29, 2018

View reviewed changes

ghost reviewed Dec 30, 2018

View reviewed changes

ghost approved these changes Dec 30, 2018

View reviewed changes

michaelcfanning added 4 commits December 31, 2018 09:55

Respond to PR feedback.

0c3e250

Respond to PR feedback

5208c43

Merge branch 'files-array' into reduced-files-array-build

5a29dd7

Further PR feedback.

43480b0

Lost PR feedback recovered.

8d505f0

michaelcfanning merged commit 621ec57 into files-array Dec 31, 2018

michaelcfanning deleted the reduced-files-array-build branch January 28, 2019 18:08


		internal static bool UnitTesting = false;

		internal static void DebugAssert(bool conditional)

		@@ -1543,8 +1543,9 @@

		"files": {
		"description": "An array of file objects relevant to the run.",

Reduced files array build #1191

Reduced files array build #1191

Conversation

michaelcfanning commented Dec 29, 2018

michaelcfanning Dec 29, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Dec 29, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Dec 29, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Dec 29, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 29, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

ghost Dec 29, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 30, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 30, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

ghost Dec 30, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 30, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

ghost Dec 30, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

ghost Dec 30, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 30, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Dec 30, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 30, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Dec 30, 2018 • edited by michaelcfanning Loading

ghost Dec 30, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

ghost commented Dec 30, 2018 • edited by ghost Loading

ghost Dec 30, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 30, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 30, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 30, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 30, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 30, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 30, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 30, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Dec 30, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 30, 2018 • edited by ghost Loading

michaelcfanning Dec 29, 2018 •

edited by ghost

Loading

michaelcfanning Dec 29, 2018 •

edited by ghost

Loading

michaelcfanning Dec 29, 2018 •

edited by ghost

Loading

ghost Dec 29, 2018 •

edited by ghost

Loading

ghost Dec 29, 2018 •

edited by michaelcfanning

Loading

ghost Dec 29, 2018 •

edited by michaelcfanning

Loading

ghost Dec 30, 2018 •

edited by ghost

Loading

ghost Dec 30, 2018 •

edited by michaelcfanning

Loading

ghost Dec 30, 2018 •

edited by michaelcfanning

Loading

ghost Dec 30, 2018 •

edited by michaelcfanning

Loading

ghost Dec 30, 2018 •

edited by michaelcfanning

Loading

ghost Dec 30, 2018 •

edited by michaelcfanning

Loading

ghost Dec 30, 2018 •

edited by ghost

Loading

ghost Dec 30, 2018 •

edited by michaelcfanning

Loading

ghost Dec 30, 2018 •

edited by michaelcfanning

Loading

ghost commented Dec 30, 2018 •

edited by michaelcfanning

Loading

ghost Dec 30, 2018 •

edited by michaelcfanning

Loading

ghost commented Dec 30, 2018 •

edited by ghost

Loading

ghost Dec 30, 2018 •

edited by michaelcfanning

Loading

ghost Dec 30, 2018 •

edited by ghost

Loading

ghost Dec 30, 2018 •

edited by michaelcfanning

Loading

ghost Dec 30, 2018 •

edited by michaelcfanning

Loading

ghost Dec 30, 2018 •

edited by michaelcfanning

Loading

ghost Dec 30, 2018 •

edited by ghost

Loading

ghost Dec 30, 2018 •

edited by ghost

Loading

ghost Dec 30, 2018 •

edited by ghost

Loading

ghost Dec 30, 2018 •

edited by ghost

Loading

ghost Dec 30, 2018 •

edited by ghost

Loading

ghost Dec 30, 2018 •

edited by michaelcfanning

Loading

ghost Dec 30, 2018 •

edited by ghost

Loading

ghost Dec 30, 2018 •

edited by michaelcfanning

Loading