Skip to content
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

optional forestName parameter on DocumentManager.search() #1196

Closed
ehennum opened this issue Mar 18, 2020 · 5 comments
Closed

optional forestName parameter on DocumentManager.search() #1196

ehennum opened this issue Mar 18, 2020 · 5 comments

Comments

@ehennum
Copy link
Contributor

ehennum commented Mar 18, 2020

This task requires declaring a new interface method and passing a new parameter down the call path.

The expectation for this parameter arises from a similar search() overload in QueryManager:

https://github.com/marklogic/java-client-api/blob/develop/marklogic-client-api/src/main/java/com/marklogic/client/query/QueryManager.java#L286

The steps of this task:

  1. In the DocumentManager interface, add a new search() overload method that takes all of the existing parameters but with an additional String forestName parameter as the last parameter:

https://github.com/marklogic/java-client-api/blob/develop/marklogic-client-api/src/main/java/com/marklogic/client/document/DocumentManager.java#L675

Provide JavaDoc for this new interface method by copying the JavaDoc for an existing search() overload and adding documentation for the forestName parameter that's equivalent to the documentation of the forestName parameter on the similar QueryManager.search() method.

  1. In the DocumentManagerImpl class, modify the private search() method to take an additional String forestName parameter as the last parameter:

https://github.com/marklogic/java-client-api/blob/develop/marklogic-client-api/src/main/java/com/marklogic/client/impl/DocumentManagerImpl.java#L517

Add the @OverRide implementation of the new search() overload method after the last existing @OverRide search() overload method and before the private search() method, returning the result of a call that passes all parameters through to the private search() method.

Modify all calls from the existing @OverRide search() overload methods to the private search() method to pass a final null value as the forestName parameter.

At current line 541, modify the call to getBulkDocuments() to pass the forestName parameter after the null value:

https://github.com/marklogic/java-client-api/blob/develop/marklogic-client-api/src/main/java/com/marklogic/client/impl/DocumentManagerImpl.java#L541

  1. In the RESTServices interface, modify the getBulkDocuments() interface method to add a String forestName parameter as the last parameter:

https://github.com/marklogic/java-client-api/blob/develop/marklogic-client-api/src/main/java/com/marklogic/client/impl/RESTServices.java#L147

  1. In the OkHttpServices class, modify the @OverRide getBulkDocuments() implementation method to add a String forestName parameter as the last parameter:

https://github.com/marklogic/java-client-api/blob/develop/marklogic-client-api/src/main/java/com/marklogic/client/impl/OkHttpServices.java#L945

At current line 957, modify the call to getBulkDocumentsImpl() to add the forestName parameter as the last parameter:

https://github.com/marklogic/java-client-api/blob/develop/marklogic-client-api/src/main/java/com/marklogic/client/impl/OkHttpServices.java#L957

Modify the getBulkDocumentsImpl() method to add an additional String forestName parameter as the last parameter:

https://github.com/marklogic/java-client-api/blob/develop/marklogic-client-api/src/main/java/com/marklogic/client/impl/OkHttpServices.java#L1065

At current line 1093, replace the final null value with the forestName parameter:

https://github.com/marklogic/java-client-api/blob/develop/marklogic-client-api/src/main/java/com/marklogic/client/impl/OkHttpServices.java#L1093

@llinggit
Copy link
Contributor

Hi Erik, just wondering do we need unit test for this? Thanks.

@ehennum
Copy link
Contributor Author

ehennum commented Mar 24, 2020

That would be best. One possible approach:

  • Write some documents in a collection -- enough to make it very unlikely that all documents go in one forest -- maybe 10 * the number of forest

  • Iterate over the 3 forests of the unit test database, searching the collection in each forest

  • Assert that the total number of documents for all queries is correct, that at least 2 forest-specific queries retrieve documents, and that the result set for every forest-specific query that retrieves documents is disjunct.

  • Remove the documents in the collection via QueryManager.delete()

llinggit pushed a commit to llinggit/java-client-api that referenced this issue Mar 24, 2020
llinggit pushed a commit to llinggit/java-client-api that referenced this issue Mar 24, 2020
llinggit pushed a commit to llinggit/java-client-api that referenced this issue Mar 25, 2020
@llinggit llinggit added fix and removed new labels Mar 25, 2020
llinggit pushed a commit to llinggit/java-client-api that referenced this issue Mar 25, 2020
llinggit pushed a commit that referenced this issue Mar 25, 2020
llinggit pushed a commit that referenced this issue Mar 25, 2020
@llinggit llinggit assigned georgeajit and unassigned llinggit Mar 25, 2020
@llinggit llinggit added test and removed fix labels Mar 25, 2020
@llinggit
Copy link
Contributor

Hi Ajit, I'm giving this one to you now. I added a unit test for one search method.

@ehennum
Copy link
Contributor Author

ehennum commented Mar 25, 2020

@georgeajit , do you think the unit test provides sufficient coverage?

  • The code path hasn't changed. The call just passes an optional value instead of hard-coding a null.

  • The call where the optional value replaces the hard-coded null goes to a deeper, unchanged generateSearchRequest() function that is tested both by unit tests and functionally from other code paths as well.

@georgeajit
Copy link
Contributor

Added milestone.
No QA tests needed.

@ehennum ehennum closed this as completed Oct 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants