Skip to content

Commit f9c50fa

Browse files
gerlowskijagerlowskija
committed
Replace HTTP-method string-handling with enum
A number of places in Solr referenced the HTTP method used to make the request, often inspecting the value as a string. This is needlessly brittle (e.g. case sensitivity issues). This commit fixes this problem by parsing the method value into an enum, which can then be used by later inspection.
1 parent 2ec6f38 commit f9c50fa

10 files changed

Lines changed: 113 additions & 53 deletions

File tree

‎solr/core/src/java/org/apache/solr/client/solrj/embedded/EmbeddedSolrServer.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@ public NamedList<Object> request(SolrRequest<?> request, String coreName)
171171
SolrQueryRequest req =
172172
_parser.buildRequestFrom(
173173
null, getParams(request), getContentStreams(request), request.getUserPrincipal());
174-
req.getContext().put("httpMethod", request.getMethod().name());
174+
req.getContext().put("httpMethod", request.getMethod());
175175
req.getContext().put(PATH, path);
176176
SolrQueryResponse resp = new SolrQueryResponse();
177177
handler.handleRequest(req, resp);
@@ -224,7 +224,7 @@ public NamedList<Object> request(SolrRequest<?> request, String coreName)
224224
.buildRequestFrom(
225225
core, params, getContentStreams(request), request.getUserPrincipal());
226226
req.getContext().put(PATH, path);
227-
req.getContext().put("httpMethod", request.getMethod().name());
227+
req.getContext().put("httpMethod", request.getMethod());
228228
SolrQueryResponse rsp = new SolrQueryResponse();
229229
SolrRequestInfo.setRequestInfo(new SolrRequestInfo(req, rsp));
230230

‎solr/core/src/java/org/apache/solr/handler/SchemaHandler.java‎

Lines changed: 27 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
import org.apache.solr.api.Api;
3737
import org.apache.solr.api.ApiBag;
3838
import org.apache.solr.api.JerseyResource;
39+
import org.apache.solr.client.solrj.SolrRequest;
3940
import org.apache.solr.common.SolrException;
4041
import org.apache.solr.common.params.MapSolrParams;
4142
import org.apache.solr.common.params.SolrParams;
@@ -77,26 +78,33 @@ public class SchemaHandler extends RequestHandlerBase
7778
@Override
7879
public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
7980
RequestHandlerUtils.setWt(req, JSON);
80-
String httpMethod = (String) req.getContext().get("httpMethod");
81-
if ("POST".equals(httpMethod)) {
82-
if (isImmutableConfigSet) {
83-
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "ConfigSet is immutable");
84-
}
85-
if (req.getContentStreams() == null) {
86-
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "no stream");
87-
}
88-
89-
try {
90-
List<Map<String, Object>> errs = new SchemaManager(req).performOperations();
91-
if (!errs.isEmpty())
92-
throw new ApiBag.ExceptionWithErrObject(
93-
SolrException.ErrorCode.BAD_REQUEST, "error processing commands", errs);
94-
} catch (IOException e) {
81+
final SolrRequest.METHOD httpMethod = (SolrRequest.METHOD) req.getContext().get("httpMethod");
82+
switch (httpMethod) {
83+
case POST:
84+
if (isImmutableConfigSet) {
85+
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "ConfigSet is immutable");
86+
}
87+
if (req.getContentStreams() == null) {
88+
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "no stream");
89+
}
90+
try {
91+
List<Map<String, Object>> errs = new SchemaManager(req).performOperations();
92+
if (!errs.isEmpty())
93+
throw new ApiBag.ExceptionWithErrObject(
94+
SolrException.ErrorCode.BAD_REQUEST, "error processing commands", errs);
95+
} catch (IOException e) {
96+
throw new SolrException(
97+
SolrException.ErrorCode.BAD_REQUEST,
98+
"Error reading input String " + e.getMessage(),
99+
e);
100+
}
101+
break;
102+
case GET:
103+
handleGET(req, rsp);
104+
break;
105+
default:
95106
throw new SolrException(
96-
SolrException.ErrorCode.BAD_REQUEST, "Error reading input String " + e.getMessage(), e);
97-
}
98-
} else {
99-
handleGET(req, rsp);
107+
SolrException.ErrorCode.BAD_REQUEST, "Unexpected HTTP method: " + httpMethod);
100108
}
101109
}
102110

‎solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java‎

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -131,24 +131,30 @@ public Lock getReloadLock() {
131131
public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
132132

133133
RequestHandlerUtils.setWt(req, CommonParams.JSON);
134-
String httpMethod = (String) req.getContext().get("httpMethod");
135-
Command command = new Command(req, rsp, httpMethod);
136-
if ("POST".equals(httpMethod)) {
137-
if (configEditing_disabled || isImmutableConfigSet) {
138-
final String reason =
139-
configEditing_disabled
140-
? "due to " + CONFIGSET_EDITING_DISABLED_ARG
141-
: "because ConfigSet is immutable";
134+
final var httpMethod = (SolrRequest.METHOD) req.getContext().get("httpMethod");
135+
Command command = new Command(req, rsp, httpMethod.name());
136+
switch (httpMethod) {
137+
case POST:
138+
if (configEditing_disabled || isImmutableConfigSet) {
139+
final String reason =
140+
configEditing_disabled
141+
? "due to " + CONFIGSET_EDITING_DISABLED_ARG
142+
: "because ConfigSet is immutable";
143+
throw new SolrException(
144+
SolrException.ErrorCode.FORBIDDEN, " solrconfig editing is not enabled " + reason);
145+
}
146+
try {
147+
command.handlePOST();
148+
} finally {
149+
RequestHandlerUtils.addExperimentalFormatWarning(rsp);
150+
}
151+
break;
152+
case GET:
153+
command.handleGET();
154+
break;
155+
default:
142156
throw new SolrException(
143-
SolrException.ErrorCode.FORBIDDEN, " solrconfig editing is not enabled " + reason);
144-
}
145-
try {
146-
command.handlePOST();
147-
} finally {
148-
RequestHandlerUtils.addExperimentalFormatWarning(rsp);
149-
}
150-
} else {
151-
command.handleGET();
157+
SolrException.ErrorCode.BAD_REQUEST, "Unexpected HTTP method: " + httpMethod);
152158
}
153159
}
154160

‎solr/core/src/java/org/apache/solr/handler/admin/SecurityConfHandler.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import org.apache.solr.api.Api;
3434
import org.apache.solr.api.ApiBag;
3535
import org.apache.solr.api.ApiBag.ReqHandlerToApi;
36+
import org.apache.solr.client.solrj.SolrRequest;
3637
import org.apache.solr.common.SolrException;
3738
import org.apache.solr.common.SpecProvider;
3839
import org.apache.solr.common.params.CommonParams;
@@ -81,12 +82,12 @@ public PermissionNameProvider.Name getPermissionName(AuthorizationContext ctx) {
8182
@Override
8283
public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
8384
RequestHandlerUtils.setWt(req, CommonParams.JSON);
84-
String httpMethod = (String) req.getContext().get("httpMethod");
85+
final var httpMethod = (SolrRequest.METHOD) req.getContext().get("httpMethod");
8586
String path = (String) req.getContext().get("path");
8687
String key = path.substring(path.lastIndexOf('/') + 1);
87-
if ("GET".equals(httpMethod)) {
88+
if (SolrRequest.METHOD.GET.equals(httpMethod)) {
8889
getConf(rsp, key);
89-
} else if ("POST".equals(httpMethod)) {
90+
} else if (SolrRequest.METHOD.POST.equals(httpMethod)) {
9091
Object plugin = getPlugin(key);
9192
doEdit(req, rsp, path, key, plugin);
9293
}

‎solr/core/src/java/org/apache/solr/request/SolrQueryRequest.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import java.util.Collections;
2525
import java.util.List;
2626
import java.util.Map;
27+
import org.apache.solr.client.solrj.SolrRequest;
2728
import org.apache.solr.cloud.CloudDescriptor;
2829
import org.apache.solr.common.params.CommonParams;
2930
import org.apache.solr.common.params.SolrParams;
@@ -165,7 +166,7 @@ default List<CommandOperation> getCommands(boolean validateInput) {
165166
}
166167

167168
default String getHttpMethod() {
168-
return (String) getContext().get("httpMethod");
169+
return ((SolrRequest.METHOD) getContext().get("httpMethod")).name();
169170
}
170171

171172
default HttpSolrCall getHttpSolrCall() {

‎solr/core/src/java/org/apache/solr/servlet/SolrRequestParsers.java‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@
4545
import org.apache.commons.io.input.CloseShieldInputStream;
4646
import org.apache.lucene.util.IOUtils;
4747
import org.apache.solr.api.V2HttpCall;
48+
import org.apache.solr.client.solrj.SolrRequest;
4849
import org.apache.solr.common.SolrException;
4950
import org.apache.solr.common.SolrException.ErrorCode;
5051
import org.apache.solr.common.params.CommonParams;
@@ -173,7 +174,11 @@ public SolrQueryRequest parse(SolrCore core, String path, HttpServletRequest req
173174
// Handlers and login will want to know the path. If it contains a ':'
174175
// the handler could use it for RESTful URLs
175176
sreq.getContext().put(PATH, RequestHandlers.normalize(path));
176-
sreq.getContext().put("httpMethod", req.getMethod());
177+
178+
final var methodStr = req.getMethod();
179+
final var parsedMethod =
180+
SolrRequest.METHOD.fromString(methodStr); // Throws error if method not recognized
181+
sreq.getContext().put("httpMethod", parsedMethod);
177182

178183
if (addHttpRequestToContext) {
179184
sreq.getContext().put("httpRequest", req);

‎solr/core/src/test/org/apache/solr/handler/admin/SecurityConfHandlerTest.java‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import java.util.List;
2525
import java.util.Map;
2626
import org.apache.solr.SolrTestCaseJ4;
27+
import org.apache.solr.client.solrj.SolrRequest;
2728
import org.apache.solr.common.params.ModifiableSolrParams;
2829
import org.apache.solr.common.util.CommandOperation;
2930
import org.apache.solr.common.util.ContentStreamBase;
@@ -44,7 +45,7 @@ public void testEdit() throws Exception {
4445
+ "'set-user':{ 'tom':'TomIsUberCool'}\n"
4546
+ "}";
4647
LocalSolrQueryRequest req = new LocalSolrQueryRequest(null, new ModifiableSolrParams());
47-
req.getContext().put("httpMethod", "POST");
48+
req.getContext().put("httpMethod", SolrRequest.METHOD.POST);
4849
req.getContext().put("path", "/admin/authentication");
4950
ContentStreamBase.ByteArrayStream o =
5051
new ContentStreamBase.ByteArrayStream(command.getBytes(StandardCharsets.UTF_8), "");
@@ -77,7 +78,7 @@ public void testEdit() throws Exception {
7778
+ "}";
7879

7980
req = new LocalSolrQueryRequest(null, new ModifiableSolrParams());
80-
req.getContext().put("httpMethod", "POST");
81+
req.getContext().put("httpMethod", SolrRequest.METHOD.POST);
8182
req.getContext().put("path", "/admin/authorization");
8283
o = new ContentStreamBase.ByteArrayStream(command.getBytes(StandardCharsets.UTF_8), "");
8384
req.setContentStreams(Collections.singletonList(o));
@@ -101,7 +102,7 @@ public void testEdit() throws Exception {
101102
+ " 'role': ['admin','dev']\n"
102103
+ " }}";
103104
req = new LocalSolrQueryRequest(null, new ModifiableSolrParams());
104-
req.getContext().put("httpMethod", "POST");
105+
req.getContext().put("httpMethod", SolrRequest.METHOD.POST);
105106
req.getContext().put("path", "/admin/authorization");
106107
o = new ContentStreamBase.ByteArrayStream(command.getBytes(StandardCharsets.UTF_8), "");
107108
req.setContentStreams(Collections.singletonList(o));
@@ -122,7 +123,7 @@ public void testEdit() throws Exception {
122123
+ " 'role': ['guest','admin']\n"
123124
+ " }}";
124125
req = new LocalSolrQueryRequest(null, new ModifiableSolrParams());
125-
req.getContext().put("httpMethod", "POST");
126+
req.getContext().put("httpMethod", SolrRequest.METHOD.POST);
126127
req.getContext().put("path", "/admin/authorization");
127128
o = new ContentStreamBase.ByteArrayStream(command.getBytes(StandardCharsets.UTF_8), "");
128129
req.setContentStreams(Collections.singletonList(o));
@@ -139,7 +140,7 @@ public void testEdit() throws Exception {
139140

140141
command = "{\n" + "delete-permission: 1,\n" + " set-user-role : { tom :null}\n" + "}";
141142
req = new LocalSolrQueryRequest(null, new ModifiableSolrParams());
142-
req.getContext().put("httpMethod", "POST");
143+
req.getContext().put("httpMethod", SolrRequest.METHOD.POST);
143144
req.getContext().put("path", "/admin/authorization");
144145
o = new ContentStreamBase.ByteArrayStream(command.getBytes(StandardCharsets.UTF_8), "");
145146
req.setContentStreams(Collections.singletonList(o));
@@ -163,7 +164,7 @@ public void testEdit() throws Exception {
163164
+ " 'role': 'admin'\n"
164165
+ " }}";
165166
req = new LocalSolrQueryRequest(null, new ModifiableSolrParams());
166-
req.getContext().put("httpMethod", "POST");
167+
req.getContext().put("httpMethod", SolrRequest.METHOD.POST);
167168
req.getContext().put("path", "/admin/authorization");
168169
o = new ContentStreamBase.ByteArrayStream(command.getBytes(StandardCharsets.UTF_8), "");
169170
req.setContentStreams(Collections.singletonList(o));
@@ -237,7 +238,7 @@ protected boolean persistConf(SecurityConfig props) {
237238
public String getStandardJson() throws Exception {
238239
String command = "{\n" + "'set-user': {'solr':'SolrRocks'}\n" + "}";
239240
LocalSolrQueryRequest req = new LocalSolrQueryRequest(null, new ModifiableSolrParams());
240-
req.getContext().put("httpMethod", "POST");
241+
req.getContext().put("httpMethod", SolrRequest.METHOD.POST);
241242
req.getContext().put("path", "/admin/authentication");
242243
ContentStreamBase.ByteArrayStream o =
243244
new ContentStreamBase.ByteArrayStream(command.getBytes(StandardCharsets.UTF_8), "");
@@ -249,7 +250,7 @@ public String getStandardJson() throws Exception {
249250
+ "'set-permission':{'name': 'security-edit', 'role': 'admin'}"
250251
+ "}";
251252
req = new LocalSolrQueryRequest(null, new ModifiableSolrParams());
252-
req.getContext().put("httpMethod", "POST");
253+
req.getContext().put("httpMethod", SolrRequest.METHOD.POST);
253254
req.getContext().put("path", "/admin/authorization");
254255
o = new ContentStreamBase.ByteArrayStream(command.getBytes(StandardCharsets.UTF_8), "");
255256
req.setContentStreams(Collections.singletonList(o));

‎solr/core/src/test/org/apache/solr/servlet/SolrRequestParserTest.java‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -263,6 +263,28 @@ public void testStandardParseParamsAndFillStreams() throws Exception {
263263
}
264264
}
265265

266+
@Test
267+
public void testReportsErrorForUnexpectedHttpMethod() throws Exception {
268+
final String getParams = "q=hello";
269+
HttpServletRequest request = getMock("/solr/select", "application/x-www-form-urlencoded", 0);
270+
when(request.getMethod()).thenReturn("UNEXPECTED");
271+
when(request.getQueryString()).thenReturn(getParams);
272+
273+
MultipartRequestParser multipart = new MultipartRequestParser(2048);
274+
RawRequestParser raw = new RawRequestParser();
275+
FormDataRequestParser formdata = new FormDataRequestParser(2048);
276+
StandardRequestParser standard = new StandardRequestParser(multipart, raw, formdata);
277+
278+
final SolrException thrown =
279+
expectThrows(
280+
SolrException.class,
281+
() -> {
282+
parser.parse(h.getCore(), "/select", request);
283+
});
284+
assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, thrown.code());
285+
assertEquals("Request contained unexpected HTTP method: UNEXPECTED", thrown.getMessage());
286+
}
287+
266288
static class ByteServletInputStream extends ServletInputStream {
267289
final BufferedInputStream in;
268290
final int len;

‎solr/modules/cross-dc/src/test/org/apache/solr/crossdc/handler/MirroringConfigSetsHandlerTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ private static SolrQueryRequest createRequest(
9191
List.of(new ContentStreamBase.ByteArrayStream(content, configSetName, "application/zip"));
9292
req.setContentStreams(streams);
9393
}
94-
req.getContext().put("httpMethod", method);
94+
req.getContext().put("httpMethod", SolrRequest.METHOD.fromString(method));
9595
return req;
9696
}
9797

‎solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,13 @@
2323
import java.util.Collections;
2424
import java.util.HashMap;
2525
import java.util.List;
26+
import java.util.Locale;
2627
import java.util.Map;
2728
import java.util.Set;
2829
import java.util.concurrent.TimeUnit;
2930
import org.apache.solr.client.solrj.impl.HttpSolrClientBase;
3031
import org.apache.solr.client.solrj.request.RequestWriter;
32+
import org.apache.solr.common.SolrException;
3133
import org.apache.solr.common.params.SolrParams;
3234
import org.apache.solr.common.util.ContentStream;
3335

@@ -48,9 +50,23 @@ public Principal getUserPrincipal() {
4850

4951
public enum METHOD {
5052
GET,
53+
HEAD,
5154
POST,
5255
PUT,
53-
DELETE
56+
DELETE;
57+
58+
/**
59+
* Returns the METHOD enum value matching the provided string, or 'null' if no match is found.
60+
*/
61+
public static METHOD fromString(String methodStr) {
62+
try {
63+
return METHOD.valueOf(methodStr.toUpperCase(Locale.ROOT));
64+
} catch (IllegalArgumentException e) {
65+
throw new SolrException(
66+
SolrException.ErrorCode.BAD_REQUEST,
67+
"Request contained unexpected HTTP method: " + methodStr);
68+
}
69+
}
5470
};
5571

5672
public enum ApiVersion {

0 commit comments

Comments
 (0)