rockset/rockset-java-client

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