fix(recordingoptions): client may explicitly unset options

HTTP_API.md

@@ -686,15 +686,15 @@
     **The request must include the following fields:**
 
     `toDisk` - Whether a recording is stored to disk;
-    either `true` or `false`.
+    either `true` or `false`, or `unset` to restore the JVM default.
 
     **The request may include the following fields:**
 
-    `maxAge` - The maximum event age of a recording, in seconds.
-    A value of zero means there is no maximum event age.
+    `maxAge` - The maximum event age of a recording, in seconds as a positive
+    integer, or `unset` to restore the JVM default.
 
-    `maxSize` - The maximum size of a recording, in bytes.
-    A value of zero means there is no maximum recording size.
+    `maxSize` - The maximum size of a recording, in bytes as a positive integer,
+    or `unset` to restore the JVM default.
 
     ###### response
     `200` - The body is the updated default recording options of the
@@ -721,8 +721,8 @@
 
     ###### example
     ```
-    $ curl -X PATCH --data "toDisk=true&maxAge=0" localhost:8181/api/v1/targets/localhost/recordingOptions
-    {"maxAge":0,"toDisk":true,"maxSize":0}
+    $ curl -X PATCH --data "toDisk=unset&maxAge=60&maxSize=1024" localhost:8181/api/v1/targets/localhost/recordingOptions
+    {"maxAge":60,"toDisk":"unset","maxSize":1024}
     ```
 
 

pom.xml

@@ -484,6 +484,7 @@
       <version>${org.apache.maven.plugins.surefire.version}</version>
       <configuration>
         <skipTests>${skipUTs}</skipTests>
+        <trimStackTrace>false</trimStackTrace>
       </configuration>
     </plugin>
     <plugin>

src/main/java/io/cryostat/net/web/http/api/v1/TargetRecordingOptionsPatchHandler.java

@@ -68,6 +68,7 @@
 class TargetRecordingOptionsPatchHandler extends AbstractAuthenticatedRequestHandler {
 
     static final String PATH = TargetRecordingOptionsGetHandler.PATH;
+    private static final String UNSET_KEYWORD = "unset";
     private final RecordingOptionsCustomizer customizer;
     private final TargetConnectionManager connectionManager;
     private final RecordingOptionsBuilderFactory recordingOptionsBuilderFactory;
@@ -114,7 +115,7 @@ public boolean isAsync() {
 
     @Override
     public void handleAuthenticated(RoutingContext ctx) throws Exception {
-        Pattern bool = Pattern.compile("true|false");
+        Pattern bool = Pattern.compile("true|false|" + UNSET_KEYWORD);
         MultiMap attrs = ctx.request().formAttributes();
         if (attrs.contains("toDisk")) {
             Matcher m = bool.matcher(attrs.get("toDisk"));
@@ -125,8 +126,12 @@ public void handleAuthenticated(RoutingContext ctx) throws Exception {
                         key -> {
                             if (attrs.contains(key)) {
                                 try {
-                                    Long.parseLong(attrs.get(key));
-                                } catch (Exception e) {
+                                    String v = attrs.get(key);
+                                    if (UNSET_KEYWORD.equals(v)) {
+                                        return;
+                                    }
+                                    Long.parseLong(v);
+                                } catch (NumberFormatException e) {
                                     throw new HttpStatusException(400, "Invalid options");
                                 }
                             }
@@ -139,13 +144,14 @@ public void handleAuthenticated(RoutingContext ctx) throws Exception {
                                     .forEach(
                                             key -> {
                                                 if (attrs.contains(key)) {
-                                                    OptionKey.fromOptionName(key)
-                                                            .ifPresent(
-                                                                    optionKey ->
-                                                                            customizer.set(
-                                                                                    optionKey,
-                                                                                    attrs.get(
-                                                                                            key)));
+                                                    String v = attrs.get(key);
+                                                    OptionKey optionKey =
+                                                            OptionKey.fromOptionName(key).get();
+                                                    if (UNSET_KEYWORD.equals(v)) {
+                                                        customizer.unset(optionKey);
+                                                    } else {
+                                                        customizer.set(optionKey, v);
+                                                    }
                                                 }
                                             });
 

src/test/java/io/cryostat/net/web/http/api/v1/TargetRecordingOptionsPatchHandlerTest.java

@@ -115,16 +115,16 @@ void shouldHaveExpectedRequiredPermissions() {
 
     @Test
     void shouldSetRecordingOptions() throws Exception {
-        Map<String, String> defaultValues =
+        Map<String, String> originalValues =
                 Map.of("toDisk", "true", "maxAge", "50", "maxSize", "32");
         Mockito.when(recordingOptionsBuilderFactory.create(Mockito.any())).thenReturn(builder);
         Mockito.when(builder.build()).thenReturn(recordingOptions);
-        Mockito.when(recordingOptions.get("toDisk")).thenReturn(defaultValues.get("toDisk"));
-        Mockito.when(recordingOptions.get("maxAge")).thenReturn(defaultValues.get("maxAge"));
-        Mockito.when(recordingOptions.get("maxSize")).thenReturn(defaultValues.get("maxSize"));
+        Mockito.when(recordingOptions.get("toDisk")).thenReturn(originalValues.get("toDisk"));
+        Mockito.when(recordingOptions.get("maxAge")).thenReturn(originalValues.get("maxAge"));
+        Mockito.when(recordingOptions.get("maxSize")).thenReturn(originalValues.get("maxSize"));
 
         MultiMap requestAttrs = MultiMap.caseInsensitiveMultiMap();
-        requestAttrs.addAll(defaultValues);
+        requestAttrs.addAll(originalValues);
 
         Mockito.when(
                         connectionManager.executeConnectedTask(
@@ -159,11 +159,57 @@ public Map answer(InvocationOnMock args) throws Throwable {
         }
     }
 
+    @Test
+    void shouldUnsetRecordingOptions() throws Exception {
+        Map<String, String> originalValues =
+                Map.of("toDisk", "true", "maxAge", "50", "maxSize", "32");
+        Mockito.when(recordingOptionsBuilderFactory.create(Mockito.any())).thenReturn(builder);
+        Mockito.when(builder.build()).thenReturn(recordingOptions);
+        Mockito.when(recordingOptions.get("toDisk")).thenReturn(originalValues.get("toDisk"));
+        Mockito.when(recordingOptions.get("maxAge")).thenReturn(originalValues.get("maxAge"));
+        Mockito.when(recordingOptions.get("maxSize")).thenReturn(originalValues.get("maxSize"));
+
+        MultiMap requestAttrs = MultiMap.caseInsensitiveMultiMap();
+        requestAttrs.addAll(Map.of("toDisk", "unset", "maxAge", "unset", "maxSize", "unset"));
+
+        Mockito.when(
+                        connectionManager.executeConnectedTask(
+                                Mockito.any(ConnectionDescriptor.class), Mockito.any()))
+                .thenAnswer(
+                        new Answer<>() {
+                            @Override
+                            public Map answer(InvocationOnMock args) throws Throwable {
+                                TargetConnectionManager.ConnectedTask ct =
+                                        (TargetConnectionManager.ConnectedTask)
+                                                args.getArguments()[1];
+                                return (Map) ct.execute(jfrConnection);
+                            }
+                        });
+
+        RoutingContext ctx = Mockito.mock(RoutingContext.class);
+        Mockito.when(ctx.pathParam("targetId")).thenReturn("foo:9091");
+        HttpServerRequest req = Mockito.mock(HttpServerRequest.class);
+        Mockito.when(ctx.request()).thenReturn(req);
+        Mockito.when(req.headers()).thenReturn(MultiMap.caseInsensitiveMultiMap());
+        Mockito.when(req.formAttributes()).thenReturn(requestAttrs);
+        HttpServerResponse resp = Mockito.mock(HttpServerResponse.class);
+        Mockito.when(ctx.response()).thenReturn(resp);
+        IFlightRecorderService service = Mockito.mock(IFlightRecorderService.class);
+        Mockito.when(jfrConnection.getService()).thenReturn(service);
+
+        handler.handleAuthenticated(ctx);
+
+        for (var entry : requestAttrs.entries()) {
+            var key = OptionKey.fromOptionName(entry.getKey());
+            Mockito.verify(customizer).unset(key.get());
+        }
+    }
+
     @ParameterizedTest
     @MethodSource("getRequestMaps")
-    void shouldThrowInvalidOptionException(Map<String, String> defaultValues) throws Exception {
+    void shouldThrowInvalidOptionException(Map<String, String> values) throws Exception {
         MultiMap requestAttrs = MultiMap.caseInsensitiveMultiMap();
-        requestAttrs.addAll(defaultValues);
+        requestAttrs.addAll(values);
 
         RoutingContext ctx = Mockito.mock(RoutingContext.class);
         HttpServerRequest req = Mockito.mock(HttpServerRequest.class);

src/test/java/itest/TargetRecordingOptionsIT.java

@@ -69,9 +69,9 @@ public class TargetRecordingOptionsIT extends StandardSelfTest {
     static void resetDefaultRecordingOptions() throws Exception {
         CompletableFuture<JsonObject> dumpResponse = new CompletableFuture<>();
         MultiMap form = MultiMap.caseInsensitiveMultiMap();
-        form.add("maxAge", "0");
-        form.add("toDisk", "false");
-        form.add("maxSize", "0");
+        form.add("maxAge", "unset");
+        form.add("toDisk", "unset");
+        form.add("maxSize", "unset");
 
         webClient
                 .patch(OPTIONS_REQ_URL)