Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Emit column kind default explicitly for Windows SDK SARIF emit. #1160

Merged
merged 5 commits into from
Dec 7, 2018
Merged

Emit column kind default explicitly for Windows SDK SARIF emit. #1160

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c2ceb47
Emit column kind default explicitly for Windows SDK SARIF emit.
michaelcfanning Dec 7, 2018
b5e1797
Update release notes
michaelcfanning Dec 7, 2018
d0ea69d
More column kind test fixes
michaelcfanning Dec 7, 2018
3d50d89
Change behavior to always serialize column kind.
michaelcfanning Dec 7, 2018
83df6bd
Always serialize column kind
michaelcfanning Dec 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Sarif.FunctionalTests/v2/SpecExamples/Comprehensive.sarif
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"baselineInstanceGuid": "0A106451-C9B1-4309-A7EE-06988B95F723",
"automationLogicalId": "Build-14.0.1.2-Release-20160716-13:22:18",
"architecture": "x86",
"columnKind": "utf16CodeUnits",
"tool": {
"name": "CodeScanner",
"fullName": "CodeScanner 1.1 for Unix (en-US)",
Expand Down
51 changes: 51 additions & 0 deletions src/Sarif.UnitTests/Core/RunTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// 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 Xunit;
using Newtonsoft.Json;
using FluentAssertions;

namespace Microsoft.CodeAnalysis.Sarif
{
public class RunTests
{
[Fact]
public void Run_ColumnKindSerializesProperly()
{
// In our Windows-specific SDK, if no one has explicitly set ColumnKind, we
// will set it to the windows-specific value of Utf16CodeUnits. Otherwise,
Copy link

Choose a reason for hiding this comment

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

w [](start = 34, length = 1)

'w' => 'W'

// the SARIF file will pick up the ColumnKind default value of
// UnicodeCodePoints, which is not appropriate for Windows frameworks.
RoundTripColumnKind(persistedValue: ColumnKind.None, expectedRoundTrippedValue: ColumnKind.Utf16CodeUnits);

// When explicitly set, we should always preserve that setting
RoundTripColumnKind(persistedValue: ColumnKind.Utf16CodeUnits, expectedRoundTrippedValue: ColumnKind.Utf16CodeUnits);
RoundTripColumnKind(persistedValue: ColumnKind.UnicodeCodePoints, expectedRoundTrippedValue: ColumnKind.UnicodeCodePoints);

}

private void RoundTripColumnKind(ColumnKind persistedValue, ColumnKind expectedRoundTrippedValue)
{
var sarifLog = new SarifLog
{
Runs = new[]
{
new Run
{
Tool = new Tool { Name = "Test tool"},
ColumnKind = persistedValue
}
}
};

string json = JsonConvert.SerializeObject(sarifLog);

// We should never see the default value persisted to JSON
json.Contains("unicodeCodePoints").Should().BeFalse();

sarifLog = JsonConvert.DeserializeObject<SarifLog>(json);
sarifLog.Runs[0].ColumnKind.Should().Be(expectedRoundTrippedValue);
}
}
}
4 changes: 4 additions & 0 deletions src/Sarif/Autogenerated/Run.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Collections.Generic;
using System.Runtime.Serialization;
using Microsoft.CodeAnalysis.Sarif.Readers;
using Newtonsoft.Json;

namespace Microsoft.CodeAnalysis.Sarif
{
Expand Down Expand Up @@ -138,6 +139,8 @@ public SarifNodeKind SarifNodeKind
/// Specifies the unit in which the tool measures columns.
/// </summary>
[DataMember(Name = "columnKind", IsRequired = false, EmitDefaultValue = false)]
[JsonConverter(typeof(EnumConverter))]
[System.ComponentModel.DefaultValue(ColumnKind.UnicodeCodePoints)]
Copy link
Member Author

Choose a reason for hiding this comment

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

Jschema doesn't emit default values currently. We'll have to manually maintain them for now. microsoft/jschema#57

public ColumnKind ColumnKind { get; set; }

/// <summary>
Expand All @@ -151,6 +154,7 @@ public SarifNodeKind SarifNodeKind
/// </summary>
public Run()
{
this.ColumnKind = ColumnKind.UnicodeCodePoints;
Copy link
Member Author

Choose a reason for hiding this comment

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

this.ColumnKind = ColumnKind.UnicodeCodePoints [](start = 12, length = 46)

ok, this is an important apparent JSON.NET gap. in JSON.NET, you can provide an argument in a JSON convert settings object that specifies that default values should populate if missing on deserialization. the problem with that is that you need to explicitly create and pass the appropriate settings object (and we've attempted to make everything work with a single API call with no parameterization of this kind).

to work around the problem, we provide a programmatic equivalent of populate by setting the default value in the ctor. when JSON.NET itself is deserialization, it will overwrite this property in cases where the JSON has an explicit value. a fix to jschema #57 should provide this fix as well (and not just the default value attribute that elides property values from persisted JSON if they map to the default).

Copy link

Choose a reason for hiding this comment

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

I assume you are talking about JsonSerializerSettings.DefaultValueHandling, whose Include value means that "The Json.NET deserializer will continue setting a field/property if the JSON value is the same as the default value."

But it goes on to say "DefaultValueHandling can also be customized on individual properties with JsonPropertyAttribute." So I think we can fix this by emitting

[JsonProperty DefaultValueHandling = DefaultValueHandling.Include)]

on every property.

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

Copy link

Choose a reason for hiding this comment

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

I added that to https://github.com/Microsoft/jchema/issues/57.

In reply to: 239910615 [](ancestors = 239910615,239879240)

}

/// <summary>
Expand Down
9 changes: 8 additions & 1 deletion src/Sarif/CodeGenHints.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -656,6 +656,14 @@
"unicodeCodePoints"
]
}
},
{
"kind": "AttributeHint",
Copy link
Member Author

@michaelcfanning michaelcfanning Dec 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

AttributeHint [](start = 15, length = 13)

Oops. test gap closed here by adding columnKind explicitly to our comprehensive test files. I manually reviewed the hints file to make sure we don't have this same mistake elsewhere.

"arguments": {
"namespaceName": "Newtonsoft.Json",
"typeName": "JsonConverter",
"arguments": [ "typeof(EnumConverter)" ]
}
}
],
"Run.Files": [
Expand Down Expand Up @@ -738,7 +746,6 @@
"arguments": [ "typeof(Microsoft.CodeAnalysis.Sarif.Readers.SarifVersionConverter)" ]
}
}

],
"stack": [
{
Expand Down
18 changes: 18 additions & 0 deletions src/Sarif/Core/Run.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,24 @@ public partial class Run
private static Invocation EmptyInvocation = new Invocation();
private static LogicalLocation EmptyLogicalLocation = new LogicalLocation();

public bool ShouldSerializeColumnKind()
Copy link

Choose a reason for hiding this comment

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

ShouldSerializeColumnKind [](start = 20, length = 25)

Wow, I hadn't looked closely at this until now. This is the first time I wondered who was calling these, and I see that JSON.NET has a built-in mechanism for conditional serialization.

{
// This serialization helper does two things.
//
// First, if ColumnKind has not been
// explicitly set, we will set it to the value that works for the Microsoft
// platform (which is not the specified SARIF default). This makes sure that
// the value is set appropriate for code running on the Microsoft platform,
// even if the SARIF producer is not aware of this rather obscure value.
if (this.ColumnKind == ColumnKind.None)
{
this.ColumnKind = ColumnKind.Utf16CodeUnits;
}
Copy link

Choose a reason for hiding this comment

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

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

I agree with this logic, but not necessarily with its home in what should be essentially a "getter" method. It doesn't feel right for this method to have a side effect. Please spend a few moments pondering whether there's a better place for this, but don't block if you can't think of one.


// Second, we do not persist this value if it is set to its default.
return this.ColumnKind != ColumnKind.UnicodeCodePoints;
}

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

public bool ShouldSerializeGraphs() { return this.Graphs != null && this.Graphs.Values.Any(); }
Expand Down
Loading