Skip to content

Enhance MapActuators with null validation and logging - #1757

Closed
shreepatilprashant-ctrl wants to merge 1 commit into
SteeltoeOSS:mainfrom
shreepatilprashant-ctrl:main
Closed

shreepatilprashant-ctrl wants to merge 1 commit into
SteeltoeOSS:mainfrom
shreepatilprashant-ctrl:main

Conversation

@shreepatilprashant-ctrl

Copy link
Copy Markdown

refactor(management): inject explicit configuration validation loops and runtime catch protection to actuator endpoints

Description

{A detailed description}

Fixes #{bug number}

Quality checklist

  • Your code complies with our Coding Style.
  • You've updated unit and/or integration tests for your change, where applicable.
  • You've updated documentation for your change, where applicable.
    If your change affects other repositories, such as Documentation and/or Samples, add linked PRs here.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.
  • You've added required license files and/or file headers (explaining where the code came from with proper attribution), where code is copied from StackOverflow, a blog, or OSS.

refactor(management): inject explicit configuration validation loops and runtime catch protection to actuator endpoints
@bart-vmware

Copy link
Copy Markdown
Member

@shreepatilprashant-ctrl Can you please explain what problem this PR is supposed to solve?

@shreepatilprashant-ctrl

Copy link
Copy Markdown
Author

@shreepatilprashant-ctrl Can you please explain what problem this PR is supposed to solve?

Hi Bart,
Thanks for reaching out. The main intent here is to improve observability during the application bootstrap sequence.
By explicitly catching a null builder argument and adding a dedicated log pattern, it helps isolate configuration issues quicker when services are spinning up in distributed environments. It just provides a clearer trail in the logs before the exception bubbles up.
I'm more than happy to adjust the formatting or the message if you have a preferred convention for handling startup telemetry in Steeltoe!
Regards
Prashant Patil

@bart-vmware

Copy link
Copy Markdown
Member

Did you actually try to run it? Because looking at the code, it doesn't even work: ArgumentNullException.ThrowIfNull(builder) executes before entering the try/catch block. But more importantly, exceptions like ArgumentNullException are last-resort input validation that are never supposed to happen under normal circumstances. They typically indicate a developer is doing something wrong (so fail-fast is desired), as opposed to regular failures like an unreachable network, a file not found, or a downstream server returning an error that deserve production logging. Steeltoe already contains logging for such transient failures and errors that only surface once in a while.

If you want such an audit trail, it would make more sense to me to catch exceptions at the call site, where more information is available about the intent. When doing so, the exception stack trace provides useful information about where the call originated from.

In this case, even if the try/catch block were entered, the code would still be unreachable. This is because Steeltoe calls it from https://github.com/SteeltoeOSS/Steeltoe/blob/main/src/Management/src/Endpoint/ApplicationBuilderExtensions.cs#L64:

builder.UseRouter(routeBuilder => routeBuilder.MapActuators());

As can be seen, the routeBuilder parameter is passed to us by ASP.NET code, whose source is located at https://github.com/dotnet/aspnetcore/blob/77e4202bed594e8c1063e697b289221e57858bd5/src/Http/Routing/src/Builder/RoutingBuilderExtensions.cs#L56-L57:

var routeBuilder = new RouteBuilder(builder);
action(routeBuilder);

This shows that ASP.NET never passes null for the routeBuilder parameter. Adding code in MapActuators that catches NullReferenceException would effectively be dead code, which is an undesired maintenance burden and degrades coverage metrics. So I don't think this change makes any sense. Therefore, I'm closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants