-
Notifications
You must be signed in to change notification settings - Fork 21
fix: (odata-client) Move disable-buffer toggle from result object to request builder #850
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
Conversation
MatKuhr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
productive code largely LGTM, didn't look at tests yet. I like the non-breaking introduction of AutoClosable, even though it comes at the cost of some new API.
If we want to reduce new API a bit, we could consider adding executeOpen() to ODataRequestExecutable. That would have the downside that this is also on Batch, where then both execute and executeOpen behave the same (since changing execute to now start buffering would be a breaking change 🙁)
But maybe we can somewhere make clearer, that this solution not only buffers, but eagerly buffers. It's good for understanding the importance of having both, as you pointed out 😉
...rc/main/java/com/sap/cloud/sdk/datamodel/odata/client/request/ODataRequestResultFactory.java
Outdated
Show resolved
Hide resolved
...-client/src/main/java/com/sap/cloud/sdk/datamodel/odata/client/request/ODataRequestRead.java
Show resolved
Hide resolved
...rc/main/java/com/sap/cloud/sdk/datamodel/odata/client/request/ODataRequestResultFactory.java
Show resolved
Hide resolved
| public ODataRequestResultResource.Executable withoutResponseBuffering() | ||
| { | ||
| requestResultFactory = ODataRequestResultFactory.WITHOUT_BUFFER; | ||
| return httpClient -> (ODataRequestResultResource) this.execute(httpClient); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This effectively means that withoutResponseBuffering() will be the last method before executing, but I think that is an acceptable small limitation
MatKuhr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests LGTM 👍🏻
...t/src/test/java/com/sap/cloud/sdk/datamodel/odata/client/request/ODataFetchAsStreamTest.java
Outdated
Show resolved
Hide resolved
...t/src/test/java/com/sap/cloud/sdk/datamodel/odata/client/request/ODataFetchAsStreamTest.java
Show resolved
Hide resolved
rpanackal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reviewed low level api changes. Overall looks good to me.
Same concern as @MatKuhr on logging failed buffering of http entity.
That said, I haven't previously looked at this module deeply. So not sure if I missed anything.
MatKuhr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otherwise lgtm
Context
https://github.com/SAP/cloud-sdk-java-backlog/issues/481
Please provide a short description of what your change does and why it is needed.
Feature scope:
For Generic OData Client:
HttpResponseentity from lazy-buffered to `eagerly-buffered.disableBufferingHttpResponse()onODataRequestResultGenerictowithoutResponseBuffering()onODataRequestReadandODataRequestReadByKey.Definition of Done
Error handling created / updated & covered by the tests aboveDocumentation updated