aspnetcore icon indicating copy to clipboard operation
aspnetcore copied to clipboard

NavigationException doesn't have a useful message.

Open danmoseley opened this issue 2 years ago • 10 comments

this https://github.com/dotnet/aspnetcore/blob/d8a3aa870862d01a8b3a3235f8dd7fc951dd8ff4/src/Components/Server/src/Circuits/RemoteNavigationManager.cs#L91

creates an exception like this image with the URL in the Location field.

Ideally it should have a useful message that includes the URI, eg., "Could not navigate to https://localhost:7298/item/99"

the problem is that NavigationException is not overriding Message and ToString https://github.com/dotnet/aspnetcore/blob/d8a3aa870862d01a8b3a3235f8dd7fc951dd8ff4/src/Components/Components/src/NavigationException.cs#L9

If we want to keep the URL out of the message for security reasons, it should at least have a message like "Could not navigate to the URL".

danmoseley avatar Oct 31 '23 20:10 danmoseley

If we want to keep the URL out of the message for security reasons, it should at least have a message like "Could not navigate to the URL".

@blowdart what is our policy for what can go into exception messages out of aspnet ? We disable display by default, of course. But these may go into logs. If we don't want the URL in the message, is it OK in the ToString()? (after all, it's in a property on the exception object)

danmoseley avatar Oct 31 '23 20:10 danmoseley

@danmoseley you are only probably seeing that because you have first chance exceptions turned on. The exception gets internally caught by the framework and handled. Is that not the case in your situation?

javiercn avatar Nov 01 '23 15:11 javiercn

I'm in conversations with other teams internally about logs and error messages, which may drive some requirements for 9

blowdart avatar Nov 01 '23 16:11 blowdart

I'm following up with the Debugger team to see how we can ignore certain types of exceptions to avoid confusion here.

mkArtakMSFT avatar Nov 01 '23 16:11 mkArtakMSFT

you have first chance exceptions turned on

Could well be. If it's always caught then it doesn't matter much. But first chance do still get logged in the debugger output - why not override Message.

It's possible you could ask the VS debugger team to buy default not catch this first chance. They seem to do this for certain exception types out of the box. (I'm assuming you can't avoid throw/catch here due to architecture)

danmoseley avatar Nov 02 '23 01:11 danmoseley

Thanks for contacting us.

We're moving this issue to the .NET 9 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

ghost avatar Nov 02 '23 16:11 ghost

After some further discussion in the team we're moving forward with the following:

  1. Change the exception message to make it more user friendly
  2. Follow up with the debugger team to see how to avoid treating these as first-chance exceptions.

mkArtakMSFT avatar Nov 02 '23 16:11 mkArtakMSFT

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

ghost avatar Nov 19 '23 05:11 ghost

Looks like this issue has been identified as a candidate for community contribution. If you're considering sending a PR for this issue, look for the Summary Comment link in the issue description. That comment has been left by an engineer on our team to help you get started with handling this issue. You can learn more about our Help Wanted process here

Could I possibly pick this issue up seeing that the open PR is currently not active

spdebeer avatar Jun 27 '24 07:06 spdebeer