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

Fix null reference when markdown content is empty #7463

Merged
merged 4 commits into from
Aug 27, 2018
Merged

Fix null reference when markdown content is empty #7463

merged 4 commits into from
Aug 27, 2018

Conversation

adityapatwardhan
Copy link
Member

@adityapatwardhan adityapatwardhan commented Aug 6, 2018 •
edited
Loading

Fix #7453

PR Summary

Fix cases where markdown content can be empty and we get a null reference exception.

PR Checklist

iSazonov
iSazonov reviewed Aug 7, 2018
View reviewed changes
Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

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

I wonder that we have to check so many null references. Seems we need review all the code (not in the PR).

src/Microsoft.PowerShell.MarkdownRender/FencedCodeBlockRenderer.cs Outdated
@@ -16,25 +16,28 @@ internal class FencedCodeBlockRenderer : VT100ObjectRenderer<FencedCodeBlock>
{
protected override void Write(VT100Renderer renderer, FencedCodeBlock obj)
{
foreach (var codeLine in obj.Lines.Lines)
if (obj?.Lines != null && obj.Lines.Lines != null)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we use "?" we could do

if (obj?.Lines?.Lines != null)

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to obj?.Lines.Lines != null

obj.Lines is of type StringLineGroup which cannot be null.

src/Microsoft.PowerShell.MarkdownRender/FencedCodeBlockRenderer.cs Outdated
{
if (!string.IsNullOrWhiteSpace(codeLine.ToString()))
foreach (var codeLine in obj?.Lines.Lines)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have checked obj != null in previous line and could remove "?"

foreach (var codeLine in obj.Lines.Lines)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

src/Microsoft.PowerShell.MarkdownRender/HeaderBlockRenderer.cs Outdated
@@ -16,38 +16,43 @@ internal class HeaderBlockRenderer : VT100ObjectRenderer<HeadingBlock>
{
protected override void Write(VT100Renderer renderer, HeadingBlock obj)
{
// Format header and then add blank line to improve readability.
switch (obj.Level)
string headerText = obj.Inline?.FirstChild?.ToString();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we check obj != null too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

src/Microsoft.PowerShell.MarkdownRender/LinkInlineRenderer.cs Outdated
}
else
{
renderer.Write(renderer.EscapeSequences.FormatLink(obj.FirstChild.ToString(), obj.Url));
renderer.Write(renderer.EscapeSequences.FormatLink(obj.FirstChild?.ToString(), obj.Url));
Copy link
Collaborator

Choose a reason for hiding this comment

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

We check obj.FirstChild != null in both code path in the "if-else" - seems we should move the check to new "if":

if (obj.FirstChild != null)
{
    if (obj.IsImage)
    {
        renderer.Write(renderer.EscapeSequences.FormatImage(obj.FirstChild.ToString()));
    }
    else
    {
        renderer.Write(renderer.EscapeSequences.FormatLink(obj.FirstChild.ToString(), obj.Url));
    }
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

The question marks here would have different semantics of course, but I imagine FormatLink will thrown an NRE if its first argument is null.

So I would do similar to @iSazonov (just a style change):

if (obj.FirstChild == null)
{
    return;
}

if (obj.IsImage)
{
    renderer.Write(renderer.EscapeSequences.FormatImage(obj.FirstChild.ToString()));
}
else
{
    renderer.Write(renderer.EscapeSequences.FormatLink(obj.FirstChild.ToString(), obj.Url));
}

Copy link
Member Author

Choose a reason for hiding this comment

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

We support having empty alt-text for images and empty link text. So updated accordingly.

@adityapatwardhan
Copy link
Member Author

@rjmholt @iSazonov Addressed the comments please have another look.

rjmholt
rjmholt reviewed Aug 13, 2018
View reviewed changes
src/Microsoft.PowerShell.MarkdownRender/FencedCodeBlockRenderer.cs Outdated
{
if (!string.IsNullOrWhiteSpace(codeLine.ToString()))
foreach (var codeLine in obj.Lines.Lines)
Copy link
Collaborator

Choose a reason for hiding this comment

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

string instead of var would be more obvious here

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

src/Microsoft.PowerShell.MarkdownRender/LinkInlineRenderer.cs Outdated
@@ -16,14 +16,16 @@ internal class LinkInlineRenderer : VT100ObjectRenderer<LinkInline>
{
protected override void Write(VT100Renderer renderer, LinkInline obj)
{
string text = obj?.FirstChild?.ToString();

// Format link as image or link.
if (obj.IsImage)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If obj is null, this will cause an NRE. Would the logic be preserved like this:

string text = obj?.FirstChild?.ToString();
if (text == null)
{
    return;
}

// Golden path code...

?

Copy link
Member Author

Choose a reason for hiding this comment

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

obj can never be null, hence removed ? after obj.

We allow text to be null hence the above logic holds.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if (obj.IsImage) - can cause an NRE.

Copy link
Member Author

Choose a reason for hiding this comment

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

@iSazonov obj is guaranteed to be not null. If it is null the renderer is not called.

@adityapatwardhan
Copy link
Member Author

@rjmholt Updated.

rjmholt
rjmholt approved these changes Aug 14, 2018
View reviewed changes
Copy link
Collaborator

@rjmholt rjmholt left a comment

Choose a reason for hiding this comment

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

LGTM!

iSazonov
iSazonov reviewed Aug 15, 2018
View reviewed changes
src/Microsoft.PowerShell.MarkdownRender/FencedCodeBlockRenderer.cs
// This specifically helps for parameters help content.
if (string.Equals(obj.Info, "yaml", StringComparison.OrdinalIgnoreCase))
{
renderer.Write("\t").WriteLine(codeLine.ToString());
Copy link
Collaborator

Choose a reason for hiding this comment

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

We call codeLine.ToString()) twice above - should we use a variable?
And in line 34 too.

Copy link
Member Author

Choose a reason for hiding this comment

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

One is in if and other is in else, so only one of them is called. And this change is out of scope for this PR.

@iSazonov
Copy link
Collaborator

@daxian-dbw @SteveL-MSFT The PR is ready to merge if you haven't comments.

@iSazonov iSazonov merged commit aec954e into PowerShell:master Aug 27, 2018
@adityapatwardhan adityapatwardhan deleted the FixMarkdownNullReference branch August 28, 2018 17:09
@TravisEz13 TravisEz13 added this to the v6.1.0 milestone Aug 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjmholt rjmholt rjmholt approved these changes

@iSazonov iSazonov iSazonov approved these changes

@SteveL-MSFT SteveL-MSFT Awaiting requested review from SteveL-MSFT

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw

Assignees

@iSazonov iSazonov

Labels
None yet
Projects
None yet
Milestone
v6.1.0
Development

Successfully merging this pull request may close these issues.

NullReferenceException in ConvertFrom-Markdown -AsVT100EncodedString with empty alt text
4 participants
@adityapatwardhan @iSazonov @rjmholt @TravisEz13