Uh oh!
There was an error while loading. Please reload this page.
SOLR-18343: Require explicit HTTP methods for admin requests - #4861
SOLR-18343: Require explicit HTTP methods for admin requests#4861zhangxinyao88 wants to merge 4 commits into
Conversation
dsmiley
left a comment
There was a problem hiding this comment.
Thanks! Can you please write a changelog (use writeChangelog task) with type changed and title:
SolrJ v1 admin requests now use appropriate HTTP verbs -- mostly POST, some GET
| @Deprecated(since = "11.0") | ||
| public CollectionAdminRequest(CollectionAction action) { | ||
| this("/admin/collections", action); | ||
| this(METHOD.GET, action); |
There was a problem hiding this comment.
switch to POST as a default (a safer default)
There was a problem hiding this comment.
Fixed in df6ed4d. This constructor now defaults to POST.
| public CollectionAdminRequest(String path, CollectionAction action) { | ||
| super(METHOD.GET, path, SolrRequestType.ADMIN); | ||
| this.action = checkNotNull(CoreAdminParams.ACTION, action); | ||
| this(METHOD.GET, path, action); |
There was a problem hiding this comment.
Fixed in df6ed4d. This constructor now defaults to POST.
| @Deprecated(since = "11.0") | ||
| public AsyncCollectionAdminRequest(CollectionAction action) { | ||
| super(action); | ||
| this(METHOD.GET, action); |
There was a problem hiding this comment.
Fixed in df6ed4d. This constructor now defaults to POST.
| @Deprecated(since = "11.0") | ||
| public AsyncShardSpecificAdminRequest( | ||
| CollectionAction action, String collection, String shard) { | ||
| this(METHOD.GET, action, collection, shard); |
There was a problem hiding this comment.
Fixed in df6ed4d. This constructor now defaults to POST.
| */ | ||
| @Deprecated(since = "11.0") | ||
| public CollectionAdminRoleRequest(CollectionAction action, String node, String role) { | ||
| this(METHOD.GET, action, node, role); |
There was a problem hiding this comment.
Fixed in df6ed4d. This constructor now defaults to POST.
| @Deprecated(since = "11.0") | ||
| public CoreAdminRequest() { | ||
| super(METHOD.GET, "/admin/cores", SolrRequestType.ADMIN); | ||
| this(METHOD.GET); |
There was a problem hiding this comment.
Fixed in df6ed4d. This constructor now defaults to POST.
| @Deprecated(since = "11.0") | ||
| public CoreAdminRequest(String path) { | ||
| super(METHOD.GET, path, SolrRequestType.ADMIN); | ||
| this(METHOD.GET, path); |
There was a problem hiding this comment.
Fixed in df6ed4d. This constructor now defaults to POST.
There was a problem hiding this comment.
this is strange... why would these have changed?
There was a problem hiding this comment.
Good catch. After switching to POST, the action is no longer in the query string, so tracing was falling back to the HTTP method. I’ve updated it to use the parsed request params instead, which keeps the action names in these spans.
https://issues.apache.org/jira/browse/SOLR-18343
Description
SolrJ's multi-action admin request classes currently default to GET, including requests that modify server state.
Solution
This adds constructors that require an HTTP method and updates the Collection, Core, and ConfigSet admin requests to choose GET or POST explicitly. The existing GET-defaulting constructors remain available but are deprecated for compatibility.
Read-only requests use GET. Other admin actions use POST.
I used an AI coding assistant to audit the constructor call sites, make the mechanical edits, and run validation. I reviewed the final diff and the method chosen for each action.
Tests
./gradlew tidy:solr:core:ecjLintTest:solr:solrj:checkThe full
./gradlew check -x testreached the Antora documentation build, which failed because the local checkout path contains spaces.Checklist
main../gradlew check.