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

Externalize a handler to write json fields #878

Merged
merged 2 commits into from Nov 3, 2020

Conversation

rjmveloso
Copy link

Allows to contribute with a custom JsonFieldWriter to have full freedom on how to write headers and body (naming and ordering).


private final JsonFactory factory;

private JsonFieldWriter delegate = this; // default to this implementation
Copy link
Collaborator

Choose a reason for hiding this comment

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

By implementing this interface directly you're exposing that implementation detail to the outside world because FastJsonHttpLogFormatter is public. I'd hide that with a static inner class.

Copy link
Author

Choose a reason for hiding this comment

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

Completely agree with that. In fact I was thinking that I should have followed that approach from the beginning.

public String format(
final Precorrelation precorrelation,
final HttpRequest request) throws IOException {
public FastJsonHttpLogFormatter(final ObjectMapper mapper,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this constructor is not covered by tests. If you follow my approach from above you get that for free, because you would actually use it then.

@whiskeysierra
Copy link
Collaborator

👍

@whiskeysierra whiskeysierra merged commit 90eda0f into zalando:main Nov 3, 2020
@whiskeysierra
Copy link
Collaborator

I'm releasing this as 2.4.0 as we speak. Thanks for the contribution! 🎉

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

2 participants