Skip to content

Add null check for args in CommandLineParser - #13451

Merged
Rob Holt (rjmholt) merged 4 commits into
PowerShell:masterfrom
iSazonov:cpp-agrs-null
Aug 28, 2020
Merged

Rob Holt (rjmholt) merged 4 commits into
PowerShell:masterfrom
iSazonov:cpp-agrs-null

Conversation

@iSazonov

@iSazonov Ilya (iSazonov) commented Aug 15, 2020 •

Copy link
Copy Markdown
Collaborator

PR Summary

Throw if args array contains a null.

PR Context

#13429 (comment)

PR Checklist

@iSazonov Ilya (iSazonov) added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Aug 15, 2020
@iSazonov

Copy link
Copy Markdown
Collaborator Author

While we are here maybe add a comment like "The 'args' can not be null and can not contain null elements." in XML summary for all public methods where it is used?

{
if (args[i] is null)
{
throw new ArgumentNullException("The 'args' array contains a null element.");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The message should be put in .resx file. Make the message to "The specified arguments should not contain null elements.". Also, please pass in the parameter name to ArgumentNullException.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nameof(args)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

No exceptions in the file use Resx file.
Please confirm using Resx file in the case.

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 27, 2020 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you need to differentiate an exception thrown due to user action from an exception that's thrown due to internal error only.
The InvalidOperationException thrown when _dirty == true can happen only when there is an internal error -- the parser gets reused somehow. It shouldn't happen and is not user facing.
The ArgumentNullException thrown here only because a user action -- the user pass in invalid args. It's user facing. For user-facing exceptions, the message should be localized eventually.

@iSazonov Ilya (iSazonov) Aug 28, 2020 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I see your point. The issue could be raised (1) only in host application, (2) only if the host application developer constructs args with null element. So it is not "end user action" but a bug in the host application that is "internal error" case too. It looks the same as if we passed a bug with _dirty and an user catches this edge case with non-localized message. Thoughts?

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 28, 2020 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Anyone using a PowerShell public API is an end user. An internal error is an error that cannot be triggered by an end user. In this case, an end user cannot cause the argument parser to be reused.
I hope this makes it more clear to you.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for clarify!

Done.

@ghost ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Aug 19, 2020
@ghost ghost added the Review - Needed The PR is being reviewed label Aug 27, 2020
@ghost

Copy link
Copy Markdown

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@rjmholt

Copy link
Copy Markdown
Collaborator

Dongbo Wang (@daxian-dbw) please update your review

@ghost ghost removed the Review - Needed The PR is being reviewed label Aug 27, 2020
@rjmholt Rob Holt (rjmholt) added the Review - Needed The PR is being reviewed label Aug 27, 2020
@daxian-dbw

Copy link
Copy Markdown
Member

Review updated: #13451 (comment)

@ghost ghost removed the Review - Needed The PR is being reviewed label Aug 27, 2020
…meterParserStrings.resx

Co-authored-by: Robert Holt <rjmholt@gmail.com>
@rjmholt
Rob Holt (rjmholt) merged commit 3b83a68 into PowerShell:master Aug 28, 2020
@iSazonov
Ilya (iSazonov) deleted the cpp-agrs-null branch August 29, 2020 06:41
@iSazonov Ilya (iSazonov) added this to the 7.1.0-preview.7 milestone Aug 29, 2020
@ghost

Copy link
Copy Markdown

🎉v7.1.0-preview.7 has been released which incorporates this pull request.:tada:

Handy links:

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants