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

[Bug]: XmlPeek and XmlPoke tasks when optional parameter is not provided, throw unhandled exceptions #9140

Closed
jrdodds opened this issue Aug 20, 2023 · 2 comments · Fixed by #9194
Closed

[Bug]: XmlPeek and XmlPoke tasks when optional parameter is not provided, throw unhandled exceptions #9140

jrdodds opened this issue Aug 20, 2023 · 2 comments · Fixed by #9194
Assignees
@jrdodds
Labels
bug triaged

Comments

@jrdodds
Copy link
Contributor

jrdodds commented Aug 20, 2023 •
edited
Loading

Issue Description

XmlPeek

Query parameter

The XmlPeek task has an optional Query parameter. When the Query parameter is not provided, the task will fail with an "unhandled exception" error.

...: error MSB4018: The "XmlPeek" task failed unexpectedly.
...: error MSB4018: This is an unhandled exception from a task -- PLEASE OPEN A BUG AGAINST THE TASK OWNER.
...: error MSB4018: System.ArgumentNullException: Parameter "Query" cannot be null.
...

XmlPoke

Query parameter

The XmlPoke task also has an optional Query parameter with the same "unhandled exception" issue.

...: error MSB4018: The "XmlPoke" task failed unexpectedly.
...: error MSB4018: This is an unhandled exception from a task -- PLEASE OPEN A BUG AGAINST THE TASK OWNER.
...: error MSB4018: System.ArgumentNullException: Parameter "Query" cannot be null.
...
XmlInputPath parameter

Further the optional XmlInputPath parameter when not provided will also cause the task to fail with an "unhandled exception" error.

...: error MSB4018: The "XmlPoke" task failed unexpectedly.
...: error MSB4018: This is an unhandled exception from a task -- PLEASE OPEN A BUG AGAINST THE TASK OWNER.
...: error MSB4018: System.ArgumentNullException: Parameter "XmlInputPath" cannot be null.
...

Steps to Reproduce

Running MSBuild against each of the following three minimal projects will demonstrate each of the three issues.

XmlPeek - Query

<Project>
    <Target Name="Test">
        <XmlPeek />
    </Target>
</Project>

XmlPoke - Query

<Project>
    <Target Name="Test">
        <XmlPoke XmlInputPath="nonesuch" />
    </Target>
</Project>

XmlPoke - XmlInputPath

<Project>
    <Target Name="Test">
        <XmlPoke Query="nonesuch" />
    </Target>
</Project>

TestProjects.zip

Expected Behavior

Appropriate specific error messages should be reported.

The Query parameter for both tasks and the XmlInputPath parameter for XmlPoke are de facto required parameters. If changed from optional to required, then error MSB4044 can be reported, e.g.

error MSB4044: The "XmlPeek" task was not given a value for the required parameter "Query".

Actual Behavior

The "This is an unhandled exception from a task ..." message is reported.

Analysis

The two tasks for the three parameters are calling ErrorUtilities.VerifyThrowArgumentNull() instead of reporting a more specific error message.

A possible fix:

  • Add [Required] to the parameters.
  • Test the parameters and report a RequiredPropertyNotSetError message (MSB4044) when no value is provided.
  • Update the task documentation.

Versions & Configurations

macOS Monterey
MSBuild version 17.8.0-dev-23419-01+76a6ec27c for .NET
17.8.0.41901

Windows 11
MSBuild version 17.8.0-dev-23419-01+c5a28edd3 for .NET
17.8.0.41901
@jrdodds jrdodds added bug needs-triage Have yet to determine what bucket this goes in. labels Aug 20, 2023
@jrdodds
Copy link
Contributor Author

jrdodds commented Aug 22, 2023

If the choice is made to address this issue, I can provide the fix.

@AR-May
Copy link
Member

AR-May commented Aug 29, 2023

If the choice is made to address this issue, I can provide the fix.

Team triage: sounds like a bug to us. We would gladly take a fix for it, thanks!

@AR-May AR-May removed the needs-triage Have yet to determine what bucket this goes in. label Aug 29, 2023
@jrdodds jrdodds mentioned this issue Aug 31, 2023
Merged
rainersigwald pushed a commit that referenced this issue Oct 4, 2023
@jrdodds
update XmlPeek and XmlPoke tasks (#9194)
1aba44b 
Add `[Required]` to parameters.

- `XmlPeek` Task
  - Change `Query` parameter.
  - Remove redundant `Dispose` that was flagged by the analyzer.
  - Change XmlPeek.XmlInput class from `Internal` to `private sealed` and change access of some members
  - Minor cleanup changes
- `XmlPoke` Task
  - Change `Query` parameter.
  - Change `XmlInputPath` parameter.
  - Minor cleanup changes
- XmlPeek_Tests class
  - Add new `PeekWithNoParameters` test
- XmlPoke_Tests class
  - Remove `PokeMissingParams` test
    - The test was defined as a `[Fact]` and used a for loop to test 4 distinct cases
    - The test expected `ArgumentNullException` to be thrown
  - Add 4 new tests, one for each of the four cases:
    - `PokeWithNoParameters`
    - `PokeWithMissingRequiredQuery`
    - `PokeWithMissingRequiredXmlInputPath`
    - `PokeWithRequiredParameters` (completes the replacement of `PokeMissingParams` but might be a redundant test)

Fixes #9140.
bulatgrzegorz pushed a commit to bulatgrzegorz/selective-condition-evaluator that referenced this issue Oct 16, 2023
@jrdodds
update XmlPeek and XmlPoke tasks (dotnet#9194)
68d4aef 
Add `[Required]` to parameters.

- `XmlPeek` Task
  - Change `Query` parameter.
  - Remove redundant `Dispose` that was flagged by the analyzer.
  - Change XmlPeek.XmlInput class from `Internal` to `private sealed` and change access of some members
  - Minor cleanup changes
- `XmlPoke` Task
  - Change `Query` parameter.
  - Change `XmlInputPath` parameter.
  - Minor cleanup changes
- XmlPeek_Tests class
  - Add new `PeekWithNoParameters` test
- XmlPoke_Tests class
  - Remove `PokeMissingParams` test
    - The test was defined as a `[Fact]` and used a for loop to test 4 distinct cases
    - The test expected `ArgumentNullException` to be thrown
  - Add 4 new tests, one for each of the four cases:
    - `PokeWithNoParameters`
    - `PokeWithMissingRequiredQuery`
    - `PokeWithMissingRequiredXmlInputPath`
    - `PokeWithRequiredParameters` (completes the replacement of `PokeMissingParams` but might be a redundant test)

Fixes dotnet#9140.
@AR-May AR-May added the triaged label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jrdodds jrdodds

Labels
bug triaged
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

update XmlPeek and XmlPoke tasks jrdodds/msbuild
2 participants
@jrdodds @AR-May