Edit to make entirely customizable ContentCachingRequestWrapper and ContentCachingInputStream
Comment From: pivotal-cla
@Roberto-Gentili Please sign the Contributor License Agreement!
Click here to manually synchronize the status of this Pull Request.
See the FAQ for frequently asked questions.
Comment From: pivotal-cla
@Roberto-Gentili Thank you for signing the Contributor License Agreement!
Comment From: sbrannen
Hi @Roberto-Gentili,
Please provide the rationale for the proposed changes.
In other words, what is the concrete use case that you are attempting to address that was not supported prior to the proposed changes.
In addition, please keep in mind that such changes are generally rejected without a detailed rationale, proper documentation changes/updates, and tests that fail prior to the changes and pass after the changes.
Comment From: Roberto-Gentili
Hi @Roberto-Gentili,
Please provide the rationale for the proposed changes.
In other words, what is the concrete use case that you are attempting to address that was not supported prior to the proposed changes.
In addition, please keep in mind that such changes are generally rejected without a detailed rationale, proper documentation changes/updates, and tests that fail prior to the changes and pass after the changes.
I created a HandlerInterceptor and in this interceptor I used a ContentCachingRequestWrapper (injected through a FilterRegistrationBean) to fetch the body content by calling the getContentAsByteArray method which returns no content because the InputStream has not been read. In this interceptor I can't consume the InputStream returned by the getInputStream method because if I do, later intervening Spring components will throw exception because the stream has been consumed. I also tryed by adding a hook with setReadListener but on Undertow an exception is thrown when the listener is called. In this situation I solved by copying the source of this class and overriding the read methods that advise the interceptor when no bytes are available in the stream.
The version I proposed in this pull request does not contain these changes but only contains the changes necessary to make these components fully customizable so that no one has to copy the source of this class to be able to extend it:
It's really bad having to copy a whole class to override it because it contains private methods and you can't change its behavior
Comment From: bclozel
Thanks @Roberto-Gentili but I'm going to decline this change. The classes considered here are being used for selected use cases supported by Spring Framework and not meant for others, unspecified behaviors.
While copying the content of this class in not practical, we believe that opening this class for extension will make maintenance harder and prevent us from applying future optimizations and changes as it would break existing code out there.
Thanks for your contribution!
Comment From: Roberto-Gentili
Thanks @Roberto-Gentili but I'm going to decline this change. The classes considered here are being used for selected use cases supported by Spring Framework and not meant for others, unspecified behaviors.
While copying the content of this class in not practical, we believe that opening this class for extension will make maintenance harder and prevent us from applying future optimizations and changes as it would break existing code out there.
Thanks for your contribution!
The version of the class I provided also fixed the concurrency problem it suffers from. I think your opinion is dirty and that it makes this class crippled and almost useless
Comment From: bclozel
The version of the class I provided also fixed the concurrency problem it suffers from. I think your opinion is dirty and that it makes this class crippled and almost useless
This kind of comment is unprofessional and is not likely to change our mind. If this class doesn't fit with your use case in many ways, this is probably a sign that it's not meant to be.
Now this is the first time you're mentioning a potential concurrency issue with the current implementation. If you believe there is a concurrency issue with the current implementation and for the use case it's built for (so not in the context of your custom extension), please provide a sample application that demonstrate the problem.
Comment From: Roberto-Gentili
The version of the class I provided also fixed the concurrency problem it suffers from. I think your opinion is dirty and that it makes this class crippled and almost useless
This kind of comment is unprofessional and is not likely to change our mind. If this class doesn't fit with your use case in many ways, this is probably a sign that it's not meant to be.
Now this is the first time you're mentioning a potential concurrency issue with the current implementation. If you believe there is a concurrency issue with the current implementation and for the use case it's built for (so not in the context of your custom extension), please provide a sample application that demonstrate the problem.
Your class is poorly designed and has several limitations as well as flaws: for example, creating an interceptor on top of everything and reading from the inputstream obtained by the getInputStream method causes exceptions in subsequent spring components.
I am the creator of Burningwave Core and of the JVM Driver and I know very well how to design a DI framework. I have already spent enough time refacotoring your component and following your procedures to make the improved component available, only to see me get responses without constructive logic. So I won't waste any more time demonstrating all the problems this class suffers from: it's your job.
Good work
Comment From: Roberto-Gentili
P.S.: even a novice can see that the lack of double checking in the getInputStream method can cause a concurrency problem