gerlowskija commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r2001444583
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/GenericSolrRequest.java:
##########
@@ -50,7 +50,7 @@ public GenericSolrRequest(METHOD m, String path) {
* @param params query parameter names and values for making this request.
*/
public GenericSolrRequest(METHOD m, String path, SolrParams params) {
- super(m, path);
+ super(m, path, SolrRequestType.UNSPECIFIED);
Review Comment:
> I could see defaulting to ADMIN only if the path starts with "/admin" and
otherwise throwing an IllegalArgumentException
+1 to the additional ctor so that callers can specify their own request-type
as desired, but I dislike the suggestion around conditionally defaulting to
"ADMIN", for two reasons.
1. Some built-in paths containing "admin" are emphatically NOT "ADMIN" APIs;
`/admin/security` being a particularly thorny example. And the assumption gets
even rockier when you start considering users who might want GenericSolrRequest
to hit their custom endpoints. The whole purpose of the class is to be generic
- making assumptions about the type of request (beyond "UNSPECIFIED") seems
shaky IMO.
2. One of the main purposes of this JIRA/PR is to get rid of path-inspection
and string comparison, which tends to be brittle. Let's not just shift it to a
different place!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
