Validation and related exception handling improvements (#1439)

# Changes
## Deploy board

### Front end
Slightly improved the UI for displaying auto promotion config change failures.

### Back end
The code base doesn't handle `IllegalArgumentException` everywhere. Changed `IllegalArgumentException` to a subclass of `TeletraanException`, so it can be handled whenever `TeletraanException` is handled. This works because the handling is the same - show users the error.

## Teletraan service

### Validation
Added constraints to the `PromoteBean`. When users tried to save an invalid schedule, it will be rejected by validation

### Exception handling
Added `JerseyViolationExceptionMapper` and removed our own handling in `GenericExceptionMapper`.  `JerseyViolationExceptionMapper` is simply better and it provides a Json as the response body (v.s. plain text).

Additionally, without `JerseyViolationExceptionMapper` and when an invalid input is caught by validation, the service simply returns 400 without any message, because Jersey's own `ConstraintViolationExceptionMapper` handles the exception without invoking our `GenericExceptionMapper`. 

### Junit 5
Added Junit 5 and used it in the resources tests. Note that Junit 5 can run in parallel with Junit 4. 

# Validations and tests
Currently, there is no validation. Users can save any schedule and they won't know if it is a valid one. 

With this change:
<img width="1653" alt="Screenshot 2024-02-01 at 3 14 41 PM" src="https://github.com/pinterest/teletraan/assets/8442875/96399806-15a2-43b5-87f2-ece9f11ce676">

<img width="1204" alt="Screenshot 2024-02-01 at 3 17 25 PM" src="https://github.com/pinterest/teletraan/assets/8442875/97c5cda5-46f1-4daa-b093-e70c0b4052cc">

deploy-board/deploy_board/templates/configs/promote_config.tmpl

@@ -235,7 +235,7 @@
                     btn.button('reset');
                 },
                 error: function (data) {
-                    $('#errorBannerId').append(data.responseText);
+                    $('#errorBannerId').append(`${data.status}: ${data.responseText}`);
                     $('#errorBannerId').show();
                 }
             });

deploy-board/deploy_board/templates/error.tmpl

@@ -2,5 +2,9 @@
     <div class="alert alert-danger" role="alert">{{ message }}</div>
     <div class="alert" role="alert">Sorry for the inconvenience, please contact your friendly Teletraan owners for
         immediate assistance!</div>
-    <div class="alert alert-danger" role="alert">{{ stacktrace }}</div>
+    <div class="alert alert-danger" role="alert">
+        <pre>
+            {{ stacktrace }}
+        </pre>
+    </div>
 </div>

deploy-board/deploy_board/webapp/helpers/base_client.py

@@ -63,7 +63,7 @@ def api(path, token=None, params=None, data=None):
                     "Oops! You do not have the required permissions for this action. Contact an environment ADMIN for "
                     "assistance. " + response.text)
 
-            if response.status_code == 400:
+            if response.status_code == 400 or response.status_code == 422:
                 raise IllegalArgumentException(response.text)
 
             if response.status_code == 404:

deploy-board/deploy_board/webapp/helpers/exceptions.py

@@ -17,10 +17,6 @@
 """
 
 
-class IllegalArgumentException(Exception):
-    pass
-
-
 class FailedAuthenticationException(Exception):
     pass
 
@@ -34,4 +30,11 @@ class NotFoundException(Exception):
 
 
 class TeletraanException(Exception):
-    pass
+    def __init__(self, message: str, status=500) -> None:
+        super().__init__(message)
+        self.status = status
+
+
+class IllegalArgumentException(TeletraanException):
+    def __init__(self, message: str) -> None:
+        super().__init__(message, status=400)

deploy-board/deploy_board/webapp/promote_views.py

@@ -23,6 +23,7 @@
 from django.views.generic import View
 from . import common
 from .helpers import environs_helper
+from .helpers.exceptions import TeletraanException, IllegalArgumentException
 
 
 class EnvPromoteConfigView(View):
@@ -66,5 +67,8 @@ def post(self, request, name, stage):
             data["delay"] = int(query_dict["promoteDelay"])
         if "promoteQueueSize" in query_dict:
             data["queueSize"] = int(query_dict["promoteQueueSize"])
-        environs_helper.update_env_promotes_config(request, name, stage, data=data)
+        try:
+            environs_helper.update_env_promotes_config(request, name, stage, data=data)
+        except TeletraanException as e:
+            return HttpResponse(e, status=e.status, content_type="application/json")
         return self.get(request, name, stage)

deploy-service/common/pom.xml

@@ -285,5 +285,14 @@
             <version>0.10.5</version>
             <scope>runtime</scope>
         </dependency>
+        <dependency>
+            <groupId>org.quartz-scheduler</groupId>
+            <artifactId>quartz</artifactId>
+        </dependency>
+        <dependency>
+            <groupId>io.dropwizard</groupId>
+            <artifactId>dropwizard-validation</artifactId>
+            <version>${dropwizard.version}</version>
+        </dependency>
     </dependencies>
 </project>

deploy-service/common/src/main/java/com/pinterest/deployservice/bean/PromoteBean.java

@@ -4,9 +4,9 @@
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- *  
+ *
  *     http://www.apache.org/licenses/LICENSE-2.0
- *    
+ *
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS,
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@@ -15,11 +15,21 @@
  */
 package com.pinterest.deployservice.bean;
 
-import com.fasterxml.jackson.annotation.JsonProperty;
+import java.io.Serializable;
+
+import javax.validation.constraints.Max;
+import javax.validation.constraints.Min;
+import javax.validation.constraints.NotNull;
+
 import org.apache.commons.lang.builder.ReflectionToStringBuilder;
+import org.apache.commons.lang3.StringUtils;
+import org.quartz.CronExpression;
 
-import java.io.Serializable;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.pinterest.deployservice.common.Constants;
 
+import io.dropwizard.validation.ValidationMethod;
 /**
  * Keep the bean and table in sync
  * <p>
@@ -47,16 +57,19 @@ public class PromoteBean implements Updatable, Serializable {
     @JsonProperty("lastUpdate")
     private Long last_update;
 
+    @NotNull
     private PromoteType type;
 
     @JsonProperty("predStage")
     private String pred_stage;
 
+    @Min(1) @Max(Constants.DEFAULT_MAX_PROMOTE_QUEUE_SIZE)
     @JsonProperty("queueSize")
     private Integer queue_size;
 
     private String schedule;
 
+    @Min(0)
     private Integer delay;
 
     @JsonProperty("disablePolicy")
@@ -165,4 +178,22 @@ public SetClause genSetClause() {
     public String toString() {
         return ReflectionToStringBuilder.toString(this);
     }
+
+    @ValidationMethod(message = "schedule must be a valid cron expression")
+    @JsonIgnore
+    public boolean isScheduleValid() {
+        if (type != PromoteType.AUTO) {
+            return true;
+        }
+        if (StringUtils.isBlank(schedule)) {
+            return true;
+        }
+
+        try {
+            new CronExpression(schedule);
+            return true;
+        } catch (Exception e) {
+            return false;
+        }
+    }
 }

deploy-service/pom.xml

@@ -14,6 +14,8 @@
         <encoding>UTF-8</encoding>
         <project.build_id.sourceEncoding>UTF-8</project.build_id.sourceEncoding>
         <micrometer.version>1.11.3</micrometer.version>
+        <dropwizard.version>1.3.29</dropwizard.version>
+        <junit-jupiter.version>5.10.1</junit-jupiter.version>
     </properties>
 
     <modules>
@@ -36,10 +38,28 @@
                 <type>pom</type>
                 <scope>import</scope>
             </dependency>
+            <dependency>
+                <groupId>org.quartz-scheduler</groupId>
+                <artifactId>quartz</artifactId>
+                <version>2.3.2</version>
+            </dependency>
         </dependencies>
     </dependencyManagement>
 
     <dependencies>
+        <!--JUnit Jupiter Engine to depend on the JUnit5 engine and JUnit 5 API -->
+        <dependency>
+            <groupId>org.junit.jupiter</groupId>
+            <artifactId>junit-jupiter-engine</artifactId>
+            <version>${junit-jupiter.version}</version>
+            <scope>test</scope>
+        </dependency>
+        <!--JUnit Jupiter Engine to depend on the JUnit4 engine and JUnit 4 API  -->
+        <dependency>
+            <groupId>org.junit.vintage</groupId>
+            <artifactId>junit-vintage-engine</artifactId>
+            <version>${junit-jupiter.version}</version>
+        </dependency>
         <dependency>
             <groupId>junit</groupId>
             <artifactId>junit</artifactId>
@@ -71,8 +91,19 @@
                     <artifactId>maven-source-plugin</artifactId>
                     <version>2.1.2</version>
                 </plugin>
+                <plugin>
+                    <groupId>org.apache.maven.plugins</groupId>
+                    <artifactId>maven-surefire-plugin</artifactId>
+                    <version>3.2.5</version>
+                </plugin>
             </plugins>
         </pluginManagement>
+        <plugins>
+            <plugin>
+                <groupId>org.apache.maven.plugins</groupId>
+                <artifactId>maven-surefire-plugin</artifactId>
+            </plugin>
+        </plugins>
     </build>
 
     <distributionManagement>
@@ -116,4 +147,4 @@
             </build>
         </profile>
     </profiles>
-</project>
+</project>

deploy-service/teletraanservice/pom.xml

@@ -19,7 +19,6 @@
         <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
         <project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
         <maven.deploy.skip>true</maven.deploy.skip>
-        <dropwizard.version>1.3.29</dropwizard.version>
         <dropwizard.health.version>1.7.3</dropwizard.health.version>
     </properties>
 
@@ -97,7 +96,6 @@
         <dependency>
             <groupId>org.quartz-scheduler</groupId>
             <artifactId>quartz</artifactId>
-            <version>2.3.2</version>
         </dependency>
         <!-- The following two dependencies needed only for embedded mysql, remove them there is standalone mysql -->
         <dependency>
@@ -120,6 +118,18 @@
             <groupId>io.micrometer</groupId>
             <artifactId>micrometer-core</artifactId>
         </dependency>
+        <dependency>
+            <groupId>io.dropwizard</groupId>
+            <artifactId>dropwizard-testing</artifactId>
+            <version>${dropwizard.version}</version>
+            <scope>test</scope>
+        </dependency>
+        <dependency>
+            <groupId>org.junit.jupiter</groupId>
+            <artifactId>junit-jupiter-params</artifactId>
+            <version>${junit-jupiter.version}</version>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 
     <build>

deploy-service/teletraanservice/src/main/java/com/pinterest/teletraan/TeletraanService.java

@@ -25,6 +25,7 @@
 import io.dropwizard.health.conf.HealthConfiguration;
 import io.dropwizard.health.core.HealthCheckBundle;
 import io.dropwizard.jersey.jackson.JsonProcessingExceptionMapper;
+import io.dropwizard.jersey.validation.JerseyViolationExceptionMapper;
 import io.dropwizard.setup.Bootstrap;
 import io.dropwizard.setup.Environment;
 import io.swagger.jaxrs.config.BeanConfig;
@@ -172,7 +173,9 @@ public void run(TeletraanServiceConfiguration configuration, Environment environ
 
         environment.healthChecks().register("generic", new GenericHealthCheck(context));
 
-        // Exception handler
+        // Exception handlers
+        // Constrains validation exceptions, returns 4xx
+        environment.jersey().register(new JerseyViolationExceptionMapper());
         // Jackson Json parsing exceptions
         environment.jersey().register(new JsonProcessingExceptionMapper(true));
         environment.jersey().register(new GenericExceptionMapper(configuration.getSystemFactory().getClientError()));

deploy-service/teletraanservice/src/main/java/com/pinterest/teletraan/exception/GenericExceptionMapper.java

@@ -10,8 +10,6 @@
 import java.io.StringWriter;
 
 import javax.servlet.http.HttpServletRequest;
-import javax.validation.ConstraintViolationException;
-import javax.validation.ConstraintViolation;
 import javax.ws.rs.WebApplicationException;
 import javax.ws.rs.core.Context;
 import javax.ws.rs.core.Response;
@@ -56,17 +54,6 @@ public Response toResponse(Throwable t) {
                     return Response.serverError().entity(sb.toString()).build();
                 }
             }
-        } else if (t instanceof ConstraintViolationException) {
-            StringBuilder sb = new StringBuilder();
-            ConstraintViolationException cve = (ConstraintViolationException)t;
-            for (ConstraintViolation cv : cve.getConstraintViolations()) {
-                if (cv.getInvalidValue() != null) {
-                    sb.append(cv.getPropertyPath().toString() + ":" + cv.getInvalidValue().toString());
-                    sb.append(" " + cv.getMessage());
-                }
-            }
-            sb.append("\nParameters in request violate configured constraints.");
-            return Response.status(Response.Status.BAD_REQUEST).entity(sb.toString()).build();
         } else {
             String errorMessage = buildErrorMessage(request);
             StringBuilder sb = new StringBuilder();

deploy-service/teletraanservice/src/main/java/com/pinterest/teletraan/resource/EnvPromotes.java

@@ -4,9 +4,9 @@
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- *  
+ *
  *     http://www.apache.org/licenses/LICENSE-2.0
- *    
+ *
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS,
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@@ -74,10 +74,10 @@ public PromoteBean get(@ApiParam(value = "Environment name", required = true)@Pa
             value = "Update promote info",
             notes = "Updates promote info given environment and stage names by given promote info object")
     public void update(@Context SecurityContext sc,
-                       @ApiParam(value = "Environment name", required = true)@PathParam("envName") String envName,
-                       @ApiParam(value = "Stage name", required = true)@PathParam("stageName") String stageName,
-                       @ApiParam(value = "Promote object to update with", required = true)
-                           @Valid PromoteBean promoteBean) throws Exception {
+            @ApiParam(value = "Environment name", required = true) @PathParam("envName") String envName,
+            @ApiParam(value = "Stage name", required = true) @PathParam("stageName") String stageName,
+            @ApiParam(value = "Promote object to update with", required = true) @Valid PromoteBean promoteBean)
+            throws Exception {
         EnvironBean environBean = Utils.getEnvStage(environDAO, envName, stageName);
         authorizer.authorize(sc, new Resource(environBean.getEnv_name(), Resource.Type.ENV), Role.OPERATOR);
         String operator = sc.getUserPrincipal().getName();

deploy-service/teletraanservice/src/main/java/com/pinterest/teletraan/worker/AutoPromoter.java

@@ -103,7 +103,7 @@ void processBatch() throws Exception {
                 errorBudgetSuccess.increment();
             } catch (Throwable t) {
                 // Catch all throwable so that subsequent job not suppressed
-                LOG.error("AutoPromoter failed to process {}, Exception: {}", envId, t);
+                LOG.error("AutoPromoter failed to process {}", envId, t);
 
                 errorBudgetFailure.increment();
             }