Internal server error if there are multiple matching callbacks #269
Conversation
WalkthroughThe changes introduce several new test cases to the Postman collection for the nanoFramework WebServer end-to-end tests. A new test case named Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
Client->>Server: GET /multiplecallback
Server-->>Client: 500 Internal Server Error
Note right of Server: Response includes error message about multiple callbacks
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (9)
📒 Files selected for processing (1)
🧰 Additional context used🔇 Additional comments (10)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
nanoFramework.WebServer/WebServer.cs
Outdated
| } | ||
| else if (selectedRoute is null) | ||
| { | ||
| if (CommandReceived != null) |
There was a problem hiding this comment.
You still need to check auth. So, you should have to adjust with the previous logic.
There was a problem hiding this comment.
Authentication! That is a use case for multiple methods. Same route, but different method depending on whether credentials have been passed or not.
Changed the logic and documented the change in the README.md. Added extra unit tests.
Also put the WiFi data in a separate WiFi.cs file (not in git) and WiFi.TEMPLATE.cs (in git) as I almost committed my WAN credentials.
There was a problem hiding this comment.
@frobijn let's have atomic PRs please. Change only what is meant to be changed on the PR. In this case the fix for the multiple matching callbacks.
If there is an issue with the way wifi credentials are being stored, lets discuss a way to address that and add those changes on another PR.
But that should then have been in a PR before this one. I made the change of multiple callbacks before having issues with the WiFi credentials. If I would have to make the WiFi change before I could continue with the multiple callback change. Given the wait times between initiating a PR and having it closed, that makes it quite hard to do some small work. I would almost have to work on as many branches as there are code changes. Please please please allow for small improvements that make life easier for a contributor. |
|
In this case it's a simple matter of not including the changes with wifi.cs, nothing complicated. Please understand that this is not to make developers life complicated, is to improve project maintainability. FYI: I've just extracted these changes and added them in #270 . Updating this branch now so these are not part of the PR anymore. Our contribution guideline suggests that for breaking or large changes, these are discussed beforehand. That is to prevent waste of time/work and even frustration. No one likes to see it's work changed or the effort dismissed. No one here wants to bash developers willing to contribute. Quite the oposite. Still there are aspects that require a broader and 10.000ft view that (most likely) only maintainers have. Also know that for simple changes and fixes, you can do that right on the github webpage. No need to branch, github offers to start a PR for you. |
josesimoes
force-pushed
the
MultipleCallbacks
branch
from
a8a6b8d to
bc0dcc4
Compare
|
Well, I like to test stuff before starting a PR. Then a local branch is more practical. My point was not the local branch, but having multiple changes as PR in parallel. Keeping the PR-branches in sync is sometimes a hassle. But sequential PRs is a bit of a start-and-stop type of development. Checked the part that was separated in #270, the remaining part is in #271. |
|
I understand your point. Sometimes it can hinder productivity, but it's required for the greater good. 😉 |
…ication method/credentials. Error for multiple callbacks restricted to route-methods with the same authentication (method + credentials).
josesimoes
force-pushed
the
MultipleCallbacks
branch
from
bc0dcc4 to
c8588c3
Compare
|
| } | ||
| } | ||
| ``` | ||
| The webserver selects the route for a request: |
There was a problem hiding this comment.
| The webserver selects the route for a request: | |
| The webserver selects the route for a request: |
| - If one of the methods does not require authentication, that method is called. | ||
| - Otherwise a non-authorized response (401) will be returned. If one of the methods requires basic authentication, the `WWW-Authenticate` header is included to request credentials. | ||
|
|
||
| If two or more methods match the authentication method and credentials of the request, an internal server error is returned with a list of the methods. |
There was a problem hiding this comment.
then from the previous posted experience, can you be a bit more explicit on what will happen?
There was a problem hiding this comment.
In the new PR, the line last line is replaced by:
The webserver does not support more than one matching method. Calling multiple methods most likely results in an exception as a subsequent method tries to modify a response that is already processed by the first method. The webserver does not know what to do and returns an internal server error (500). The body of the response lists the matching methods.
Having multiple matching methods is considered a programming error. One way this occurs is if two methods in a controller accidentally have the same route. Returning an internal server error with the names of the methods makes it easy to discover the error. It is expected that the error is discovered and fixed in testing. Then the internal error will not occur in the application that is deployed to a device.
|
I tried to clean up my local git repository and create a cleaner MultipleCallbacks branch. I'm a bit surprised that this PR got closed by it. A new PR is on its way: #272 |
|
@frobijn this PR was closed the moment you've deleted the branch that originated it. It can be restored and reopened. If you prefer. |
Description
If a developer has created multiple methods of controller(s) that match the same web request, an internal server error is generated with the names of the matching methods.
Plus: .editorconfig added. This also changes the header comment of the modified code files.
Motivation and Context
It is a protection against coding errors. In the current code, if multiple callbacks match, both are executed with results in an exception in an unexpected place in the code. With the change applied, it is impossible to miss that there is a problem.
Assuming a developer/tester tests new or modified code the internal server error will not be generated in a production situation, so no data is leaked that may be considered a security risk.
I list it as "improvement", as one type of error (exception in the second callback) is replaced by another (internal server error).
How Has This Been Tested?
Postman, as prescribed.
Types of changes
Checklist:
Summary by CodeRabbit
New Features
SimpleRouteController_MultipleCallbackand various scenarios under theMixedControllercategory.Bug Fixes