dsmiley commented on code in PR #4101:
URL: https://github.com/apache/solr/pull/4101#discussion_r2801395397
##########
solr/test-framework/src/java/org/apache/solr/util/TestHarness.java:
##########
@@ -400,42 +399,41 @@ public class LocalRequestFactory {
public LocalRequestFactory() {}
/**
- * Creates a LocalSolrQueryRequest based on variable args; for historical
reasons, this method
+ * Creates a SolrQueryRequestBase based on variable args; for historical
reasons, this method
* has some peculiar behavior:
*
* <ul>
* <li>If there is a single arg, then it is treated as the "q" param,
and the
- * LocalSolrQueryRequest consists of that query string along with
"qt", "start", and
- * "rows" params (based on the qtype, start, and limit properties of
this factory) along
- * with any other default "args" set on this factory.
+ * SolrQueryRequestBase consists of that query string along with
"qt", "start", and "rows"
+ * params (based on the qtype, start, and limit properties of this
factory) along with any
+ * other default "args" set on this factory.
* <li>If there are multiple args, then there must be an even number of
them, and each pair of
- * args is used as a key=value param in the LocalSolrQueryRequest.
<b>NOTE: In this usage,
+ * args is used as a key=value param in the SolrQueryRequestBase.
<b>NOTE: In this usage,
* the "qtype", "start", "limit", and "args" properties of this
factory are ignored.</b>
* </ul>
*
* TODO: this isn't really safe in the presence of core reloads! Perhaps
the best we could do is
* increment the core reference count and decrement it in the request
close() method?
*/
@SuppressWarnings({"unchecked"})
- public LocalSolrQueryRequest makeRequest(String... q) {
+ public SolrQueryRequestBase makeRequest(String... q) {
+ args.computeIfAbsent("wt", k -> "xml");
Review Comment:
This is a side-effect, which is unfortunate (it pre-existed, I know). I
wonder if we could simply initialize args with it so we don't manipulate args
here.
##########
solr/test-framework/src/java/org/apache/solr/util/TestHarness.java:
##########
@@ -400,42 +399,41 @@ public class LocalRequestFactory {
public LocalRequestFactory() {}
/**
- * Creates a LocalSolrQueryRequest based on variable args; for historical
reasons, this method
+ * Creates a SolrQueryRequestBase based on variable args; for historical
reasons, this method
* has some peculiar behavior:
*
* <ul>
* <li>If there is a single arg, then it is treated as the "q" param,
and the
- * LocalSolrQueryRequest consists of that query string along with
"qt", "start", and
- * "rows" params (based on the qtype, start, and limit properties of
this factory) along
- * with any other default "args" set on this factory.
+ * SolrQueryRequestBase consists of that query string along with
"qt", "start", and "rows"
+ * params (based on the qtype, start, and limit properties of this
factory) along with any
+ * other default "args" set on this factory.
* <li>If there are multiple args, then there must be an even number of
them, and each pair of
- * args is used as a key=value param in the LocalSolrQueryRequest.
<b>NOTE: In this usage,
+ * args is used as a key=value param in the SolrQueryRequestBase.
<b>NOTE: In this usage,
* the "qtype", "start", "limit", and "args" properties of this
factory are ignored.</b>
* </ul>
*
* TODO: this isn't really safe in the presence of core reloads! Perhaps
the best we could do is
* increment the core reference count and decrement it in the request
close() method?
*/
@SuppressWarnings({"unchecked"})
- public LocalSolrQueryRequest makeRequest(String... q) {
+ public SolrQueryRequestBase makeRequest(String... q) {
+ args.computeIfAbsent("wt", k -> "xml");
if (q.length == 1) {
- args.computeIfAbsent("wt", k -> "xml");
- return new LocalSolrQueryRequest(
- TestHarness.this.getCore(), q[0], qtype, start, limit, args);
+ Map<String, String[]> map = new HashMap<>();
+ for (Map.Entry<String, String> e : args.entrySet()) {
+ map.put(e.getKey(), new String[] {e.getValue()});
+ }
+ if (q[0] != null) map.put(CommonParams.Q, new String[] {q[0]});
+ if (qtype != null) map.put(CommonParams.QT, new String[] {qtype});
+ map.put(CommonParams.START, new String[] {Integer.toString(start)});
+ map.put(CommonParams.ROWS, new String[] {Integer.toString(limit)});
+ return new SolrQueryRequestBase(TestHarness.this.getCore(), new
MultiMapSolrParams(map));
}
if (q.length % 2 != 0) {
throw new RuntimeException(
"The length of the string array (query arguments) needs to be
even");
}
- @SuppressWarnings({"rawtypes"})
- Map.Entry<String, String>[] entries = new NamedListEntry[q.length / 2];
- for (int i = 0; i < q.length; i += 2) {
- entries[i / 2] = new NamedListEntry<>(q[i], q[i + 1]);
- }
- @SuppressWarnings({"rawtypes"})
- NamedList nl = new NamedList(entries);
- if (nl.get("wt") == null) nl.add("wt", "xml");
- return new LocalSolrQueryRequest(TestHarness.this.getCore(), nl);
+ return new SolrQueryRequestBase(TestHarness.this.getCore(),
SolrTestCaseJ4.params(q));
Review Comment:
This change appears to ignore `args`. Don't we have to combine them? Like
to consider `wt`, for example.
See `org.apache.solr.common.params.SolrParams#wrapDefaults` to combine 2.
Consider taking this feedback holistically with my other comment in this
method -- produce a SolrParams of args. And then produce another SolrParams of
`q`. And combine with `wrapDefaults`, returning from this method in exactly
one spot.
--
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]
