Logical locations dictionaries to arrays #1170

michaelcfanning · 2018-12-21T21:37:08Z

The core change here is the transformation of logical locations from dictionaries (which are 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).

@lgolding @EasyRhinoMSFT @rtaket

…arlier (due to converter bugs)

…ked in baselines.

michaelcfanning · 2018-12-21T21:49:52Z

src/Sarif/Baseline/ResultMatching/SarifLogMatcher.cs

+                Result result = resultPair.CalculateBasedlinedResult(PropertyBagMergeBehavior);
+                newRunResults.Add(resultPair.CalculateBasedlinedResult(PropertyBagMergeBehavior));
+
+                var logicalLocationIndexRemapper = new LogicalLocationIndexRemapper(resultPair.LogicalLocations, remappedLogicalLocations);


LogicalLocationIndexRemapper [](start = 55, length = 28)

this visitor fixes up indices so that merged results (absent, shared and new) all have logical location index property values that correspond to the newly merged logical locations in the computed baseline. #ByDesign

michaelcfanning · 2018-12-21T21:50:53Z

src/Sarif/NotYetAutoGenerated/Location.cs

+        /// </summary>
+        public Location()
+        {
+            LogicalLocationIndex = -1;


LogicalLocationIndex = [](start = 12, length = 23)

It is helpful for constructors to populate default values. JSON.NET won't do so except when deserializing. our current jschema code does not do this. #Closed

Documented gap in the following issue:
microsoft/jschema#57

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

michaelcfanning · 2018-12-21T21:51:18Z

src/Sarif/NotYetAutoGenerated/Location.cs

+        /// </summary>
+        [DataMember(Name = "logicalLocationIndex", IsRequired = false, EmitDefaultValue = false)]
+        [JsonProperty(DefaultValueHandling = DefaultValueHandling.IgnoreAndPopulate)]
+        [System.ComponentModel.DefaultValue(-1)]


missing jschema functionality. we must manually apply these attributes. #WontFix

No, we have AttributeHint for that. See CodeGenHints.json for examples. In fact, you used it extensively to apply the JsonConverter attribute when you removed the contract resolver :-)

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

Right. Still would be helpful to have more automatic code gen for this. Note that System.ComponentModel.DefaultValue is a standard attribute. A more important gap, mentioned in another comment thread: we need to initialize in ctor. I've updated comments in the issue below.

microsoft/jschema#57

In reply to: 243719649 [](ancestors = 243719649,243699993)

michaelcfanning · 2018-12-21T21:52:52Z

src/Sarif/NotYetAutoGenerated/SarifRewritingVisitor.cs

@@ -756,13 +756,11 @@ public virtual Run VisitRun(Run node)

                if (node.LogicalLocations != null)
                {
-                    var keys = node.LogicalLocations.Keys.ToArray();
-                    foreach (var key in keys)
+                    if (node.LogicalLocations != null)


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

i have to manually update this visitor because our jschema code does not have the logical to properly visit all dictionaries (where the key value might be updated). the more dictionary to array conversion work we do, the less we need this feature and may be able to drop the request altogether. #Resolved

I don't actually understand this comment. It seems to be saying something about how the code that JSchema generates treats dictionaries, but logicalLocations is now an array -- Oh! you are saying that if only all the dictionaries were arrays, we wouldn't need this hand-coded visitor, and this is a step in the right direction. Got it.

No need to repeat the test from line 757.

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

michaelcfanning · 2018-12-21T21:59:48Z

src/Sarif.FunctionalTests/v2/ConverterTestData/PREfast/Expected-022.xml.sarif

@@ -1,6 +1,6 @@
 {
-  "$schema": "http://json.schemastore.org/sarif-2.0.0-csd.2.beta.2018-09-26",
-  "version": "2.0.0-csd.2.beta.2018-09-26",
+  "$schema": "http://json.schemastore.org/sarif-2.0.0-csd.2.beta.2018-11-28",


$schema [](start = 3, length = 7)

A small # of test files had some inconsistencies that would have required manual updates to resolve. Instead, I've simply converted them to the current version of SARIF. We have plenty of test content that continues to be transformed (in fact, the bulk of our test content is still pre-release v2). It is impressive that our transformer continues to allow all our testing to function. #ByDesign

michaelcfanning · 2018-12-21T22:01:22Z

src/Sarif.FunctionalTests/v2/ConverterTestData/PREfast/Expected-050.xml

-PREFAST_NEWLINE
-Accessing the element (4 bytes/element) at element offset i`4
-PREFAST_NEWLINE
+<DEFECTS>


DEFECTS [](start = 1, length = 7)

Here, I've expressly expanded some PREfast log file content based on reformatting in VS. This has slightly increased the variability of the test content. #ByDesign

michaelcfanning · 2018-12-21T22:03:27Z

src/Sarif.Multitool/TransformCommand.cs

@@ -67,7 +67,12 @@ public int Run(TransformOptions transformOptions)
                    else
                    {
                        // We have a pre-release v2 file that we should upgrade to current. 
-                        string sarifText = PrereleaseCompatibilityTransformer.UpdateToCurrentVersion(_fileSystem.ReadAllText(inputFilePath), formatting: formatting);
+                        PrereleaseCompatibilityTransformer.UpdateToCurrentVersion(


PrereleaseCompatibilityTransformer [](start = 24, length = 34)

This helper now returns the transformed contents as a SarifLog. We need to do this in the transformer in order to invoke the visitors that rewrite indices. There is no sense converting this log file back to text again, so that a consumer deserializes yet one more time. Instead, we have a rather cumbersome API that can be used to get the transformed text as an out arg or the transformed log file as a returned value. Note that the out argument is set to the original contents that are passed to the API, in failure cases and when the file doesn't need to be touched at all. #Closed

IMO the out parameter should start out as null to handle the failure case. Later on, if no transform was necessary, it can be set to the input text.

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

I looked at PrereleaseCompatibilityTransformer.UpdateToCurrentVersion and contrary to this comment, it does initially set the out parameter to null, as I suggested it should, so there's nothing to do here.

In reply to: 243852926 [](ancestors = 243852926,243701919)

michaelcfanning · 2018-12-21T22:04:15Z

src/Sarif.Multitool.FunctionalTests/TestData/DeserializationError_Expected.sarif

@@ -33,7 +33,7 @@
                  "uri": "file:///C:/Code/sarif-sdk/bld/bin/Sarif.Multitool.FunctionalTests/AnyCPU_Debug/netcoreapp2.0/TestData/DeserializationError.sarif"
                },
                "region": {
-                  "startLine": 15,
+                  "startLine": 13,


13 [](start = 31, length = 2)

Larry, I can't account for this line change. Does it make sense to you? #ByDesign

There are two things happening:

We've changed Newtonsoft versions since the last time we regenerated this file, and

The test is not sensitive to startLine.

The error JSON1002 is "Required property is missing.". The arguments (Line 23) tell us that the required frames property of the stack object is missing, which is true. The startLine is where Newtonsoft thinks the offending object lives. And in fact, (13, 13) (the position immediately before the open brace of the empty frame object) is a better answer than (15, 13) (a terrible answer -- past the end of the line containing the closing square brace of the frames array).

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

ghost · 2018-12-22T00:45:04Z

    /// <param name="logicalLocationDictionary">

Update parameter name and text of doc comment. #Closed

Refers to: src/Sarif/IResultLogWriter.cs:37 in 37b9568. [](commit_id = 37b9568, deletion_comment = False)

ghost · 2018-12-22T00:49:09Z

src/Sarif/Autogenerated/LocationEqualityComparer.cs

+            if (left.LogicalLocationIndex != right.LogicalLocationIndex)
+            {
+                return false;
+            }


} [](start = 12, length = 1)

This is relevant to your comment in the PR description: that two locations with different indices in different log files might actually turn out to be the same. As I continue the review I expect to see how we avoid this potentially incorrect generated behavior. #Closed

Someone should explicitly review and confirm this in todo: #1174

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

ghost · 2018-12-22T00:49:44Z

src/Sarif/Autogenerated/LogicalLocation.cs

@@ -51,10 +52,12 @@ public SarifNodeKind SarifNodeKind
        public string DecoratedName { get; set; }

        /// <summary>
-        /// Identifies the key of the immediate parent of the construct in which the result was detected. For example, this property might point to a logical location that represents the namespace that holds a type.
+        /// Identifies the indev of the immediate parent of the construct in which the result was detected. For example, this property might point to a logical location that represents the namespace that holds a type.


indev [](start = 27, length = 5)

"indev" => "index" #Closed

Oh, right, this comes from the schema. I made the comment there.

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

ghost · 2018-12-22T00:50:58Z

src/Sarif/Autogenerated/LogicalLocationEqualityComparer.cs

@@ -43,7 +43,7 @@ public bool Equals(LogicalLocation left, LogicalLocation right)
                return false;
            }

-            if (left.ParentKey != right.ParentKey)
+            if (left.ParentIndex != right.ParentIndex)
            {
                return false;
            }


} [](start = 12, length = 1)

Again, not necessarily correct when comparing logical location objects from different runs. We shall see how this plays out. #Closed

It is critical that result matching travels the complete parent chain. It is also critical that ParentIndex itself is not part of our value type comparison semantics (but it is today!)
#1175
#1176

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

ghost · 2018-12-22T00:53:47Z

src/Sarif/Autogenerated/SarifRewritingVisitor.cs

@@ -756,13 +756,11 @@ public virtual Run VisitRun(Run node)

                if (node.LogicalLocations != null)
                {
-                    var keys = node.LogicalLocations.Keys.ToArray();
-                    foreach (var key in keys)
+                    if (node.LogicalLocations != null)


if (node.LogicalLocations != null) [](start = 20, length = 34)

No need to repeat the test from Line 757. I see that this is a copy of the hand-coded file in the NotYetAutogenerated directory. #Closed

ghost · 2018-12-22T00:57:51Z

src/Sarif/NotYetAutoGenerated/LogicalLocation.cs

+// 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 [](start = 0, length = 5)

If this file too is here just for the sake of applying the DefaultValue attribute, again, you don't need it -- use AttributeHint. #Closed

#1177

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

ghost · 2018-12-22T01:00:42Z

src/Sarif/Baseline/ResultMatching/DataStructures/MatchedResults.cs

@@ -82,6 +84,9 @@ public Result CalculateNewBaselineResult(DictionaryMergeBehavior propertyBagMerg
            {
                ResultMatchingProperties.Add(MatchedResults.MatchResultMetadata_RunKeyName, PreviousResult.OriginalRun.Id.InstanceGuid);
            }
+
+            LogicalLocations = PreviousResult.OriginalRun.LogicalLocations;


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

At first I worried that you were duplicating the entire run's worth of logical location array elements into each matched result, but now I see it's just a reference. #ByDesign

ghost · 2018-12-22T01:02:58Z

src/Sarif/Schemata/sarif-schema.json

-          "description": "Identifies the key of the immediate parent of the construct in which the result was detected. For example, this property might point to a logical location that represents the namespace that holds a type.",
-          "type": "string"
+        "parentIndex": {
+          "description": "Identifies the indev of the immediate parent of the construct in which the result was detected. For example, this property might point to a logical location that represents the namespace that holds a type.",


indev [](start = 41, length = 5)

"indev" => " index" #Closed

ghost · 2018-12-24T18:34:22Z

src/Sarif.FunctionalTests/v2/ConverterTestData/PREfast/Expected-022.xml.sarif

-        "f": {
+      "logicalLocations": [
+        {
+          "fullyQualifiedName": "f",


fullyQualifiedName [](start = 11, length = 18)

FWIW, the name property is required. fullyQualifiedName is required for non-top-level locations, optional for top-level locations (because then it is the same as name). See §3.27.3, §3.27.4.

Rationale:

Before the dictionary-to-array transformation, you always had FQLN available because it was the dictionary key. We require the fQLN property, now, to get parity with that functionality.

Without fQLN being present, a consumer would have to (a) walk the parent tree to get the sequence of components that comprise the FQLN, and (b) know the language-specific syntax for composing the components into the FQLN string.

name is required because without it, a consumer would have to know the language-specific way to parse it out of the FQLN. #Closed

TODO: #1173

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

ghost · 2018-12-24T18:35:58Z

src/Sarif.FunctionalTests/v2/ConverterTestData/PREfast/Expected-022.xml.sarif

          "decoratedName": "?bar@@YAXPAHII@Z"
        },
-        "ShiftTest::f": {
+        {
          "name": "f",


, [](start = 21, length = 1)

So yes, you did the right thing by supplying both name and fQLN for non-top-level locations. It's just that for top-level locations, it's name that's required, and fQLN is optional. #Resolved

ghost · 2018-12-24T18:40:19Z

src/Sarif.FunctionalTests/v2/ConverterTestData/PREfast/Expected-049.xml.sarif

-        "": {
+      "logicalLocations": [
+        {
+          "fullyQualifiedName": "",
          "decoratedName": ""
        },


The spec doesn't prohibit empty names, but... how did PREfast produce this? ("PREfast bug" is a fine answer. But if it's a converter bug, we should file it.) #Closed

TODO. We should look at the logical for all logical location processing things: android studio, fxcop and prefast converters. Note that updating test files for these is tricky. If we change the code logical, new files won't match old. If we universally update all these older test files, we will no longer use the prerelease transformation logic to verify them. And so I suggest we defer these issues until we're ready for that wholesale conversion.

#1171

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

ghost · 2018-12-24T18:45:40Z

src/Sarif.FunctionalTests/v2/ConverterTestData/PREfast/Expected-050.xml.sarif

          "decoratedName": "?f@@YAXH@Z"
        },
-        "f-0": {
-          "name": "f",
+        {
          "fullyQualifiedName": "f",
          "decoratedName": "?f@@YAXPAUS@@@Z"
        }


} [](start = 8, length = 1)

Here we have two fs that differ only by their decorated name due to different signatures. Do we need a readableName property, like f() or f(int, string) to help understand the decoratedName?. Or perhaps a tool that has the information can just use that readable name for name (and then fQLN would be Namespace1::Namespace2::f(int, string) #WontFix

Yes. Presumably, this collision exists simply because someone hasn't sufficiently qualified the 'name' property. These collisions can occur in C++ due to failure to express the presence of some qualifiers (such as 'const') on various components. Same problem: resolves to unique linkable thing, has a distinct decorated name, but a shared undecorated name.

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

ghost · 2018-12-24T18:50:21Z

src/Sarif.FunctionalTests/v2/ConverterTestData/PREfast/Expected-050.xml.sarif

@@ -101,7 +99,7 @@
        {
          "ruleId": "26000",
          "message": {
-            "text": "Overflow using expression 'a[a[0]]'\nBuffer accessed is a\nBuffer is of length 10 elements (4 bytes/element) [size of variable]\nAccessing the element (4 bytes/element) at element offset 10\n\nValues of variables:\na[0] = 10\n\nThere are other instances of this error:\nPotential read overflow using expression 'a[i]' at line 7\nPotential overflow using expression 'a[a[1]]' at line 11\nOverflow using expression 'a[a[0]]' at line 15\nPotential overflow using expression 'a[a[2]]' at line 18\n"
+            "text": "\n      Overflow using expression 'a[a[0]]'\n            Buffer accessed is a\n            Buffer is of length 10 elements (4 bytes/element) [size of variable]\n            Accessing the element (4 bytes/element) at element offset 10\n      \n            Values of variables:\n            a[0] = 10\n      \n            There are other instances of this error:\n            Potential read overflow using expression 'a[i]' at line 7\n            Potential overflow using expression 'a[a[1]]' at line 11\n            Overflow using expression 'a[a[0]]' at line 15\n            Potential overflow using expression 'a[a[2]]' at line 18\n          "


\n [](start = 21, length = 8)

As a result of your reformatting the PREfast XML files, all the messages have this leading white space (and multiline messages have the white space before each line). Is this worth it? What was the rationale for the reformatting? #Closed

I mean, I saw your comment that says "VS did it", but still...

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

The truth is that this reformatting wasn't necessary. I happened to reformat just for readability. On doing so, however, I observed we have this undesired introduction of newlines, etc., that flow from the original file to SARIF. I left these artifacts so that we'd drive code improvements. Tracking item below. I suggest deferring all converter improvements until we get past the files array change (when we'll be ready to do more of a wholesale conversion of prerelease sarif v2 to current). Right now we need all the old content to help validate the ongoing prerelease transformation work.

#1169

In reply to: 243852699 [](ancestors = 243852699,243852678)

ghost · 2018-12-24T19:01:39Z

                    _fileSystem.WriteAllText(fileName, _fileSystem.ReadAllText(inputFilePath));

FWIW we should have IFileSystem.CopyFile that calls through to System.IO.File.Copy, #Pending

Refers to: src/Sarif.Multitool/TransformCommand.cs:83 in 37b9568. [](commit_id = 37b9568, deletion_comment = False)

ghost · 2018-12-24T19:19:24Z

        tools = tools.Distinct().ToList();

FWIW this would suffice:

List<string> tools = runsByToolPrevious.Keys.Union(runsByToolCurrent.Keys).ToList();
``` #Closed

---
Refers to: src/Sarif/Baseline/ResultMatching/SarifLogMatcher.cs:63 in 37b9568. [](commit_id = 37b9568f74eb44d7d86e0f0628f62df7b3fe17d4, deletion_comment = False)

ghost · 2018-12-25T02:02:11Z

src/Sarif.Converters/AndroidStudioConverter.cs

            }

            result.Locations = new List<Location> { location };

            return result;
        }

-        private string CreateFullyQualifiedLogicalName(AndroidStudioProblem problem)
+        private string CreateFullyQualifiedLogicalName(AndroidStudioProblem problem, out int index)


CreateFullyQualifiedLogicalName [](start = 23, length = 31)

This method actually does three things, and although it's hard to come up with a good method name that conveys that [NOTE], it's at least worth a summary comment:

It adds an entry to the LogicalLocations array for each nested level of the logical location where the problem occurs.

It builds up and returns the fully qualified name of the most deeply nested location.

It sets the out parameter index to the index within LogicalLocations of the newly created entry for the most deeply nested location.

[NOTE] Usually a code smell that the method should be refactored, but let that be for another day. #Closed

On further reflection, I think a good method name is AddLogicalLocationAndAncestors. It returns information about the (most deeply nested) logical location.

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

ghost · 2018-12-25T02:09:32Z

src/Sarif.Converters/AndroidStudioConverter.cs

        {
-            if (!String.IsNullOrEmpty(value))
+            fullyQualifiedName = fullyQualifiedName + delimiter + value;


fullyQualifiedName [](start = 33, length = 18)

How does this work on the first call (Line 180), where fullyQualifiedName comes in as null? I could see this working if it was initialized to string.Empty on Line 175. #ByDesign

.NET string.concat happily accepts null and treats these as, effectively, string.empty instances.

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

ghost · 2018-12-25T02:11:19Z

src/Sarif.Converters/AndroidStudioConverter.cs

+            fullyQualifiedName = fullyQualifiedName + delimiter + value;
+
+            // Need to decide which item gets preference, name vs. fully qualified name
+            // if both match. The behaviors of various API in the SDK differs on this point.


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

As noted elsewhere: the spec says that if name and fullyQualifiedName match (that is, if this is a top-level logical location), then name is required (as it always is), and fullyQualifiedLogicalName is optional.

Any API in the SDK that doesn't have that behavior needs to be fixed. #Closed

tracking item created as noted

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

ghost · 2018-12-25T02:16:29Z

src/Sarif/Visitors/SarifCurrentToVersionOneVisitor.cs

@@ -50,7 +50,6 @@ internal AnnotatedCodeLocationVersionOne CreateAnnotatedCodeLocation(Location v2
                {
                    Annotations = v2Location.Annotations?.Select(CreateAnnotation).ToList(),
                    FullyQualifiedLogicalName = v2Location.FullyQualifiedLogicalName,
-                    LogicalLocationKey = v2Location.FullyQualifiedLogicalName,


LogicalLocationKey [](start = 20, length = 18)

Why was this removed?

It's true that if you're going to keep it, you have to use v2Location.FullyQualifiedLogicalName ?? v2Location.Name, because fullyQualifiedLogicalName is optional if it's the same as name. But I'd've expected to see that fix, rather than to see the line removed. #Closed

broken scenario. opened issue, marked 'urgent'

#1180

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

ghost · 2018-12-25T02:17:28Z

src/Sarif/Visitors/SarifCurrentToVersionOneVisitor.cs

                {
-                    location.DecoratedName = _currentV2Run.LogicalLocations[v2Location.FullyQualifiedLogicalName].DecoratedName;
+                    location.DecoratedName = _currentV2Run.LogicalLocations[v2Location.LogicalLocationIndex].DecoratedName;


LogicalLocationIndex [](start = 87, length = 20)

BUG: You have to test that the index isn't -1. #Closed

ghost · 2018-12-25T02:18:05Z

src/Sarif/Visitors/SarifCurrentToVersionOneVisitor.cs

+
+            if (_currentV2Run.LogicalLocations != null && v2LogicalLocation.ParentIndex > -1)
+            {
+                parentKey = _currentV2Run.LogicalLocations[v2LogicalLocation.ParentIndex].FullyQualifiedName;


FullyQualifiedName [](start = 90, length = 18)

BUG: Again, since FQLN is optional if it's the same as name, you need FQLN ?? Name. #Closed

#1181 marked urgent. suggest just creating a simple helper for constructing the key name

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

ghost · 2018-12-25T02:19:55Z

src/Sarif/Visitors/SarifCurrentToVersionOneVisitor.cs

+                logicalLocationsVersionOne = new Dictionary<string, LogicalLocationVersionOne>();
+                foreach (LogicalLocation logicalLocation in logicalLocations)
+                {
+                    logicalLocationsVersionOne[logicalLocation.FullyQualifiedName] = CreateLogicalLocation(logicalLocation);


FullyQualifiedName [](start = 63, length = 18)

Again FQLN ?? Name.

You know, this happens often enough that I suggest we add a partial class for LogicalLocation that defines a property... I don't know what to call it, "SafeName" or something... that returns FQLN ?? Name, and use that in all the places I mentioned. #Closed

I think a helper function probably makes most sense. an extension method off LogicalLocation perhaps. LogicalLocation.GetSarifVersionOneLogicalLocationsDictionaryKey. whew

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

ghost · 2018-12-25T02:21:16Z

src/Sarif/Visitors/SarifCurrentToVersionOneVisitor.cs

@@ -930,11 +950,10 @@ internal StackFrameVersionOne CreateStackFrame(StackFrame v2StackFrame)
                    string fqln = location.FullyQualifiedLogicalName;

                    if (_currentV2Run.LogicalLocations != null &&
-                        _currentV2Run.LogicalLocations.ContainsKey(fqln) &&
-                        !string.IsNullOrWhiteSpace(_currentV2Run.LogicalLocations[fqln].FullyQualifiedName))
+                        !string.IsNullOrWhiteSpace(_currentV2Run.LogicalLocations[location.LogicalLocationIndex].FullyQualifiedName))


LogicalLocationIndex [](start = 91, length = 20)

Test for -1. DRY out code by computing this expression only once: _currentV2Run.LogicalLocations[location.LogicalLocationIndex].FullyQualifiedName (and as usual, it's FQLN ?? Name.

resolve as part of #1181

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

Reactivated for the sake of adding the -1 check (otherwise I'll forget). The rest of the change is covered by #1181.

In reply to: 244054845 [](ancestors = 244054845,243867281)

ghost · 2018-12-25T02:23:06Z

src/Sarif/Visitors/SarifVersionOneToCurrentVisitor.cs

@@ -332,20 +337,22 @@ internal Location CreateLocation(LocationVersionOne v1Location)
                {
                    FullyQualifiedLogicalName = v1Location.LogicalLocationKey ?? v1Location.FullyQualifiedLogicalName,
                    PhysicalLocation = CreatePhysicalLocation(v1Location.ResultFile),
-                    Properties = v1Location.Properties
+                    Properties = v1Location.Properties,


, [](start = 54, length = 1)

Why the added comma? #Closed

ghost · 2018-12-25T02:25:58Z

src/Sarif/Visitors/SarifVersionOneToCurrentVisitor.cs

                };

                if (!string.IsNullOrWhiteSpace(location.FullyQualifiedLogicalName))
                {
-                    if (_currentRun.LogicalLocations?.ContainsKey(location.FullyQualifiedLogicalName) == true)
+                    if (_v1KeyToV2LogicalLocationMap.TryGetValue(location.FullyQualifiedLogicalName, out LogicalLocation logicalLocation))


_v1KeyToV2LogicalLocationMap [](start = 24, length = 28)

I see where this is initialized, on Line 914, but it's hard to convince myself that it's guaranteed to be initialized when we get here. And since this usage and that initialization are separated by so many hundreds of lines, I think it would be really easy for a maintainer to break the invariant that the initialiation happens first.

I suggest you initialize it to an empty dictionary in the ctor, as you do for _logicalLocationToIndexMap. #Closed

Alright, looking over the file as a whole, I see that it's initialized in CreateRun, which happens before any of the subsidiary objects are created. Still, initializing it in the ctor makes it so much easier for the reader to be convinced of its correctness. The micro-optimization of not creating this empty array until/unless it's needed will never pay back the time I spent pondering this ;-)

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

And ok, it's not the ctor, it's VisitSarifLogVersionOne. Still, it's the top-level method, it's right at the top of the file, and it's the place where you initialize _logicalLocationToIndexMap, so I'm going to move it.

In reply to: 243867847 [](ancestors = 243867847,243867544)

ghost · 2018-12-25T02:36:09Z

src/Sarif/Visitors/SarifVersionOneToCurrentVisitor.cs

@@ -332,20 +337,22 @@ internal Location CreateLocation(LocationVersionOne v1Location)
                {
                    FullyQualifiedLogicalName = v1Location.LogicalLocationKey ?? v1Location.FullyQualifiedLogicalName,


v1Location.LogicalLocationKey [](start = 48, length = 29)

EXISTING BUG: this logic is backwards. It should be:

FullyQualifiedLogicalName = v1Location.FullyQualifiedLogicalName ?? v1Location.LogicalLocationKey,

v1 FQLN is guaranteed to be the right answer if it's there; if it's absent, it's because it matched the key. #Closed

#1182 marked urgent

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

ghost · 2018-12-25T02:40:57Z

src/Sarif/Visitors/SarifVersionOneToCurrentVisitor.cs

                };

                if (!string.IsNullOrWhiteSpace(location.FullyQualifiedLogicalName))
                {
-                    if (_currentRun.LogicalLocations?.ContainsKey(location.FullyQualifiedLogicalName) == true)
+                    if (_v1KeyToV2LogicalLocationMap.TryGetValue(location.FullyQualifiedLogicalName, out LogicalLocation logicalLocation))


FullyQualifiedLogicalName [](start = 74, length = 25)

This is not the right lookup key. In the event of a collision, you'd have to do the lookup by means of the v1 key. So you need to do:

string v1Key = v1Location.LogicalLocationKey ?? v1Location.FullyQualifiedLogicalName;

... and pass v1Key to TryGetValue.

The value of v1Key is the string that was incorrectly used as the value of v2 FQLN on Line 338. Turns out we need that string after all, but for something else :-) #Closed

#1182

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

ghost · 2018-12-25T02:42:26Z

src/Sarif/Visitors/SarifVersionOneToCurrentVisitor.cs

+                        if (!string.IsNullOrEmpty(logicalLocation.DecoratedName))
+                        {
+                            logicalLocation.DecoratedName = v1Location.DecoratedName;
+                            _logicalLocationToIndexMap[logicalLocation] = index;


index [](start = 74, length = 5)

I don't get why the population of _logicalLocationToIndexMap depends on whether there's a decorated name. If this code is correct, it needs a comment. #Closed

something looks odd here. also note that when we can't retrieve the logical location, we're setting the index to 0 which is wrong.

#1183

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

ghost · 2018-12-25T02:44:04Z

                FullyQualifiedLogicalName = v1AnnotatedCodeLocation.LogicalLocationKey ?? v1AnnotatedCodeLocation.FullyQualifiedLogicalName,

The same issue as above with the role reversal between the key and the FQLN, here and starting on Line 383.

Refers to: src/Sarif/Visitors/SarifVersionOneToCurrentVisitor.cs:377 in 37b9568. [](commit_id = 37b9568, deletion_comment = False)

ghost · 2018-12-25T02:44:33Z

src/Sarif/Visitors/SarifVersionOneToCurrentVisitor.cs

-            else if (!string.IsNullOrWhiteSpace(fullyQualifiedLogicalName))
+            logicalLocation.FullyQualifiedName = fullyQualifiedLogicalName;
+            logicalLocation.Name = GetLogicalLocationName(fullyQualifiedLogicalName);
+            location.FullyQualifiedLogicalName = logicalLocationKey;


logicalLocationKey [](start = 49, length = 18)

Again, the key/FQLN reversal. #Closed

covered elsewhere

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

ghost · 2018-12-25T02:50:49Z

src/Sarif/Visitors/SarifVersionOneToCurrentVisitor.cs

+        {
+            // We saw and populated this one previously, because it was a parent to 
+            // a logical location that we encountered earlier
+            if (populatedKeys.Contains(fullyQualifiedName)) { return; }


fullyQualifiedName [](start = 39, length = 18)

Given how you called this method on Line 920, it's clear that the thing you're calling FQLN here is actually the v1 dictionary key. So, given the massive potential for confusion between the key and the FQLN, manifested several places above, it's important to name this v1Key, and make sure not to use it in a place where you really do need the FQLN.

ghost

🕐

ghost · 2018-12-25T02:55:10Z

Although I had a great many comments, it's not as bad as it seems.

Many of the comments are nits. There are also some bugs, but for the most part they are simple point fixes that I wouldn't hold up the approval for.

The reason I'm marking this PR "Code not Ready" is because of a pervasive problem in SarifVersionOneToCurrentVisitor.cs. There are many places in that code where the v1 dictionary key is used in a place where the FQLN should be used, and vice versa. There was already at least one existing bug of that kind, which I commented on. But the new code contains several other instances of this problem. That file needs to be looked at carefully before we merge (and we are clearly missing some test coverage around converting v1 files with logical location key collisions. #Resolved

ghost

🕐

michaelcfanning · 2018-12-26T19:16:37Z

Really appreciate the review! Perhaps we can take the small changes now, make note of additional changes and check-in. I will work on the files array conversion and perhaps you can clean up the VersionOneToCurrent issues?

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

ghost · 2018-12-26T19:26:32Z

Yes, we can do that. In email just now I suggested a slightly different approach: I finish up the small changes (almost done now), then you look at the now-much-smaller list of substantive, Active issues to decide what you want to address before the merge.

Now having said all that: the FQLN/key confusion that I referred to only occurs in this famous corner case we always talk about (namespace name/type name ambiguity) and I'm willing to concede that it doesn't occur in the wild. We could merge with just the small changes fixed.

So ok, let's do it your way. I'll make the remaining small changes, merge, and then work on the substantive changes separately. But I would like you to go through the remaining Active issue list (once I'm done with the small stuff) so I don't waste time making changes you disagree with.

In reply to: 450011808 [](ancestors = 450011808,449791980)

ghost

michaelcfanning · 2018-12-26T20:51:26Z

Got it, proceeding as you say.

In reply to: 450013084 [](ancestors = 450013084,450011808,449791980)

@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 7 commits December 14, 2018 09:46

Convert logical locations to arrays. Incomplete.

7880506

Bulk of logical array conversion

69b759f

Selectively update some PREfast test files that were mistransformed e…

a71bc38

…arlier (due to converter bugs)

Revert converter change that compromises diff with baselines.

2cb8831

Final converter updates to keep current behavior consistent with chec…

08217c0

…ked in baselines.

Test updates.

6465a7c

Remove debugging files.

37b9568

michaelcfanning commented Dec 21, 2018

View reviewed changes

ghost reviewed Dec 22, 2018

View reviewed changes

ghost reviewed Dec 24, 2018

View reviewed changes

ghost reviewed Dec 25, 2018

View reviewed changes

ghost suggested changes Dec 25, 2018

View reviewed changes

Larry Golding added 3 commits December 26, 2018 10:31

PR feedback, batch #1

9872fe5

PR feedback, batch #2

ebb6bbb

PR feedback, batch #3

99b526f

PR feedback, batch #4

3f8793d

ghost approved these changes Dec 26, 2018

View reviewed changes

ghost merged commit 12cbea4 into develop Dec 26, 2018

michaelcfanning deleted the dictionaries-to-arrays branch December 30, 2018 21:08

This pull request was closed.

		@@ -332,20 +337,22 @@ internal Location CreateLocation(LocationVersionOne v1Location)
		{
		FullyQualifiedLogicalName = v1Location.LogicalLocationKey ?? v1Location.FullyQualifiedLogicalName,

Logical locations dictionaries to arrays #1170

Logical locations dictionaries to arrays #1170

Conversation

michaelcfanning commented Dec 21, 2018 • edited Loading

michaelcfanning Dec 21, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

michaelcfanning Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Dec 21, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

michaelcfanning Dec 21, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

michaelcfanning Dec 21, 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 21, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Dec 22, 2018 • edited by ghost Loading

ghost Dec 22, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 22, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 22, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 22, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Dec 22, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 22, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Dec 22, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Dec 24, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 24, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Dec 24, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 24, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 24, 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 commented Dec 24, 2018 • edited by ghost Loading

ghost commented Dec 24, 2018 • edited by ghost Loading

ghost Dec 25, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 25, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 25, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 25, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 25, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Dec 25, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning commented Dec 21, 2018 •

edited

Loading

michaelcfanning Dec 21, 2018 •

edited by ghost

Loading

michaelcfanning Dec 21, 2018 •

edited

Loading

michaelcfanning Dec 21, 2018 •

edited

Loading

michaelcfanning Dec 21, 2018 •

edited by ghost

Loading

michaelcfanning Dec 21, 2018 •

edited

Loading

michaelcfanning Dec 21, 2018 •

edited by ghost

Loading

michaelcfanning Dec 21, 2018 •

edited by ghost

Loading

michaelcfanning Dec 21, 2018 •

edited by ghost

Loading

ghost commented Dec 22, 2018 •

edited by ghost

Loading

ghost Dec 22, 2018 •

edited by michaelcfanning

Loading

ghost Dec 22, 2018 •

edited by ghost

Loading

ghost Dec 22, 2018 •

edited by michaelcfanning

Loading

ghost Dec 22, 2018 •

edited by ghost

Loading

ghost Dec 22, 2018 •

edited by michaelcfanning

Loading

ghost Dec 22, 2018 •

edited by ghost

Loading

ghost Dec 22, 2018 •

edited by ghost

Loading

ghost Dec 24, 2018 •

edited by michaelcfanning

Loading

ghost Dec 24, 2018 •

edited by ghost

Loading

ghost Dec 24, 2018 •

edited by michaelcfanning

Loading

ghost Dec 24, 2018 •

edited by michaelcfanning

Loading

ghost Dec 24, 2018 •

edited by michaelcfanning

Loading

ghost commented Dec 24, 2018 •

edited by ghost

Loading

ghost commented Dec 24, 2018 •

edited by ghost

Loading

ghost Dec 25, 2018 •

edited by ghost

Loading

ghost Dec 25, 2018 •

edited by michaelcfanning

Loading

ghost Dec 25, 2018 •

edited by michaelcfanning

Loading

ghost Dec 25, 2018 •

edited by michaelcfanning

Loading

ghost Dec 25, 2018 •

edited by ghost

Loading

ghost Dec 25, 2018 •

edited by michaelcfanning

Loading

ghost Dec 25, 2018 •

edited by michaelcfanning

Loading

ghost Dec 25, 2018 •

edited by ghost

Loading

ghost Dec 25, 2018 •

edited by ghost

Loading

ghost Dec 25, 2018 •

edited by ghost

Loading

ghost Dec 25, 2018 •

edited by ghost

Loading

ghost Dec 25, 2018 •

edited by michaelcfanning

Loading

ghost Dec 25, 2018 •

edited by michaelcfanning

Loading

ghost Dec 25, 2018 •

edited by michaelcfanning

Loading

ghost Dec 25, 2018 •

edited by michaelcfanning

Loading

ghost commented Dec 25, 2018 •

edited by michaelcfanning

Loading

ghost commented Dec 26, 2018 •

edited by ghost

Loading