Breaking change from v0.9.0 -> v0.9.+ exposes private properties
kmkipta opened this issue · 4 comments
There was a recent breaking change coming through in a patch version increment that exposed some previously private properties on the RocksetClient
class and made them public - without proper getters/setters - and this has somewhat obscured the code and complicated unit testing - specifically mocking calls with something like JUnit.
For example, after we build a RocksetClient
we need to call this new queries
object, but as a client user, I am wondering why this change was needed.
// valid code for version 0.9.0
final var result = rocksetClient.query(queryRequest).getResults();
// valid code version 0.9.1 +
final var result = rocksetClient.queries.query(queryRequest).getResults();
Hello, this change was made for 2 reasons:
- To be more consistent with all our other clients (JavaScript and Python).
- To reduce the amount of handwritten code in the repo, and make everything purely codegenned.
Thanks for the reply @joe-el-khoury ! Using your generation tools, have you considered of adding setters/getters around these properties?
My immediate experience is as follows using JUnit and Mockito mocks (in my unit tests I don't actually want to reach out to Rockset, so I am mocking the client):
// works in 0.9.0
verify(rocksetClient, times(1)).query(queryRequest);
Now, if I upgrade the client to v0.9.+, I have this rocksetClient.queries.query(...)
, and I have to mock this queries
object, but I do not know how to set it so I can properly mock it.
Hi @kmkipta. From my understanding, it would be sufficient for you if we expose getters and setters for these fields: https://github.com/rockset/rockset-java-client/blob/master/src/main/java/com/rockset/client/RocksetClient.java#L9-L21?
You would then be able to set up your tests with something like
QueriesApi mockQueriesApi = mock(QueriesApi.class);
rocksetClient.setQueriesApi(mockQueriesApi);
// Do stuff...
verify(mockQueriesApi, times(1)).query(queryRequest);
Is this correct?
Hey @joe-el-khoury - yes I believe that will be fine to stub the class with a mocked query for basic unit tests