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

Downgrade the Warning to Information on missing Content-Length header in MultiplexingMiddleware #2146

Merged
merged 4 commits into from
Sep 14, 2024
+27 −23
Merged

Downgrade the Warning to Information on missing Content-Length header in MultiplexingMiddleware #2146

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e80933d
fix: downgrade the warning to information on missing content-length h…
Sep 3, 2024
e76c3b4
chore: add route name to logs
Sep 6, 2024
db1f729
test: fixing multiplexing middleware tests
Sep 6, 2024
bf57fc1
Code review by @raman-m
raman-m Sep 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Ocelot/Multiplexer/MultiplexingMiddleware.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ protected virtual async Task<Stream> CloneRequestBodyAsync(HttpRequest request,
}
else
{
Logger.LogWarning("Aggregation does not support body copy without Content-Length header!");
Logger.LogInformation("Aggregation does not support body copy without Content-Length header, skipping body copy.");
Copy link
Member

Choose a reason for hiding this comment

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

I recommend implementing a minor decision-making process based on the HTTP verb

  • If the verb is GET, then it should be logged at the Information level, as should HEAD, OPTIONS, and other verbs that do not have a body.
  • However, if the verb is PUT, POST, etc., then we should log at the Warning level.

This approach should suffice to determine the appropriate logging level.

Copy link
Contributor Author

@PaulARoy PaulARoy Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hi raman,

Hard disagree for me on this, as the presence or absence of body is not specified in the HTTP spec.

It's an entirely normal and supported scenario to have a PUT or POST query without body and it stills holds a meaning (https://lists.w3.org/Archives/Public/ietf-http-wg/2010JulSep/0276.html)

Also I know we probably won't agree on this, but GET, OPTIONS and HEAD are not prohibited from having a body (it's just undefined by the spec) :D

So I wouldn't log it as a warning and I wouldn't make a verb-specific logic here.

Copy link
Member

@raman-m raman-m Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Fair enough! Not an issue.

TODO

  • Add decision maker for logging level

raman-m marked this conversation as resolved.
Show resolved Hide resolved
}

return targetBuffer;
Expand Down