Skip to content

Conversation

@kushagraThapar
Copy link
Member

Sync responses were not setting resourceResponse in parent class CosmosResponse

This was causing sync responses to throw NPE in a bunch of methods like
getRequestLatency()
getRequestCharge()
and almost every other method.


CosmosContainerResponse(CosmosAsyncContainerResponse response, CosmosDatabase database, CosmosClient client) {
super(response.getProperties());
super(response.resourceResponseWrapper, response.getProperties());
Copy link
Member

@kirankumarkolli kirankumarkolli Oct 29, 2019

Choose a reason for hiding this comment

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

Won't sending wrapper out break design abstractions?
Or what should be the right abstraction.

Copy link
Member Author

Choose a reason for hiding this comment

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

wrapper is already being used in all the public API classes as to fetch the properties. This won't break out the abstraction design.
This will just make sure to assign the wrapper in the parent class from the response object.

@kushagraThapar
Copy link
Member Author

/azp run java - cosmos - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@mbhaskar mbhaskar left a comment

Choose a reason for hiding this comment

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

Can we add tests for these?

@kushagraThapar
Copy link
Member Author

/azp run java - cosmos - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@mbhaskar mbhaskar left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM

@kushagraThapar kushagraThapar merged commit b49f88a into Azure:feature/cosmos/v4 Oct 30, 2019
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 this pull request may close these issues.

3 participants