Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the Java client’s browse/search request models by extracting shared request parameters into a common base class, reducing duplication between BrowseRequest and SearchRequest.
Changes:
- Introduces
BrowseSearchBaseRequestto hold common browse/search parameters (pagination, facets, sorting, etc.). - Updates
SearchRequestto extend the new base request and removes duplicated fields/accessors. - Updates
BrowseRequestto extend the new base request and removes duplicated fields/accessors.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| constructorio-client/src/main/java/io/constructor/client/SearchRequest.java | Switches search request model to inherit shared parameters from the new base request. |
| constructorio-client/src/main/java/io/constructor/client/BrowseSearchBaseRequest.java | Adds the new shared base request containing common browse/search fields and accessors. |
| constructorio-client/src/main/java/io/constructor/client/BrowseRequest.java | Switches browse request model to inherit shared parameters from the new base request. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
af7ed1c to
13896bd
Compare
Alexey-Pavlov
left a comment
Alexey-Pavlov
left a comment
There was a problem hiding this comment.
LGTM! Left a few comments there and there
| /** | ||
| * Object with common request fields of search and browse | ||
| * | ||
| */ |
There was a problem hiding this comment.
It looks like mvn spotless:check fails on this file
How about running mvn spotless:apply and committing the result? For this spot, it gives:
| /** | |
| * Object with common request fields of search and browse | |
| * | |
| */ | |
| /** Object with common request fields of search and browse */ |
| /** | ||
| * @param filterMatchTypes the filterMatchTypes to set. Please refer to the relevant Search/Browse API endpoint documentation for details. | ||
| * Possible values 'all', 'any' or 'none' | ||
| */ |
There was a problem hiding this comment.
This is the second spot spotless flags (see my comment on the constructor's Javadoc). While it's being rewrapped, I think it's worth keeping a link here too.
What do you think about linking both pages? That keeps it endpoint-agnostic, as Copilot's comment asked, without losing the pointer. Formatted with spotless:apply:
| /** | |
| * @param filterMatchTypes the filterMatchTypes to set. Please refer to the relevant Search/Browse API endpoint documentation for details. | |
| * Possible values 'all', 'any' or 'none' | |
| */ | |
| /** | |
| * @param filterMatchTypes the filterMatchTypes to set. Please refer to | |
| * https://docs.constructor.com/reference/v1-search-get-search-results or | |
| * https://docs.constructor.com/reference/v1-browse-get-browse-results for details. Possible | |
| * values 'all', 'any' or 'none' | |
| */ |
| * @param query the term to return search results for | ||
| */ | ||
| public SearchRequest(String query) throws IllegalArgumentException { | ||
| super(); |
There was a problem hiding this comment.
Nitpick: the explicit super(); is what the compiler inserts anyway when a constructor doesn't start with a this(…) or super(…) call, and the other subclass constructor in this package, QuizRequest(String quizId), only spells it out because it passes super(quizId). How about dropping it here and in BrowseRequest (same note there)?
| super(); |
| * @param filterValue filter value to display results from | ||
| */ | ||
| public BrowseRequest(String filterName, String filterValue) throws IllegalArgumentException { | ||
| super(); |
There was a problem hiding this comment.
Nitpick, same as in SearchRequest: this super(); is what the compiler inserts anyway when a constructor doesn't start with a this(…) or super(…) call. How about dropping it?
| import java.util.Map; | ||
|
|
||
| /** Constructor.io base Search/Browse Request */ | ||
| public class BrowseSearchBaseRequest { |
There was a problem hiding this comment.
I think this class wants to be abstract. Right now BrowseSearchBaseRequest is a concrete public class with a public no-arg constructor, so new BrowseSearchBaseRequest() compiles, but there's nothing you can do with that object: every ConstructorIO entry point takes the concrete type, search(SearchRequest req, …) or browse(BrowseRequest req, …), and the base class has no required parameter of its own. It only exists to be extended.
How about abstract with a protected constructor (next comment)? Then the compiler rules the direct instantiation out, and the public API gains a type to pass around rather than a type to build. QuizRequestBase is concrete, so this is a small departure from that precedent, but that class is at least accepted somewhere, by createQuizRequest(QuizRequestBase req, …), whereas nothing accepts this one.
| public class BrowseSearchBaseRequest { | |
| public abstract class BrowseSearchBaseRequest { |
| public BrowseSearchBaseRequest() { | ||
|
|
There was a problem hiding this comment.
Second half of the change from my comment on the class line: protected here means only the classes that extend this one, SearchRequest and BrowseRequest, can call this constructor, and they are the only ones that ever do
| public BrowseSearchBaseRequest() { | |
| protected BrowseSearchBaseRequest() { |
Closes issue #188
To be able to factorize code between search and browse common request parameters
I didn't want the object to be called only
BaseRequestso I choseBrowseSearchBaseRequestbut not totally convinced