-
Notifications
You must be signed in to change notification settings - Fork 787
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.
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
Expunge all unsafe logging #6645
Conversation
val cl: Option[`Content-Length`] = entity.length match { | ||
case Some(l) => | ||
`Content-Length` | ||
.fromLong(l) | ||
.fold( | ||
_ => { | ||
Message.logger.warn(s"Attempt to provide a negative content length of $l") | ||
None | ||
}, | ||
Some(_), | ||
) | ||
case None => None | ||
} | ||
val cl: Option[`Content-Length`] = entity.length.flatMap( | ||
`Content-Length` | ||
.fromLong(_) | ||
.toOption | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't log in this method unless we make .withEntity
return a result in F[_]
... which seems like terrible ergonomics. So I just removed it.
@deprecated("Obsolete. Not implemented by any backends.", "0.23.12") | ||
object PushSupport { | ||
private[this] val logger = getLogger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than deal with this, since it was deprecated I just tossed it.
@deprecated( | ||
"""Deficient. See https://github.com/http4s/http4s/security/advisories/GHSA-52cf-226f-rhr6.""", | ||
"0.21.27", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed all the deprecated CORS stuff so I wouldn't have to rework its logging.
def apply[F[_]: Applicative, G[_]](http: Http[F, G]): Http[F, G] = { | ||
def apply[F[_]: Applicative, G[_]: Applicative: LoggerFactory]( | ||
http: Http[F, G] | ||
): G[Http[F, G]] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where we get into trouble. To log in this method (see below) we have to return the result in an effect now. Maybe this is ok, but it caused extensive refactoring of the CORS test suite.
@@ -82,13 +82,13 @@ class ResourceServiceBuilder[F[_]] private ( | |||
|
|||
def withBufferSize(bufferSize: Int): ResourceServiceBuilder[F] = copy(bufferSize = bufferSize) | |||
|
|||
def toRoutes(implicit F: Async[F]): HttpRoutes[F] = { | |||
def toRoutes(implicit F: Async[F]): F[HttpRoutes[F]] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another signature change here, that caused the test suite to need major refactoring. We could, also, just not log ...
Ouch 😂
|
If I get it right, would it be finally possible to use http4s without bringing slf4j/logback in the classpath? Blaze still depends on the logabck, but ember does not. |
Yes, exactly. No more slf4j dependency. |
Any updates? That's a definitively useful feature to merge. |
I can't merge my own PR, but if someone new champions this effort and fix the conflicts I can approve and merge 😜 |
This starts as a forward merge of #6614.
Then 8ee8295 goes crazy and tries to fixup all the unsafe logging by adding
LoggerFactory
constraints as needed. This ... was a bit messy.Inline comments incoming.