[BUG] DefaultServerLog.decodeFailureHandled should use doLogAllDecodeFailures function

[o-shevchenko]

Tapir version: 1.2.11

Scala version: 2.13

Describe the bug Currently, it's not possible to disable logging of decode error. decodeFailureHandled doesn't take into account logAllDecodeFailures and doesn't use doLogAllDecodeFailures function, so we can't override such behavior. We don't want to log decode errors since it might contain sensitive information but we still want to log other logWhenHandled events that are not related for decoding.

https://github.com/softwaremill/tapir/blob/master/server/core/src/main/scala/sttp/tapir/server/interceptor/log/ServerLog.scala#L102

override def decodeFailureNotHandled(ctx: DecodeFailureContext, token: TOKEN): F[Unit] =
    if (logAllDecodeFailures)
      doLogAllDecodeFailures(
        s"Request: ${showRequest(ctx.request)}, not handled by: ${showEndpoint(ctx.endpoint)}${took(token)}; decode failure: ${ctx.failure}, on input: ${ctx.failingInput.show}",
        exception(ctx)
      )
    else noLog

  override def decodeFailureHandled(ctx: DecodeFailureContext, response: ServerResponse[_], token: TOKEN): F[Unit] =
    if (logWhenHandled)
      doLogWhenHandled(
        s"Request: ${showRequest(ctx.request)}, handled by: ${showEndpoint(
            ctx.endpoint
          )}${took(token)}; decode failure: ${ctx.failure}, on input: ${ctx.failingInput.show}; response: ${showResponse(response)}",
        exception(ctx)
      )
    else noLog

[adamw]

Right, so now the logWhenHandled flag controls the behaviour of all methods which are called when a request was handled: decodeFailureHandled, securityFailureHandled and requestHandled. What would you propose as a solution to your problem - adding an additional flag?

As a work-around you can subclass / provide a completely custom implementation of ServerLog, with the behaviour that you need.

[o-shevchenko]

Hi

As a work-around you can subclass / provide a completely custom implementation of ServerLog

That's what I've done.

What would you propose as a solution to your problem - adding an additional flag?

Not sure what can be considered as a good solution here. But current logic is not flexible and confusing. I expected that logAllDecodeFailures flag should be checked in decodeFailureHandled also I expected that I can set doLogWhenHandled function to use in decodeFailureHandled.

Adding a separate function like doLogAllDecodeFailures for decodeFailureHandled sounds like a good thing to do. Or maybe we should check both flags and use doLogAllDecodeFailures instead of doLogWhenHandled in decodeFailureHandled

override def decodeFailureHandled(ctx: DecodeFailureContext, response: ServerResponse[_], token: TOKEN): F[Unit] =
    if (**logAllDecodeFailures** && logWhenHandled)
      **doLogAllDecodeFailures**(
        s"Request: ${showRequest(ctx.request)}, handled by: ${showEndpoint(
            ctx.endpoint
          )}${took(token)}; decode failure: ${ctx.failure}, on input: ${ctx.failingInput.show}; response: ${showResponse(response)}",
        exception(ctx)
      )
    else noLog

But I think adding a new function will be better to honor current structure and to not increase cognitive complexity here. Or the current structure should be reconsidered.

[adamw]

What do you think could be a better way to structure the ServerLog class?

[o-shevchenko]

Not sure, interesting to hear your thoughts

[adamw]

Ok, let's analyse this. I think that the ServerLog trait, is clear enough and provides all the necessary hooks. So no improvements are needed here - you can always provide your own implementation of the entire interface.

So the issue is with how to customise the DefaultServerLog class, specifically around handling decode failures. It is not uncommon for an endpoint to be decoded, and a decode failure not being handled, if the endpoint doesn't "match" the request (e.g. because there's a path template mismatch). So we've got two cases to cover: the decode failure results in a response, or the decode failure results in a "no match".

The design of DefaultSeverLog is that you can customise which logs appear, and how the logging is performed, while the exact log messages are fixed. Following this logic, without radically changing the structure, I think we can simply introduce a flag-per-callback, so that each of requestReceived, decodeFailureHandled would have both a doLogWhenRequestReceived method and logWhenReceived flag. This would mean breaking up the flags and logging functions which are being reused right now, hopefully providing an easier to understand solution. What do you think? Maybe you'd have time to implement this and create a PR?