Ability to mark service parameter as optional for easy API evolving #28

jmix-projects · Aug 18, 2021 · f0cfee8 · f0cfee8
1 parent f2c71b9
commit f0cfee8
Show file tree

Hide file tree

Showing 8 changed files with 158 additions and 36 deletions.
diff --git a/rest/src/main/java/io/jmix/rest/impl/config/RestServicesConfiguration.java b/rest/src/main/java/io/jmix/rest/impl/config/RestServicesConfiguration.java
@@ -19,7 +19,7 @@
 import com.fasterxml.jackson.annotation.JsonIgnore;
 import io.jmix.core.Resources;
 import io.jmix.core.common.util.Dom4j;
-import org.apache.commons.collections4.CollectionUtils;
+import io.jmix.rest.exception.RestAPIException;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.text.StringTokenizer;
 import org.dom4j.Element;
@@ -30,6 +30,7 @@
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.core.env.Environment;
 import org.springframework.core.io.Resource;
+import org.springframework.http.HttpStatus;
 import org.springframework.stereotype.Component;
 import org.springframework.stereotype.Service;
 import org.springframework.util.ClassUtils;
@@ -86,12 +87,17 @@ public RestMethodInfo getRestMethodInfo(String serviceName, String methodName, S
             checkInitialized();
             RestServiceInfo restServiceInfo = serviceInfosMap.get(serviceName);
             if (restServiceInfo == null) return null;
-            return restServiceInfo.getMethods().stream()
+            List<RestMethodInfo> restMethodInfos = restServiceInfo.getMethods().stream()
                     .filter(restMethodInfo -> methodName.equals(restMethodInfo.getName())
                             && httpMethodMatches(restMethodInfo.getHttpMethod(), httpMethod)
                             && paramsMatches(restMethodInfo.getParams(), methodParamNames))
-                    .findFirst()
-                    .orElse(null);
+                    .collect(Collectors.toList());
+            if (restMethodInfos.size() > 1) {
+                String errorMsg = String.format("Cannot determine the service method to call. %d suitable methods have been found",
+                        restMethodInfos.size());
+                throw new RestAPIException(errorMsg, errorMsg, HttpStatus.BAD_REQUEST);
+            }
+            return restMethodInfos.size() == 1 ? restMethodInfos.get(0) : null;
         } finally {
             lock.readLock().unlock();
         }
@@ -105,11 +111,12 @@ protected boolean httpMethodMatches(@Nullable String httpMethod1, @Nullable Stri
     }
 
     protected boolean paramsMatches(List<RestMethodParamInfo> paramInfos, List<String> paramNames) {
-        if (paramInfos.size() != paramNames.size()) return false;
-        List<String> paramInfosNames = paramInfos.stream()
-                .map(RestMethodParamInfo::getName)
-                .collect(Collectors.toList());
-        return CollectionUtils.isEqualCollection(paramInfosNames, paramNames);
+        if (paramNames.size() > paramInfos.size()) {
+            return false;
+        }
+
+        return paramInfos.stream()
+                .noneMatch(paramInfo -> paramInfo.required && !paramNames.contains(paramInfo.name));
     }
 
     protected void checkInitialized() {
@@ -160,7 +167,9 @@ protected void loadConfig(Element rootElem) {
                 boolean anonymousAllowed = Boolean.parseBoolean(methodElem.attributeValue("anonymousAllowed"));
                 List<RestMethodParamInfo> params = new ArrayList<>();
                 for (Element paramEl : methodElem.elements("param")) {
-                    params.add(new RestMethodParamInfo(paramEl.attributeValue("name"), paramEl.attributeValue("type")));
+                    params.add(new RestMethodParamInfo(paramEl.attributeValue("name"),
+                            paramEl.attributeValue("type"),
+                            Boolean.parseBoolean(paramEl.attributeValue("required", "true"))));
                 }
                 Method method = _findMethod(serviceName, methodName, params);
                 if (method != null) {
@@ -211,11 +220,11 @@ protected Method _findMethod(String serviceName, String methodName, List<RestMet
 
         if (paramTypes.isEmpty()) {
             List<Method> appropriateMethods = new ArrayList<>();
-                for (Method method : methods) {
-                    if (methodName.equals(method.getName()) && method.getParameterTypes().length == paramInfos.size()) {
-                        appropriateMethods.add(method);
-                    }
+            for (Method method : methods) {
+                if (methodName.equals(method.getName()) && method.getParameterTypes().length == paramInfos.size()) {
+                    appropriateMethods.add(method);
                 }
+            }
             if (appropriateMethods.size() == 1) {
                 serviceMethod = appropriateMethods.get(0);
             } else if (appropriateMethods.size() > 1) {
@@ -227,10 +236,10 @@ protected Method _findMethod(String serviceName, String methodName, List<RestMet
                 return null;
             }
         } else {
-                try {
-                    serviceMethod = serviceClass.getMethod(methodName, paramTypes.toArray(new Class[0]));
-                } catch (NoSuchMethodException ignored) {
-                }
+            try {
+                serviceMethod = serviceClass.getMethod(methodName, paramTypes.toArray(new Class[0]));
+            } catch (NoSuchMethodException ignored) {
+            }
             if (serviceMethod == null) {
                 log.error("Method not found. Service: {}, method: {}, argument types: {}", serviceName, methodName, paramTypes);
                 return null;
@@ -336,10 +345,12 @@ public void setMethod(Method method) {
     public static class RestMethodParamInfo {
         protected String name;
         protected String type;
+        protected boolean required;
 
-        public RestMethodParamInfo(String name, String type) {
+        public RestMethodParamInfo(String name, String type, boolean required) {
             this.name = name;
             this.type = type;
+            this.required = required;
         }
 
         public String getName() {
@@ -357,5 +368,13 @@ public String getType() {
         public void setType(String type) {
             this.type = type;
         }
+
+        public boolean isRequired() {
+            return required;
+        }
+
+        public void setRequired(boolean required) {
+            this.required = required;
+        }
     }
 }
diff --git a/rest/src/main/java/io/jmix/rest/impl/service/ServicesControllerManager.java b/rest/src/main/java/io/jmix/rest/impl/service/ServicesControllerManager.java
@@ -16,18 +16,18 @@
 
 package io.jmix.rest.impl.service;
 
+import io.jmix.core.Entity;
 import io.jmix.core.EntitySerialization;
 import io.jmix.core.EntitySerializationOption;
-import io.jmix.core.Entity;
 import io.jmix.core.Metadata;
 import io.jmix.core.metamodel.datatype.Datatype;
 import io.jmix.core.metamodel.datatype.DatatypeRegistry;
 import io.jmix.core.metamodel.model.MetaClass;
+import io.jmix.rest.exception.RestAPIException;
 import io.jmix.rest.impl.RestControllerUtils;
 import io.jmix.rest.impl.RestParseUtils;
 import io.jmix.rest.impl.config.RestServicesConfiguration;
 import io.jmix.rest.impl.controller.ServicesController;
-import io.jmix.rest.exception.RestAPIException;
 import io.jmix.rest.transform.JsonTransformationDirection;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -137,7 +137,8 @@ protected ServiceCallResult _invokeServiceMethod(String serviceName,
             int idx = i;
             try {
                 idx = paramNames.indexOf(restMethodInfo.getParams().get(i).getName());
-                paramValues.add(restParseUtils.toObject(types[i], paramValuesStr.get(idx), modelVersion));
+                String valueStr = idx == -1 ? null : paramValuesStr.get(idx);
+                paramValues.add(restParseUtils.toObject(types[i], valueStr, modelVersion));
             } catch (Exception e) {
                 log.error("Error on parsing service param value", e);
                 throw new RestAPIException("Invalid parameter value",

diff --git a/rest/src/main/resources/io/jmix/rest/xsd/services.xsd b/rest/src/main/resources/io/jmix/rest/xsd/services.xsd
@@ -31,7 +31,7 @@
 
     <xs:complexType name="serviceType">
         <xs:sequence>
-            <xs:element name="method" type="methodType"  maxOccurs="unbounded" minOccurs="0"/>
+            <xs:element name="method" type="methodType" maxOccurs="unbounded" minOccurs="0"/>
         </xs:sequence>
         <xs:attribute name="name" type="xs:string" use="required"/>
     </xs:complexType>
@@ -47,5 +47,6 @@
     <xs:complexType name="methodParamType">
         <xs:attribute name="name" type="xs:string" use="required"/>
         <xs:attribute name="type" type="xs:string"/>
+        <xs:attribute name="required" type="xs:boolean" default="true"/>
     </xs:complexType>
 </xs:schema>
diff --git a/sample-rest/src/main/java/io/jmix/samples/rest/service/OtherRestTestService.java b/sample-rest/src/main/java/io/jmix/samples/rest/service/OtherRestTestService.java
@@ -16,19 +16,6 @@
 
 package io.jmix.samples.rest.service;
 
-import io.jmix.samples.rest.entity.driver.Car;
-import io.jmix.samples.rest.entity.driver.NotPersistentStringIdEntity;
-import org.springframework.validation.annotation.Validated;
-
-import javax.validation.constraints.NotNull;
-import javax.validation.constraints.Pattern;
-import java.math.BigDecimal;
-import java.text.ParseException;
-import java.util.Date;
-import java.util.List;
-import java.util.Map;
-import java.util.UUID;
-
 /**
  * Service is used in functional tests
  */

diff --git a/sample-rest/src/main/java/io/jmix/samples/rest/service/RestTestService.java b/sample-rest/src/main/java/io/jmix/samples/rest/service/RestTestService.java
@@ -100,4 +100,10 @@ public interface RestTestService {
     List<Map<String, Object>> methodReturnsListOfMap();
 
     NotPersistentStringIdEntity getNotPersistentStringIdEntity();
+
+    Map<String, String> methodWithOptionalArgs(String arg1, String arg2, String arg3);
+
+    String overloadedMethodWithOptionalArgs(String arg1);
+
+    String overloadedMethodWithOptionalArgs(String arg1, String arg2);
 }
diff --git a/sample-rest/src/main/java/io/jmix/samples/rest/service/RestTestServiceBean.java b/sample-rest/src/main/java/io/jmix/samples/rest/service/RestTestServiceBean.java
@@ -275,4 +275,23 @@ public NotPersistentStringIdEntity getNotPersistentStringIdEntity() {
         notPersistentStringIdEntity.setName("Bob");
         return notPersistentStringIdEntity;
     }
+
+    @Override
+    public Map<String, String> methodWithOptionalArgs(String arg1, String arg2, String arg3) {
+        Map<String, String> stringMap = new LinkedHashMap<>();
+        stringMap.put("arg1", arg1);
+        stringMap.put("arg2", arg2);
+        stringMap.put("arg3", arg3);
+        return stringMap;
+    }
+
+    @Override
+    public String overloadedMethodWithOptionalArgs(String arg1) {
+        return "one arg";
+    }
+
+    @Override
+    public String overloadedMethodWithOptionalArgs(String arg1, String arg2) {
+        return "two args";
+    }
 }
diff --git a/sample-rest/src/test/java/services/ServicesControllerFT.java b/sample-rest/src/test/java/services/ServicesControllerFT.java
@@ -75,6 +75,80 @@ public void serviceWithParamsGET() throws Exception {
         }
     }
 
+    @Test
+    public void serviceWithOptionalParamsGET() throws Exception {
+        //with 1 required
+        Map<String, String> params = new LinkedHashMap<>();
+        params.put("arg1", "1");
+        try (CloseableHttpResponse response = sendGet(baseUrl + "/services/" + RestTestService.NAME + "/methodWithOptionalArgs", oauthToken, params)) {
+            assertEquals(HttpStatus.SC_OK, statusCode(response));
+            ReadContext readContext = parseResponse(response);
+            assertEquals("1", readContext.read("$.arg1"));
+            assertThrows(PathNotFoundException.class, () -> readContext.read("$.arg2"));
+            assertThrows(PathNotFoundException.class, () -> readContext.read("$.arg3"));
+        }
+
+        //with 1 required and 1 optional
+        params.clear();
+        params.put("arg1", "1");
+        params.put("arg2", "2");
+        try (CloseableHttpResponse response = sendGet(baseUrl + "/services/" + RestTestService.NAME + "/methodWithOptionalArgs", oauthToken, params)) {
+            assertEquals(HttpStatus.SC_OK, statusCode(response));
+            ReadContext readContext = parseResponse(response);
+            assertEquals("1", readContext.read("$.arg1"));
+            assertEquals("2", readContext.read("$.arg2"));
+            assertThrows(PathNotFoundException.class, () -> readContext.read("$.arg3"));
+        }
+
+        //with 1 required and 1 optional
+        params.clear();
+        params.put("arg1", "1");
+        params.put("arg3", "3");
+        try (CloseableHttpResponse response = sendGet(baseUrl + "/services/" + RestTestService.NAME + "/methodWithOptionalArgs", oauthToken, params)) {
+            assertEquals(HttpStatus.SC_OK, statusCode(response));
+            ReadContext readContext = parseResponse(response);
+            assertEquals("1", readContext.read("$.arg1"));
+            assertThrows(PathNotFoundException.class, () -> readContext.read("$.arg2"));
+            assertEquals("3", readContext.read("$.arg3"));
+        }
+
+        //with 1 required and 2 optional
+        params.clear();
+        params.put("arg1", "1");
+        params.put("arg2", "2");
+        params.put("arg3", "3");
+        try (CloseableHttpResponse response = sendGet(baseUrl + "/services/" + RestTestService.NAME + "/methodWithOptionalArgs", oauthToken, params)) {
+            assertEquals(HttpStatus.SC_OK, statusCode(response));
+            ReadContext readContext = parseResponse(response);
+            assertEquals("1", readContext.read("$.arg1"));
+            assertEquals("2", readContext.read("$.arg2"));
+            assertEquals("3", readContext.read("$.arg3"));
+        }
+
+        //without params
+        params.clear();
+        try (CloseableHttpResponse response = sendGet(baseUrl + "/services/" + RestTestService.NAME + "/methodWithOptionalArgs", oauthToken, params)) {
+            assertEquals(HttpStatus.SC_NOT_FOUND, statusCode(response));
+            assertTrue(responseToString(response).contains("Service method not found"));
+        }
+
+        //1 suitable method
+        params.put("arg1", "1");
+        params.put("arg2", "2");
+        try (CloseableHttpResponse response = sendGet(baseUrl + "/services/" + RestTestService.NAME + "/overloadedMethodWithOptionalArgs", oauthToken, params)) {
+            assertEquals(HttpStatus.SC_OK, statusCode(response));
+            assertEquals("two args", responseToString(response));
+        }
+
+        //more than 1 suitable method
+        params.clear();
+        params.put("arg1", "1");
+        try (CloseableHttpResponse response = sendGet(baseUrl + "/services/" + RestTestService.NAME + "/overloadedMethodWithOptionalArgs", oauthToken, params)) {
+            assertEquals(HttpStatus.SC_BAD_REQUEST, statusCode(response));
+            assertTrue(responseToString(response).contains("Can not determine the service method"));
+        }
+    }
+
     @Test
     public void serviceWithJavaTimeParams() throws Exception {
         String requestBody = getFileContent("serviceWithJavaTimeParams.json", null);

diff --git a/sample-rest/src/test/resources/test_support/rest-services.xml b/sample-rest/src/test/resources/test_support/rest-services.xml
@@ -113,6 +113,21 @@
         <method name="getNotPersistentStringIdEntity"/>
 
         <method name="methodReturnsListOfMap"/>
+
+        <method name="methodWithOptionalArgs">
+            <param name="arg1"/>
+            <param name="arg2" required="false"/>
+            <param name="arg3" required="false"/>
+        </method>
+
+        <method name="overloadedMethodWithOptionalArgs">
+            <param name="arg1"/>
+            <param name="arg2" required="false"/>
+        </method>
+
+        <method name="overloadedMethodWithOptionalArgs">
+            <param name="arg1"/>
+        </method>
     </service>
 
     <service name="jmix_OtherRestTestService">