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.

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

`HttpMessage#withStringRepresentation` #2412

Closed

Conversation

@pfcoperez
Copy link

commented Feb 2, 2019

Closes #2409

Allow custom string representations for HttpRequest and HttpResponse. These representations are provided as functions from HttpRequest (or HttpResponse) to String and default to the representation so far provided by toString. e.g:

val stringRepresentation: HttpRequest  String = { request 
    import request._
    s"""HttpRequest(${_1},${_2},${_3},${_4},${_5})"""
  }

This possibilities the reduction of the risk of leaking headers in logs or forgotten traces by, for example, using mapRequest at the top level route:

        mapRequest(_.withStringRepresentation(rq  s"""HttpRequest(${rq._1},${rq._2},${rq._4},${rq._5})""")) {
          path("abc") {
            extractRequest { request 
              complete(request.toString)
            }
          }
        }
@lightbend-cla-validator

This comment has been minimized.

Copy link

commented Feb 2, 2019

Hi @pfcoperez,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Feb 2, 2019

Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK TO TEST' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged.

For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask!

@@ -0,0 +1,11 @@
# HttpMessage new declares methods to represent and set its string representation

This comment has been minimized.

Copy link
@pfcoperez

pfcoperez Feb 3, 2019

Author

Ignore binary changes derived from the addition of stringRepresentation, these are incremental (they add optional parameters), e.g:

val stringRepresentation: HttpRequest String = { request
import request._
s"""HttpRequest(${_1},${_2},${_3},${_4},${_5})"""
}

def copy(
method: HttpMethod = method,
uri: Uri = uri,
headers: immutable.Seq[HttpHeader] = headers,
entity: RequestEntity = entity,
protocol: HttpProtocol = protocol,
stringRepresentation: HttpRequest String = stringRepresentation
) = new HttpRequest(method, uri, headers, entity, protocol, stringRepresentation)

They happen in:

Neither the sealed trait nor the final classes can be extended by any user code (if they were, this PR wouldn't be necessary). So I find these exceptions valid as they seem to meet the criteria listed in https://github.com/akka/akka/blob/master/CONTRIBUTING.md#binary-compatibility

if it is adding API to classes / traits which are only meant for extension by Akka itself, i.e. should not be extended by end-users

This comment has been minimized.

Copy link
@raboof

raboof Feb 25, 2019

Member

Thanks for being careful about this. I think the change to the 'copy' method is not safe, though: even though there is a default value, so it will be source-compatible, the signature of the public 'copy' method does change, and this would introduce problems with code compiled against the other version of the 'copy' method. I think a way to solve this might be to keep the old signature (but remove the default values), mark it 'deprecated', and add a new one with the additional field. The constructor has the same issue.

This comment has been minimized.

Copy link
@pfcoperez

pfcoperez Apr 2, 2019

Author

@raboof I've just applied your suggestion and provided legacy versions of copy. Also replaced the default parameter for the constructors by two versions of the constructor, one of them binary backwards compatible.

The only exclusions are now those for the new methods.

@jypma

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

A couple of general thoughts on the direction:

  • I wonder if how to represent an object in logging isn't more a property of the logging framework, rather than the object. However, I do understand the desire to do this, and the subjectivity on which parameters/headers to include/exclude by default.
  • Java API is missing
@pfcoperez

This comment has been minimized.

Copy link
Author

commented Feb 5, 2019

@jypma Thanks for taking a look 🙇

I wonder if how to represent an object in logging isn't more a property of the logging framework, rather than the object.

💯 I agree and the idea of this is not to use it as a way of providing the representation for the logs but as a security measure which removes the potential risk of leaking of sensitive headers in IO traces or interaction due to the fact that all JVM objects have a toString method.

However, I do understand the desire to do this, and the subjectivity on which parameters/headers to include/exclude by default.

✔️

Java API is missing

I'll be adding it soon 🙂

@pfcoperez pfcoperez force-pushed the pfcoperez:feature/2409/with_string_representation branch from 619fbd9 to 6e34252 Feb 7, 2019
@pfcoperez

This comment has been minimized.

Copy link
Author

commented Feb 7, 2019

@jypma 3ac44f1 Adds withStringRepresentation receiving a java.util.function.Function[Self, String] to the Java API.

@pfcoperez pfcoperez force-pushed the pfcoperez:feature/2409/with_string_representation branch from 6e34252 to 3ac44f1 Feb 7, 2019
@raboof

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

OK TO TEST

@akka-ci akka-ci added validating tested and removed validating labels Feb 25, 2019
@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Feb 25, 2019

Test PASSed.

Copy link
Member

left a comment

I'm not sure I'm comfortable with the extra weight (in memory use and in complexity) this PR adds for this feature.

Perhaps we should consider trimming the toString output for everyone, rather than making this configurable?

val protocol: HttpProtocol)
extends jm.HttpRequest with HttpMessage {
val protocol: HttpProtocol,
val stringRepresentation: HttpRequest String = { request

This comment has been minimized.

Copy link
@raboof

raboof Feb 25, 2019

Member

It looks a bit strange to have a function from HttpRequest => String on a HttpRequest. I understand how we got here: calculating the string representation in the constructor seems wasteful, but you want it to be customizable and be able to refer to it from the toString. Using a lazy val would be neat, but introduces 2 new fields to this class, while adding a single one already seems kind of questionable.

@jypma

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

Trimming it by default is an interesting alternative approach.

Perhaps in documentation, people could then be pointed at things like Show in cats and scalaz, which provide decent ideas on how to generically approach it without having to predefine toString().

@pfcoperez

This comment has been minimized.

Copy link
Author

commented Feb 25, 2019

@raboof @jypma 💯 To the very limited toString approach and the usage of type classes to bestow presentation behaviour to the class.

If you concur on that solution I am more than happy to change the code proposed here to a simplified message where headers nor other request details are present.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Mar 5, 2019

Test PASSed.

@pfcoperez pfcoperez force-pushed the pfcoperez:feature/2409/with_string_representation branch from 3ac44f1 to 2c12718 Apr 2, 2019
@pfcoperez

This comment has been minimized.

Copy link
Author

commented Apr 2, 2019

@raboof I've upgraded this PR with your suggestions related to binary compatibility. Did you decide about whether using this or a crippled toString by default?

If needed, I can open a second PR with that and close this one.
I personally like that solution (crippling toString) but I also see it as the more disruptive for users. I can imagine new issues complaining about the change. It is, obviously, Akka people call 🙂

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Apr 2, 2019

Test PASSed.

@pfcoperez pfcoperez force-pushed the pfcoperez:feature/2409/with_string_representation branch from 2c12718 to faaa181 Apr 2, 2019
@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Apr 2, 2019

Test PASSed.

@pfcoperez

This comment has been minimized.

Copy link
Author

commented Jul 8, 2019

Superseded by #2560

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.