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

SPR-16245 make it easy to manipulate payload #1609

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@jeffnelson
Contributor

jeffnelson commented Nov 29, 2017

No description provided.

@fmcejudo

another points of view and some improvements ;)

/**
* Extract the message payload.<p>Used by {@link #createMessage(HttpServletRequest, String, String)} in creating the payload portion of the message (only if {@link #isIncludePayload()} returns true)
*/
protected String getMessagePayload(HttpServletRequest request) {

This comment has been minimized.

@fmcejudo

fmcejudo Dec 13, 2017

this might be better being private encapsulation, as it is going to be used just in the current class. What do you think?

@fmcejudo

fmcejudo Dec 13, 2017

this might be better being private encapsulation, as it is going to be used just in the current class. What do you think?

This comment has been minimized.

@fmcejudo

fmcejudo Dec 13, 2017

This method, to work with Optional, it might have the signature
private Optional<String> getMessagePayload(HttpServletRequest request)

@fmcejudo

fmcejudo Dec 13, 2017

This method, to work with Optional, it might have the signature
private Optional<String> getMessagePayload(HttpServletRequest request)

This comment has been minimized.

@jeffnelson

jeffnelson Dec 13, 2017

Contributor

@fmcejudo - I think you missed the entire point of this PR/change if you suggest getMessagePayload be private. Putting the logic to get the payload into a protected method that can be overridden by child classes is what enables manipulation of the payload.

My use case for this is that I am logging the full payload, but need to sanitize/scrub secret values (passwords, shared secrets, auth keys, etc) so they aren't spit out into logs.

@jeffnelson

jeffnelson Dec 13, 2017

Contributor

@fmcejudo - I think you missed the entire point of this PR/change if you suggest getMessagePayload be private. Putting the logic to get the payload into a protected method that can be overridden by child classes is what enables manipulation of the payload.

My use case for this is that I am logging the full payload, but need to sanitize/scrub secret values (passwords, shared secrets, auth keys, etc) so they aren't spit out into logs.

*/
protected String getMessagePayload(HttpServletRequest request) {
ContentCachingRequestWrapper wrapper =
WebUtils.getNativeRequest(request, ContentCachingRequestWrapper.class);

This comment has been minimized.

@fmcejudo

fmcejudo Dec 13, 2017

From my point of view, to avoid nested indentation, this might be done like if(wrapper==null){ return null; } and keep coding the other use cases outside of the block.

@fmcejudo

fmcejudo Dec 13, 2017

From my point of view, to avoid nested indentation, this might be done like if(wrapper==null){ return null; } and keep coding the other use cases outside of the block.

This comment has been minimized.

@jeffnelson

jeffnelson Dec 13, 2017

Contributor

I don't disagree, but if you look closely, existing logic was simply copy/pasted to a protected method

@jeffnelson

jeffnelson Dec 13, 2017

Contributor

I don't disagree, but if you look closely, existing logic was simply copy/pasted to a protected method

This comment has been minimized.

@fmcejudo

fmcejudo Dec 14, 2017

alright, but in the other hand, "Try and leave this world a little better than you found it." - Robert Stephenson Smyth Baden-Powell, the father of scouting

@fmcejudo

fmcejudo Dec 14, 2017

alright, but in the other hand, "Try and leave this world a little better than you found it." - Robert Stephenson Smyth Baden-Powell, the father of scouting

}
msg.append(";payload=").append(payload);
}
String payload = getMessagePayload(request);

This comment has been minimized.

@fmcejudo

fmcejudo Dec 13, 2017

In order to make this more fluent and java 8 style, what about having this as an Optional? . so getMessagePayload(request).ifPresent(msg.append(";payload=")::append). Just as an idea

@fmcejudo

fmcejudo Dec 13, 2017

In order to make this more fluent and java 8 style, what about having this as an Optional? . so getMessagePayload(request).ifPresent(msg.append(";payload=")::append). Just as an idea

String payload = null;
if (wrapper != null) {
byte[] buf = wrapper.getContentAsByteArray();
if (buf.length > 0) {

This comment has been minimized.

@fmcejudo

fmcejudo Dec 13, 2017

Does it make sense to be less or equal to zero? would it be wrapper null in that case? if so, the this condition might be removed

@fmcejudo

fmcejudo Dec 13, 2017

Does it make sense to be less or equal to zero? would it be wrapper null in that case? if so, the this condition might be removed

This comment has been minimized.

@jeffnelson

jeffnelson Dec 13, 2017

Contributor

see above comment about existing logic being copy/pasted

@jeffnelson

jeffnelson Dec 13, 2017

Contributor

see above comment about existing logic being copy/pasted

rstoyanchev added a commit that referenced this pull request Jan 9, 2018

@rstoyanchev rstoyanchev self-assigned this Jan 9, 2018

@rstoyanchev

This comment has been minimized.

Show comment
Hide comment
@rstoyanchev

rstoyanchev Jan 9, 2018

Contributor

Merged.

Contributor

rstoyanchev commented Jan 9, 2018

Merged.

@rstoyanchev rstoyanchev closed this Jan 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment