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

Incorrect configuration for default Request and Response filters #669

Closed
srknkbs opened this issue Dec 16, 2019 · 7 comments · Fixed by #670
Closed

Incorrect configuration for default Request and Response filters #669

srknkbs opened this issue Dec 16, 2019 · 7 comments · Fixed by #670
Labels

Comments

@srknkbs
Copy link

srknkbs commented Dec 16, 2019

When we look at the spring boot starter section in logbook documentation it states that the default configuration for request and response filters are none():

RequestFilter   RequestFilter.none()
ResponseFilter   ResponseFilter.none()

However, when i look at the Logbook Auto Configuration class , they are configured differently.

@API(status = INTERNAL)
@Bean
@ConditionalOnMissingBean(RequestFilter.class)
public RequestFilter requestFilter() {
    return RequestFilters.defaultValue();
}

@API(status = INTERNAL)
@Bean
@ConditionalOnMissingBean(ResponseFilter.class)
public ResponseFilter responseFilter() {
    return ResponseFilters.defaultValue();
}

As a library user, i do not expect any filterfor the response and request object by default.

@whiskeysierra
Copy link
Collaborator

The code is actually correct. I opened #670 to fix it.

The default values make sense because they will either provide now additional benefit (logging binaries, multipart) or break applications (trying to log endless streams).

@srknkbs
Copy link
Author

srknkbs commented Dec 16, 2019

Logbook logs request/response to the log console by default. So it make sense to replace binaries or multipart files with static string valus (such as "multipart" or "binary") to prevent polluting the console. But if i persist logs to the database i need to persist original binaries and multipart files. So i can write my own filters to eliminate those filters.

Is my assumption correct?

@srknkbs
Copy link
Author

srknkbs commented Dec 16, 2019

I have another question. When i override RequestFilter with RequestFilter.none(), my requst body is null for multipart files. Before I had "multipart" string in the request body.

When i remove the default requestfilter, i expect original value in the request value.

How can i handle this situation?

@whiskeysierra
Copy link
Collaborator

But if i persist logs to the database i need to persist original binaries and multipart files. So i can write my own filters to eliminate those filters.

Is my assumption correct?

That is correct. Configure/Register RequestFilter.none() and ResponseFilter.none() respectively instead of the default.

When i remove the default requestfilter, i expect original value in the request value.

That's a reasonable expectation to have. Can you paste your request here or a minimal version of it? Maybe even produce a test case or demo project?

@srknkbs
Copy link
Author

srknkbs commented Dec 16, 2019

I am using postman form-data/file option to pass multipart bodies.

I debug the code and i found out that the issue is in RemoteRequest class.

The following line (line number 108) return empty byte array for multipart bodies.
new Buffering(toByteArray(request.getInputStream()));

When i use binary option (in postman) to pass one file (just to check the behaviour), i have proper byte array object.

@whiskeysierra
Copy link
Collaborator

The handling of multipart is always somewhat special. Is there any filter running before Logbook? Which component is actually parsing the multipart request body? The servlet container? Application framework?

@srknkbs
Copy link
Author

srknkbs commented Dec 17, 2019

Is there any filter running before Logbook?
Yes, secure logbook filter. But in my opinion it is not a problem,since secure logbook filter delays handling the request log.

Which component is actually parsing the multipart request body? The servlet container? Application framework?

It is application context.

When i call the request.getBody() in my Sink implementation, here is the stackstrace:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants