-
Notifications
You must be signed in to change notification settings - Fork 93
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.
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
Emit column kind default explicitly for Windows SDK SARIF emit. #1160
Changes from all commits
c2ceb47
b5e1797
d0ea69d
3d50d89
83df6bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
// 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, | ||
// 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); | ||
|
||
sarifLog = JsonConvert.DeserializeObject<SarifLog>(json); | ||
sarifLog.Runs[0].ColumnKind.Should().Be(expectedRoundTrippedValue); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -656,6 +656,14 @@ | |
"unicodeCodePoints" | ||
] | ||
} | ||
}, | ||
{ | ||
"kind": "AttributeHint", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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": [ | ||
|
@@ -738,7 +746,6 @@ | |
"arguments": [ "typeof(Microsoft.CodeAnalysis.Sarif.Readers.SarifVersionConverter)" ] | ||
} | ||
} | ||
|
||
], | ||
"stack": [ | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,25 @@ public partial class Run | |
private static Invocation EmptyInvocation = new Invocation(); | ||
private static LogicalLocation EmptyLogicalLocation = new LogicalLocation(); | ||
|
||
public bool ShouldSerializeColumnKind() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 will always explicitly serialize this value. Otherwise, we can't easily | ||
// distinguish between earlier versions of the format for which this property was typically absent. | ||
return true; | ||
} | ||
|
||
public bool ShouldSerializeFiles() { return this.Files != null && this.Files.Values.Any(); } | ||
|
||
public bool ShouldSerializeGraphs() { return this.Graphs != null && this.Graphs.Values.Any(); } | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,7 +21,6 @@ public override object ReadJson(JsonReader reader, Type objectType, object exist | |
{ | ||
throw new ArgumentNullException(nameof(reader)); | ||
} | ||
|
||
string value = (string)reader.Value; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Odd change. Doesn't our style require a blank line after a closing brace? See lines 34, 39, 53... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, we do not have a strict guideline around vertical whitespace in this scenario. Generally, code should be crafted to be readable and we prefer that developers have some latitude interpreting that guideline. Most of our coding requirements are designed to ensure completeness of information and minimize introduction of correctness problems. Still, I honestly don't know how this line deletion crept in and can revert. In reply to: 239900759 [](ancestors = 239900759) |
||
return Enum.Parse(objectType, ConvertToPascalCase(value)); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'w' => 'W'