Skip to content

Commit

Permalink
Fix #1673 (property bag testing); Fix #1687 (NuGet packaging warning) (…
Browse files Browse the repository at this point in the history 
…#1688)

I added a comprehensive end-to-end test of round-tripping property bag properties. I also added a `DateTime` test to the `PropertyBagConverterTests` functional tests.

The problem that @ScottLouvau ran into with the `DateTime` test is related to my fix for #1577. I don't mean that my fix caused his problem; I mean that to avoid the problem, we need to do the same thing I did to fix #1577. For an explanation, see [this comment](https://github.com/microsoft/sarif-sdk/blob/83ecf0fa2d919a0be70a144464fc25d8b898a294/src/Sarif/SarifUtilities.cs#L190-L214) in SarifUtilities.cs.

In short, when Newtonsoft.Json deserializes a string-valued property that looks to it like a `DateTime`, its default behavior is to convert it to a `DateTime` object. That behavior breaks our property round-tripping. To avoid it, we need to call `JsonConvert.DeserializeObject` with a `JsonSerializerSettings` object that specifies `DateParseHandling.None`. That tells Newtonsoft.Json to leave string-valued properties alone and let us handle the `DateTime`-iness. In #1577, I introduced `SarifUtilities.DeserializeObject` to do exactly that.

@michaelcfanning and I have discussed this and we know it's not the end of the story. It's too easy to forget (or not to know about) the need to call the `SarifUtilities` method, which is exactly what happened in that existing unit test. But as far as verifying that our handling of property bag properties is good, I believe we now have sufficient test coverage to be confident of that.

Also:
- Fix a NuGet packaging warning.
- Improve our internal handling of property bag properties with `null` values.
  • Loading branch information
Larry Golding authored Sep 30, 2019
1 parent 83ecf0f commit 68182f0
Show file tree
Hide file tree
Showing 8 changed files with 259 additions and 70 deletions.
44 changes: 24 additions & 20 deletions src/ReleaseHistory.md
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# SARIF Package Release History (SDK, Driver, Converters, and Multitool)

## **v2.1.18** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.1.18) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.1.18) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.1.18) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.1.18)
* FEATURE: Enhanced property bag serialization unit testing. [#1673](https://github.com/microsoft/sarif-sdk/issues/1673)
* BUGFIX: Fix packaging warning NU5048 during build. [#1687](https://github.com/microsoft/sarif-sdk/issues/1687)

## **v2.1.17** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.1.17) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.1.17) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.1.17) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.1.17)
* API NON-BREAKING: emit all core object model members as 'virtual'.
* FEATURE: Introduce SarifConsolidator to shrink large log files. [#1675](https://github.com/microsoft/sarif-sdk/issues/1675)
Expand Down Expand Up @@ -106,16 +110,16 @@
* API BREAKING: Improve `address` object design. [oasis-tcs/sarif-spec#401](https://github.com/oasis-tcs/sarif-spec/issues/401)

## **v2.1.0-beta.2** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.1.0-beta.2) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.1.0-beta.2) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.1.0-beta.2)) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.1.0-beta.2)
* API NON-BREAKING: Change `request.target` type to string. https://github.com/oasis-tcs/sarif-spec/issues/362
* API BREAKING: anyOf `physicalLocation.artifactLocation` and `physicalLocation.address` is required. https://github.com/oasis-tcs/sarif-spec/issues/353
* API NON-BREAKING: Change `request.target` type to string. [oasis-tcs/sarif-spec#362](https://github.com/oasis-tcs/sarif-spec/issues/362)
* API BREAKING: anyOf `physicalLocation.artifactLocation` and `physicalLocation.address` is required. [oasis-tcs/sarif-spec#353](https://github.com/oasis-tcs/sarif-spec/issues/353)
* API BREAKING: Rename `run.defaultFileEncoding` to `run.defaultEncoding`.
* API NON-BREAKING: Add `threadFlowLocation.taxa`. https://github.com/oasis-tcs/sarif-spec/issues/381
* API NON-BREAKING: Add `threadFlowLocation.taxa`. [oasis-tcs/sarif-spec#381](https://github.com/oasis-tcs/sarif-spec/issues/381)
* API BREAKING: anyOf `message.id` and `message.text` is required.
* API NON-BREAKING: Add `request.noResponseReceived` and `request.failureReason`. https://github.com/oasis-tcs/sarif-spec/issues/378
* API NON-BREAKING: Add `request.noResponseReceived` and `request.failureReason`. [oasis-tcs/sarif-spec#378](https://github.com/oasis-tcs/sarif-spec/issues/378)
* API BREAKING: anyOf `externalPropertyFileReference.guid` and `externalPropertyFileReference.location` is required.
* API BREAKING: `artifact.length` should have `default: -1, minimum: -1` values.
* API BREAKING: Rename `fix.changes` to `fix.artifactChanges`.
* API BREAKING: Each redaction token in an originalUriBaseId represents a unique location. https://github.com/oasis-tcs/sarif-spec/issues/377
* API BREAKING: Each redaction token in an originalUriBaseId represents a unique location. [oasis-tcs/sarif-spec#377](https://github.com/oasis-tcs/sarif-spec/issues/377)
* API BREAKING: Rename file related enums in `artifact.roles`.
* API BREAKING: anyOf `artifactLocation.uri` and `artifactLocation.index` is required.
* API BREAKING: `multiformatMessageString.text` is required.
Expand All @@ -136,17 +140,17 @@
* API BREAKING: `request.index` should have default: -1, minimum: -1.
* API BREAKING: `response.index` should have default: -1, minimum: -1.
* API NON-BREAKING: `externalProperties.version` is not a required property if it is not root element.
* API NON-BREAKING: Add artifact roles for configuration files. https://github.com/oasis-tcs/sarif-spec/issues/372
* API NON-BREAKING: Add suppression.justification. https://github.com/oasis-tcs/sarif-spec/issues/373
* API NON-BREAKING: Associate descriptor metadata with thread flow locations. https://github.com/oasis-tcs/sarif-spec/issues/381
* API BREAKING: Move `location.physicalLocation.id` to `location.id`. https://github.com/oasis-tcs/sarif-spec/issues/375
* API NON-BREAKING: Add artifact roles for configuration files. [oasis-tcs/sarif-spec#372](https://github.com/oasis-tcs/sarif-spec/issues/372)
* API NON-BREAKING: Add suppression.justification. [oasis-tcs/sarif-spec#373](https://github.com/oasis-tcs/sarif-spec/issues/373)
* API NON-BREAKING: Associate descriptor metadata with thread flow locations. [oasis-tcs/sarif-spec#381](https://github.com/oasis-tcs/sarif-spec/issues/381)
* API BREAKING: Move `location.physicalLocation.id` to `location.id`. [oasis-tcs/sarif-spec#375](https://github.com/oasis-tcs/sarif-spec/issues/375)
* API BREAKING: `result.stacks` array should have unique items.
* API BREAKING: `result.relatedLocations` array should have unique items.
* API BREAKING: Separate `suppression` `status` from `kind`. https://github.com/oasis-tcs/sarif-spec/issues/371
* API BREAKING: Separate `suppression` `status` from `kind`. [oasis-tcs/sarif-spec#371](https://github.com/oasis-tcs/sarif-spec/issues/371)
* API BREAKING: `reportingDescriptorReference` requires anyOf (`index`, `guid`, `id`).
* API BREAKING: Rename `request` object and related properties to `webRequest`.
* API BREAKING: Rename `response` object and related properties to `webResponse`.
* API NON-BREAKING: Add `locationRelationship` object. https://github.com/oasis-tcs/sarif-spec/issues/375
* API NON-BREAKING: Add `locationRelationship` object. [oasis-tcs/sarif-spec#375](https://github.com/oasis-tcs/sarif-spec/issues/375)
* API BREAKING: `externalPropertyFileReference.itemCount` can be 0 and defaults to minimum: -1, default: -1.
* API BREAKING: `threadFlowLocation.executionOrder` can be 0 and defaults to -1, so minimum: -1, default: -1
* API BREAKING: Rename artifact role `traceFile` to `tracedFile`.
Expand All @@ -161,21 +165,21 @@
* API BREAKING: Change `notification.physicalLocation` of type `physicalLocation` to `notification.locations` of type `locations`.

## **v2.1.0-beta.1** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.1.0-beta.1) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.1.0-beta.1) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.1.0-beta.1)) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.1.0-beta.1))
* API BREAKING: Change `request.uri` to `request.target`. https://github.com/oasis-tcs/sarif-spec/issues/362
* API BREAKING: Change `request.uri` to `request.target`. [oasis-tcs/sarif-spec#362](https://github.com/oasis-tcs/sarif-spec/issues/362)

## **v2.1.0-beta.0** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.1.0-beta.0) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.1.0-beta.0) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.1.0-beta.0)) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.1.0-beta.0))
* API BREAKING: All SARIF state dictionaries now contains multiformat strings as values. https://github.com/oasis-tcs/sarif-spec/issues/361
* API NON-BREAKING: Define `request` and `response` objects. https://github.com/oasis-tcs/sarif-spec/issues/362
* API BREAKING: All SARIF state dictionaries now contains multiformat strings as values. [oasis-tcs/sarif-spec#361](https://github.com/oasis-tcs/sarif-spec/issues/361)
* API NON-BREAKING: Define `request` and `response` objects. [oasis-tcs/sarif-spec#362](https://github.com/oasis-tcs/sarif-spec/issues/362)

## **v2.0.0-csd.2.beta.2019.04-03.3** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.0.0-csd.2.beta.2019.04-03.3) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.0.0-csd.2.beta.2019.04-03.3) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.0.0-csd.2.beta.2019.04-03.3)) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.0.0-csd.2.beta.2019.04-03.3))
* API BREAKING: Rename `reportingDescriptor.descriptor` to `reportingDescriptor.target`. https://github.com/oasis-tcs/sarif-spec/issues/356
* API NON-BREAKING: Remove `canPrecedeOrFollow` from relationship kind list. https://github.com/oasis-tcs/sarif-spec/issues/356
* API BREAKING: Rename `reportingDescriptor.descriptor` to `reportingDescriptor.target`. [oasis-tcs/sarif-spec#356](https://github.com/oasis-tcs/sarif-spec/issues/356)
* API NON-BREAKING: Remove `canPrecedeOrFollow` from relationship kind list. [oasis-tcs/sarif-spec#356](https://github.com/oasis-tcs/sarif-spec/issues/356)

## **v2.0.0-csd.2.beta.2019.04-03.2** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.0.0-csd.2.beta.2019.04-03.2) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.0.0-csd.2.beta.2019.04-03.2) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.0.0-csd.2.beta.2019.04-03.2)) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.0.0-csd.2.beta.2019.04-03.2))
* API NON-BREAKING: Add `module` to `address.kind`. https://github.com/oasis-tcs/sarif-spec/issues/353
* API BREAKING: `address.baseAddress` & `address.offset` to int. https://github.com/oasis-tcs/sarif-spec/issues/353
* API BREAKING: Update how reporting descriptors describe their taxonomic relationships. https://github.com/oasis-tcs/sarif-spec/issues/356
* API NON-BREAKING: Add `initialState` and `immutableState` properties to thread flow object. Add `immutableState` to `graphTraversal` object. https://github.com/oasis-tcs/sarif-spec/issues/168
* API NON-BREAKING: Add `module` to `address.kind`. [oasis-tcs/sarif-spec#353](https://github.com/oasis-tcs/sarif-spec/issues/353)
* API BREAKING: `address.baseAddress` & `address.offset` to int. [oasis-tcs/sarif-spec#353](https://github.com/oasis-tcs/sarif-spec/issues/353)
* API BREAKING: Update how reporting descriptors describe their taxonomic relationships. [oasis-tcs/sarif-spec#356](https://github.com/oasis-tcs/sarif-spec/issues/356)
* API NON-BREAKING: Add `initialState` and `immutableState` properties to thread flow object. Add `immutableState` to `graphTraversal` object. [oasis-tcs/sarif-spec#168](https://github.com/oasis-tcs/sarif-spec/issues/168)

## **v2.0.0-csd.2.beta.2019.04-03.1** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.0.0-csd.2.beta.2019.04-03.1) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.0.0-csd.2.beta.2019.04-03.1) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.0.0-csd.2.beta.2019.04-03.1)) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.0.0-csd.2.beta.2019.04-03.1))
* API BREAKING: Rename `message.messageId` property to `message.id`. https://github.com/oasis-tcs/sarif-spec/issues/352
Expand Down
22 changes: 12 additions & 10 deletions src/Sarif.Driver/Sdk/AnalyzeCommandBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -273,8 +273,7 @@ private HashSet<string> CreateTargetsSet(TOptions analyzeOptions)
{
string normalizedSpecifier = specifier;

Uri uri;
if (Uri.TryCreate(specifier, UriKind.RelativeOrAbsolute, out uri))
if (Uri.TryCreate(specifier, UriKind.RelativeOrAbsolute, out Uri uri))
{
if (uri.IsAbsoluteUri && (uri.IsFile || uri.IsUnc))
{
Expand Down Expand Up @@ -305,9 +304,11 @@ protected virtual TContext CreateContext(
RuntimeConditions runtimeErrors,
string filePath = null)
{
var context = new TContext();
context.Logger = logger;
context.RuntimeErrors = runtimeErrors;
var context = new TContext
{
Logger = logger,
RuntimeErrors = runtimeErrors
};

if (filePath != null)
{
Expand Down Expand Up @@ -449,11 +450,12 @@ private void InitializeOutputFile(TOptions analyzeOptions, TContext context, Has

private IEnumerable<string> GenerateSensitiveTokensList()
{
var result = new List<String>();

result.Add(Environment.MachineName);
result.Add(Environment.UserName);
result.Add(Environment.UserDomainName);
var result = new List<string>
{
Environment.MachineName,
Environment.UserName,
Environment.UserDomainName
};

string userDnsDomain = Environment.GetEnvironmentVariable("USERDNSDOMAIN");
string logonServer = Environment.GetEnvironmentVariable("LOGONSERVER");
Expand Down
36 changes: 17 additions & 19 deletions src/Sarif/Core/PropertyBagHolder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ namespace Microsoft.CodeAnalysis.Sarif
/// </summary>
public class PropertyBagHolder : IPropertyBagHolder
{
private const string NullValue = "null";

protected PropertyBagHolder()
{
Tags = new TagsCollection(this);
Expand Down Expand Up @@ -63,17 +61,17 @@ public string GetProperty(string propertyName)
propertyName));
}

if (Properties[propertyName] == null) { return null; }

if (!Properties[propertyName].IsString)
{
throw new InvalidOperationException(SdkResources.CallGenericGetProperty);
}

string value = Properties[propertyName].SerializedValue;

// Remove the quotes around the serialized value ("x" => x) -- unless it's null.
return value.Equals(NullValue, StringComparison.Ordinal)
? null
: value.Substring(1, value.Length - 2);
// Remove the quotes around the serialized value ("x" => x).
return value.Substring(1, value.Length - 2);
}

public bool TryGetProperty<T>(string propertyName, out T value)
Expand All @@ -84,7 +82,7 @@ public bool TryGetProperty<T>(string propertyName, out T value)
return true;
}

value = default(T);
value = default;
return false;
}

Expand Down Expand Up @@ -155,20 +153,23 @@ public string GetSerializedPropertyValue(string propertyName)

public void SetProperty<T>(string propertyName, T value)
{
if (Properties == null)
{
Properties = new Dictionary<string, SerializedPropertyInfo>();
}
Properties = Properties ?? new Dictionary<string, SerializedPropertyInfo>();

bool isString = typeof(T) == typeof(string);

string serializedValue;
if (value == null)
{
serializedValue = NullValue;
// This is consistent with what the PropertyBagConverter does when it encounters
// a null-valued property. Whether we create a property bag dictionary entry
// by deserializing a null from the log file or by calling SetProperty("aProp", null),
// the internal representation is the same: a null value in the Properties
// dictionary.
Properties[propertyName] = null;
}
else
{
string serializedValue;

if (isString)
{
serializedValue = JsonConvert.ToString(value);
Expand All @@ -189,9 +190,9 @@ public void SetProperty<T>(string propertyName, T value)

serializedValue = JsonConvert.SerializeObject(value, settings);
}
}

Properties[propertyName] = new SerializedPropertyInfo(serializedValue, isString);
Properties[propertyName] = new SerializedPropertyInfo(serializedValue, isString);
}
}

public void SetPropertiesFrom(IPropertyBagHolder other)
Expand All @@ -217,10 +218,7 @@ public void SetPropertiesFrom(IPropertyBagHolder other)

public void RemoveProperty(string propertyName)
{
if (Properties != null)
{
Properties.Remove(propertyName);
}
Properties?.Remove(propertyName);
}

[JsonIgnore]
Expand Down
47 changes: 29 additions & 18 deletions src/Test.FunctionalTests.Sarif/PropertyBagConverterTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
using System;
using System.Collections.Generic;
using FluentAssertions;
using Microsoft.CodeAnalysis.Sarif;
using Newtonsoft.Json;
using Xunit;

Expand All @@ -28,34 +27,38 @@ public void PropertyBagConverter_RoundTrip()
string longPropertyName = "hugeFileSizeBytes";
long longPropertyValue = (long)10 * int.MaxValue;

var run = new Run();
string dateTimePropertyName = nameof(dateTimePropertyName);
DateTime dateTimePropertyValue = new DateTime(2019, 9, 27, 13, 52, 0);

var run = new Run
{
Tool = new Tool
{
Driver = new ToolComponent
{
Name = "CodeScanner"
}
}
};

run.SetProperty(intPropertyName, 42);
run.SetProperty(stringPropertyName, stringPropertyValue);
run.SetProperty(normalStringPropertyName, normalStringPropertyValue);
run.SetProperty<long>(longPropertyName, longPropertyValue);
run.SetProperty(longPropertyName, longPropertyValue);
run.SetProperty(dateTimePropertyName, dateTimePropertyValue);

var originalLog = new SarifLog
{
Runs = new List<Run>
{
new Run
{
Tool = new Tool
{
Driver = new ToolComponent
{
Name = "CodeScanner"
}
},
Properties = run.Properties
}
run
}
};

var settings = new JsonSerializerSettings { Formatting = Formatting.Indented };
string originalLogText = JsonConvert.SerializeObject(originalLog, settings);
string originalLogContents = JsonConvert.SerializeObject(originalLog, settings);

SarifLog deserializedLog = JsonConvert.DeserializeObject<SarifLog>(originalLogText);
SarifLog deserializedLog = SarifUtilities.DeserializeObject<SarifLog>(originalLogContents);
run = deserializedLog.Runs[0];

int integerProperty = run.GetProperty<int>(intPropertyName);
Expand All @@ -70,9 +73,17 @@ public void PropertyBagConverter_RoundTrip()

run.GetProperty<long>(longPropertyName).Should().Be(longPropertyValue);

string reserializedLog = JsonConvert.SerializeObject(deserializedLog, settings);
run.GetProperty<DateTime>(dateTimePropertyName).Should().Be(dateTimePropertyValue);

// In addition to checking the individual properties (which is nice for explicitness),
// we redundantly check that the entire SarifLog objects match.
// THIS ASSERTION IS COMMENTED OUT BECAUSE OF https://github.com/microsoft/sarif-sdk/issues/1689,
// "Generated code for property bag comparison is incorrect."
//originalLog.ValueEquals(deserializedLog).Should().BeTrue();

string reserializedLogContents = JsonConvert.SerializeObject(deserializedLog, settings);

reserializedLog.Should().Be(originalLogText);
reserializedLogContents.Should().Be(originalLogContents);
}
}
}
Loading

0 comments on commit 68182f0

Please sign in to comment.