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

Snippet production #2629

Merged
merged 9 commits into from
Feb 28, 2023
Merged

Snippet production #2629

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
47c2ee7
Update pipeline for multithreaded analysis.
michaelcfanning Feb 27, 2023
46fe546
All tests pass.
michaelcfanning Feb 27, 2023
10aeee3
Review clean-up.
michaelcfanning Feb 27, 2023
4892444
Rename object to globalContext.
michaelcfanning Feb 28, 2023
11b52ed
Update release notes.
michaelcfanning Feb 28, 2023
0a79322
Allow 512 byte context region snippets only (except when original con…
michaelcfanning Feb 28, 2023
8628812
Update release notes.
michaelcfanning Feb 28, 2023
48af19b
Merge remote-tracking branch 'origin' into snippet-production
michaelcfanning Feb 28, 2023
79b579f
Code review feedback.
michaelcfanning Feb 28, 2023
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 ReleaseHistory.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* BRK: Eliminate `MulthreadedAnalyzeCommandBase.EngineException` and `IAnalysisContext.RuntimeException` properties in favor of `IAnalysisContext.RuntimeExceptions`. [#2627](https://github.com/microsoft/sarif-sdk/pull/2627)
* BRK: Rename `LogFilePersistenceOptions` to `FilePersistenceOptions` (due to its general applicability in other file persistence contexts other than output logs).[#2625](https://github.com/microsoft/sarif-sdk/pull/2625)
* BRK: Many breaking changes in `IAnalysisContext` and `AnalyzeContextBase`. [#2625](https://github.com/microsoft/sarif-sdk/pull/2625)
* BUG: Eliminate creation of extremely large context region snippets (now always restricted to 512 chars). https://github.com/microsoft/sarif-sdk/pull/2629
* BUG: Eliminate per-context allocations contributing to unnecessary memory use. [#2625](https://github.com/microsoft/sarif-sdk/pull/2625)
* NEW: Rewrite `MultithreadedAnalyzeCommandBase` pipeline to allow for timeout, cancellation, and better API-driven use. [#2625](https://github.com/microsoft/sarif-sdk/pull/2625)
* NEW: Move large amounts of scan data to the context object, to streamline pipeline and allow for XML-driven configuration. [#2625](https://github.com/microsoft/sarif-sdk/pull/2625)
Expand Down
9 changes: 6 additions & 3 deletions src/Sarif/FileRegionsCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,11 @@ public Region ConstructMultilineContextSnippet(Region inputRegion, Uri uri, stri
// Generating full inputRegion to prevent issues.
Region originalRegion = this.PopulateTextRegionProperties(inputRegion, uri, populateSnippet: true, fileText);

if (originalRegion.CharLength >= BIGSNIPPETLENGTH)
Copy link
Collaborator

@EasyRhinoMSFT EasyRhinoMSFT Feb 28, 2023 •
edited by michaelcfanning
Loading

Choose a reason for hiding this comment

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

=

So 512 is considered big? I.e., the max is 511 #WontFix

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1. This should be >

Copy link
Member Author

Choose a reason for hiding this comment

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

why? if the size of the original region is as big as our bigsnippet maximum, why would attempt to generate it? what am i missing?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was verifying that the max length is 511, since the >= operator excludes 512. This seems to be an off-by-one considering the statement that snippets are "restricted to 512 bytes in size"

{
return originalRegion.DeepClone();
}

int maxLineNumber = newLineIndex.MaximumLineNumber;

var region = new Region
Expand All @@ -188,9 +193,7 @@ public Region ConstructMultilineContextSnippet(Region inputRegion, Uri uri, stri
region.EndLine = 0;
region.CharOffset = Math.Max(0, originalRegion.CharOffset - SMALLSNIPPETLENGTH);

region.CharLength =
Math.Min(originalRegion.CharLength + region.CharOffset + SMALLSNIPPETLENGTH,
fileText.Length - region.CharOffset);
region.CharLength = Math.Min(BIGSNIPPETLENGTH, fileText.Length - region.CharOffset);

// Generating multiline region with 128 characters to the left and right from the
// originalRegion if possible.
Expand Down
10 changes: 2 additions & 8 deletions src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -722,12 +722,7 @@ public void FileRegionsCache_IncreasingToLeftAndRight()
region = fileRegionsCache.PopulateTextRegionProperties(region, uri, true, fileContent);

Region multilineRegion = fileRegionsCache.ConstructMultilineContextSnippet(region, uri);

// 114 (charoffset) + 600 (charlength) + (128 - prefixed length).

// The length of our prepended data;
int prefixed = Math.Max(multilineRegion.CharOffset - 128, 0);
multilineRegion.CharLength.Should().Be(prefixed + region.CharLength + (128 - prefixed));
multilineRegion.CharLength.Should().Be(region.CharLength);
}

[Fact]
Expand All @@ -745,8 +740,7 @@ public void FileRegionsCache_PopulatesWithOneLine_IncreasingToTheRight()
region = fileRegionsCache.PopulateTextRegionProperties(region, uri, true, content);
Region multilineRegion = fileRegionsCache.ConstructMultilineContextSnippet(region, uri);

// CharLength + 128 to the right = 428 characters
multilineRegion.CharLength.Should().Be(300 + 128);
multilineRegion.CharLength.Should().Be(FileRegionsCache.BIGSNIPPETLENGTH);
multilineRegion.Snippet.Text.Should().NotBeNullOrEmpty();
}

Expand Down