Apache
/
lucene
mirror of https://github.com/apache/lucene.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

SOLR-11722: Improve the v2/v1 API mapping, including a bug. 

Wrapped getParameterNamesIterator failed to consider attrToParams.
(committing this separately from rest)
This commit is contained in:
David Smiley 2018-01-25 14:23:44 -05:00
parent 09f903ef8d
commit d8e9ab8785
4 changed files with 102 additions and 58 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
solr/core/src/java/org/apache/solr/api/ApiBag.java
View File

@ -305,7 +305,12 @@ public class ApiBag {
continue;
} else {
List<String> errs = validator.validateJson(cmd.getCommandData());
if (errs != null) for (String err : errs) cmd.addError(err);
if (errs != null){
// otherwise swallowed in solrj tests, and just get "Error in command payload" in test log
// which is quite unhelpful.
log.error("Command errors for {}:{}", cmd.name, errs );
for (String err : errs) cmd.addError(err);
}
}
}

11
solr/core/src/java/org/apache/solr/handler/admin/BaseHandlerApiSupport.java
View File

@ -119,7 +119,7 @@ public abstract class BaseHandlerApiSupport implements ApiSupport {
} catch (SolrException e) {
throw e;
} catch (Exception e) {
throw new SolrException(BAD_REQUEST, e);
throw new SolrException(BAD_REQUEST, e); //TODO BAD_REQUEST is a wild guess; should we flip the default? fail here to investigate how this happens in tests
} finally {
req.setParams(params);
}
@ -129,6 +129,9 @@ public abstract class BaseHandlerApiSupport implements ApiSupport {
}
/**
* Wrapper for SolrParams that wraps V2 params and exposes them as V1 params.
*/
private static void wrapParams(final SolrQueryRequest req, final CommandOperation co, final ApiCommand cmd, final boolean useRequestParams) {
final Map<String, String> pathValues = req.getPathTemplateValues();
final Map<String, Object> map = co == null || !(co.getCommandData() instanceof Map) ?
@ -148,7 +151,7 @@ public abstract class BaseHandlerApiSupport implements ApiSupport {
}
private Object getParams0(String param) {
param = cmd.meta().getParamSubstitute(param);
param = cmd.meta().getParamSubstitute(param); // v1 -> v2, possibly dotted path
Object o = param.indexOf('.') > 0 ?
Utils.getObjectByPath(map, true, splitSmart(param, '.')) :
map.get(param);
@ -172,11 +175,9 @@ public abstract class BaseHandlerApiSupport implements ApiSupport {
@Override
public Iterator<String> getParameterNamesIterator() {
return cmd.meta().getParamNames(co).iterator();
return cmd.meta().getParamNamesIterator(co);
}
});
}

120
solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionApiMapping.java
View File

@ -21,8 +21,11 @@ package org.apache.solr.client.solrj.request;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.stream.Stream;
import org.apache.solr.client.solrj.SolrRequest;
import org.apache.solr.common.params.CollectionParams.CollectionAction;
@ -77,7 +80,7 @@ public class CollectionApiMapping {
"createNodeSet.shuffle", "shuffleNodes",
"createNodeSet", "nodeSet"
),
Utils.makeMap("properties.", "property.")),
Utils.makeMap("property.", "properties.")),
DELETE_COLL(EndPoint.PER_COLLECTION_DELETE,
DELETE,
@ -124,7 +127,7 @@ public class CollectionApiMapping {
CREATESHARD,
"create",
Utils.makeMap("createNodeSet", "nodeSet"),
Utils.makeMap("coreProperties.", "property.")) {
Utils.makeMap("property.", "coreProperties.")) {
@Override
public String getParamSubstitute(String param) {
return super.getParamSubstitute(param);
@ -137,7 +140,7 @@ public class CollectionApiMapping {
"split",
Utils.makeMap(
"split.key", "splitKey"),
Utils.makeMap("coreProperties.", "property.")),
Utils.makeMap("property.", "coreProperties.")),
DELETE_SHARD(PER_COLLECTION_PER_SHARD_DELETE,
DELETE, DELETESHARD),
@ -146,7 +149,7 @@ public class CollectionApiMapping {
ADDREPLICA,
"add-replica",
null,
Utils.makeMap("coreProperties.", "property.")),
Utils.makeMap("property.", "coreProperties.")),
DELETE_REPLICA(PER_COLLECTION_PER_SHARD_PER_REPLICA_DELETE,
DELETE, DELETEREPLICA),
@ -203,12 +206,14 @@ public class CollectionApiMapping {
public final String commandName;
public final EndPoint endPoint;
public final SolrRequest.METHOD method;
//mapping of http param name to json attribute
public final Map<String, String> paramstoAttr;
//mapping of old prefix to new for instance properties.a=val can be substituted with property:{a:val}
public final Map<String, String> prefixSubstitutes;
public final CollectionAction action;
//bi-directional mapping of v1 http param name to v2 json attribute
public final Map<String, String> paramsToAttrs; // v1 -> v2
public final Map<String, String> attrsToParams; // v2 -> v1
//mapping of old prefix to new for instance properties.a=val can be substituted with property:{a:val}
public final Map<String, String> prefixParamsToAttrs; // v1 -> v2
public SolrRequest.METHOD getMethod() {
return method;
}
@ -219,20 +224,33 @@ public class CollectionApiMapping {
}
Meta(EndPoint endPoint, SolrRequest.METHOD method, CollectionAction action,
String commandName, Map paramstoAttr) {
this(endPoint, method, action, commandName, paramstoAttr, Collections.EMPTY_MAP);
String commandName, Map paramsToAttrs) {
this(endPoint, method, action, commandName, paramsToAttrs, Collections.emptyMap());
}
// lame... the Maps aren't typed simply because callers want to use Utils.makeMap which yields object vals
@SuppressWarnings("unchecked")
Meta(EndPoint endPoint, SolrRequest.METHOD method, CollectionAction action,
String commandName, Map paramstoAttr, Map prefixSubstitutes) {
String commandName, Map paramsToAttrs, Map prefixParamsToAttrs) {
this.action = action;
this.commandName = commandName;
this.endPoint = endPoint;
this.method = method;
this.paramstoAttr = paramstoAttr == null ? Collections.EMPTY_MAP : Collections.unmodifiableMap(paramstoAttr);
this.prefixSubstitutes = Collections.unmodifiableMap(prefixSubstitutes);
this.paramsToAttrs = paramsToAttrs == null ? Collections.emptyMap() : Collections.unmodifiableMap(paramsToAttrs);
this.attrsToParams = Collections.unmodifiableMap(reverseMap(this.paramsToAttrs));
this.prefixParamsToAttrs = prefixParamsToAttrs == null ? Collections.emptyMap() : Collections.unmodifiableMap(prefixParamsToAttrs);
}
private static Map<String, String> reverseMap(Map<String, String> input) { // swap keys and values
Map<String, String> attrToParams = new HashMap<>(input.size());
for (Map.Entry<String, String> entry :input.entrySet()) {
final String existing = attrToParams.put(entry.getValue(), entry.getKey());
if (existing != null) {
throw new IllegalArgumentException("keys and values must collectively be unique");
}
}
return attrToParams;
}
@Override
@ -250,49 +268,52 @@ public class CollectionApiMapping {
return endPoint;
}
// Returns iterator of v1 "params".
@Override
public Collection<String> getParamNames(CommandOperation op) {
public Iterator<String> getParamNamesIterator(CommandOperation op) {
Collection<String> paramNames = getParamNames_(op, this);
if (!prefixSubstitutes.isEmpty()) {
Collection<String> result = new ArrayList<>(paramNames.size());
for (Map.Entry<String, String> e : prefixSubstitutes.entrySet()) {
for (String paramName : paramNames) {
if (paramName.startsWith(e.getKey())) {
result.add(paramName.replace(e.getKey(), e.getValue()));
} else {
result.add(paramName);
Stream<String> pStream = paramNames.stream();
if (!attrsToParams.isEmpty()) {
pStream = pStream.map(paramName -> attrsToParams.getOrDefault(paramName, paramName));
}
if (!prefixParamsToAttrs.isEmpty()) {
pStream = pStream.map(paramName -> {
for (Map.Entry<String, String> e : prefixParamsToAttrs.entrySet()) {
final String prefixV1 = e.getKey();
final String prefixV2 = e.getValue();
if (paramName.startsWith(prefixV2)) {
return prefixV1 + paramName.substring(prefixV2.length()); // replace
}
}
paramNames = result;
}
return paramName;
});
}
return paramNames;
return pStream.iterator();
}
// returns params (v1) from an underlying v2, with param (v1) input
@Override
public String getParamSubstitute(String param) {
String s = paramstoAttr.containsKey(param) ? paramstoAttr.get(param) : param;
if (prefixSubstitutes != null) {
for (Map.Entry<String, String> e : prefixSubstitutes.entrySet()) {
if (s.startsWith(e.getValue())) return s.replace(e.getValue(), e.getKey());
public String getParamSubstitute(String param) {//input is v1
for (Map.Entry<String, String> e : prefixParamsToAttrs.entrySet()) {
final String prefixV1 = e.getKey();
final String prefixV2 = e.getValue();
if (param.startsWith(prefixV1)) {
return prefixV2 + param.substring(prefixV1.length()); // replace
}
}
return s;
return paramsToAttrs.getOrDefault(param, param);
}
public Object getReverseParamSubstitute(String param) {
String s = paramstoAttr.containsKey(param) ? paramstoAttr.get(param) : param;
if (prefixSubstitutes != null) {
for (Map.Entry<String, String> e : prefixSubstitutes.entrySet()) {
if(param.startsWith(e.getValue())){
return new Pair<>(e.getKey().substring(0, e.getKey().length() - 1), param.substring(e.getValue().length()));
}
// TODO document!
public Object getReverseParamSubstitute(String param) {//input is v1
for (Map.Entry<String, String> e : prefixParamsToAttrs.entrySet()) {
final String prefixV1 = e.getKey();
final String prefixV2 = e.getValue();
if (param.startsWith(prefixV1)) {
return new Pair<>(prefixV2.substring(0, prefixV2.length() - 1), param.substring(prefixV1.length()));
}
}
return s;
return paramsToAttrs.getOrDefault(param, param);
}
}
@ -386,14 +407,15 @@ public class CollectionApiMapping {
private static Collection<String> getParamNames_(CommandOperation op, CommandMeta command) {
List<String> result = new ArrayList<>();
Object o = op.getCommandData();
if (o instanceof Map) {
Map map = (Map) o;
List<String> result = new ArrayList<>();
collectKeyNames(map, result, "");
return result;
} else {
return Collections.emptySet();
}
return result;
}
public static void collectKeyNames(Map<String, Object> map, List<String> result, String prefix) {
@ -415,11 +437,11 @@ public class CollectionApiMapping {
V2EndPoint getEndPoint();
default Collection<String> getParamNames(CommandOperation op) {
return getParamNames_(op, CommandMeta.this);
default Iterator<String> getParamNamesIterator(CommandOperation op) {
return getParamNames_(op, CommandMeta.this).iterator();
}
/** Given a v1 param, return the v2 attribute (possibly a dotted path). */
default String getParamSubstitute(String name) {
return name;
}

22
solr/solrj/src/java/org/apache/solr/common/util/JsonSchemaValidator.java
View File

@ -18,6 +18,7 @@
package org.apache.solr.common.util;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedList;
@ -226,12 +227,27 @@ class RequiredValidator extends Validator<List<String>> {
@Override
boolean validate(Object o, List<String> errs) {
return validate(o,errs,requiredProps);
}
boolean validate( Object o, List<String> errs, Set<String> requiredProps) {
if (o instanceof Map) {
Set fnames = ((Map) o).keySet();
for (String requiredProp : requiredProps) {
if (!fnames.contains(requiredProp)) {
errs.add("Missing required attribute '" + requiredProp + "' in object " + Utils.toJSONString(o));
return false;
if (requiredProp.contains(".")) {
if (requiredProp.endsWith(".")) {
errs.add("Illegal required attribute name (ends with '.': " + requiredProp + "). This is a bug.");
return false;
}
String subprop = requiredProp.substring(requiredProp.indexOf(".") + 1);
if (!validate(((Map)o).get(requiredProp), errs, Collections.singleton(subprop))) {
return false;
}
} else {
if (!fnames.contains(requiredProp)) {
errs.add("Missing required attribute '" + requiredProp + "' in object " + Utils.toJSONString(o));
return false;
}
}
}
return true;