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

Refactored library completely #6

Merged
merged 32 commits into from Oct 6, 2015
Merged

Refactored library completely #6

merged 32 commits into from Oct 6, 2015

Conversation

whiskeysierra
Copy link
Collaborator

I renamed the project to Logbook in the process. Still missing:

  • support for obfuscators (headers, parameters, body)
  • ~50% code coverage
  • logo for readme

@lukasniemeier-zalando
Copy link
Member

  • I'd stick with obfuscate the body as a String
  • You need to insert you as a (or the) maintainer 😛

@whiskeysierra
Copy link
Collaborator Author

I'd stick with obfuscate the body as a String

Absolutely, I'd just think about having a different interface for the body (without the fake key):

String obfuscate(final String body);

vs.

String obfuscate(final String key, final String value);

@whiskeysierra
Copy link
Collaborator Author

I'd just think about having a different interface for the body

Otherwise I have the fear that clients, without reading the docs properly, would just look at the method having a key and a value and would assume that a body obfuscator would get called for every property + value (e.g. for json).

@lukasniemeier-zalando
Copy link
Member

👍 obfuscation of the body will surely be also very different from obfuscating a header/parameter implementation-wise

@whiskeysierra
Copy link
Collaborator Author

Any idea whether this is safe to use as a wallpaper?
https://en.wikipedia.org/wiki/File:Grand_Turk(34).jpg


import javax.servlet.ServletRequest;

interface Markable extends Named {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the need for having this as an interface with default methods instead of protected methods in OnceFilter. Default methods are nice if a subclass might have a more optimized implementation, but here it just makes the code harder to follow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but here it just makes the code harder to follow.

I inlined most of them now (not pushed yet).

<version>1.7.12</version>
</dependency>
<dependency>
<groupId>org.projectlombok</groupId>
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you using lombok just for one SneakyThrows or did I miss any more usages?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are you using lombok just for one SneakyThrows or did I miss any more usages?

This is the only usage right here. I haven't check if it actually helps to get full coverage.

lines.add(formatRequestLine(request));
lines.addAll(formatHeaders(request.getHeaders()));

final String body = request.getBodyAsString();
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to extract lines 45-51 as they're equal to 66-72?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

whiskeysierra added a commit that referenced this pull request Oct 6, 2015
Refactored library completely
@whiskeysierra whiskeysierra merged commit 03af156 into zalando:master Oct 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants