fix(recordingoptions): client may explicitly unset options

cryostatio · Aug 10, 2021 · 25146c0 · 25146c0
1 parent 73e41e2
commit 25146c0
Show file tree

Hide file tree

Showing 3 changed files with 65 additions and 13 deletions.
diff --git a/src/main/java/io/cryostat/net/web/http/api/v1/TargetRecordingOptionsPatchHandler.java b/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);
+                                                    }
                                                 }
                                             });
 

diff --git a/src/test/java/io/cryostat/net/web/http/api/v1/TargetRecordingOptionsPatchHandlerTest.java b/src/test/java/io/cryostat/net/web/http/api/v1/TargetRecordingOptionsPatchHandlerTest.java
@@ -159,6 +159,52 @@ public Map answer(InvocationOnMock args) throws Throwable {
         }
     }
 
+    @Test
+    void shouldUnsetRecordingOptions() throws Exception {
+        Map<String, String> defaultValues =
+                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"));
+
+        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 {

diff --git a/src/test/java/itest/TargetRecordingOptionsIT.java b/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)