Ignore irrelevant parameters during start of an operation by karrgov · Pull Request #1680 · cloudfoundry/multiapps-controller

karrgov · 2025-08-07T16:18:01Z

Jira Item: LMCROSSITXSADEPLOY-3120

theghost5800 · 2025-08-08T08:34:42Z

    private Operation addParameterValues(Operation operation, Set<ParameterMetadata> predefinedParameters) {
        Map<String, Object> parameters = new HashMap<>(operation.getParameters());
        parameters.putAll(ParameterConversion.toFlowableVariables(predefinedParameters, parameters));
+        filterUnnecessaryParameters(predefinedParameters, parameters);


Can we perform this filtering in startOperation method before invocation of addServiceParameters and addParameterValues. The idea is to reduce as much as possible to add all user input parameters to the flowable context by mistake.

I have thought about this to filter as early as possible but after debugging - I have found out that these parameters are added during the creation of the operation and the methods OperationsApiServiceImpl::addParameterValues and OperationsApiServiceImpl::addServiceParameters are actually the earliest places that the parameters of the operation are retrieved - Map<String, Object> parameters = new HashMap<>(operation.getParameters()); - thus I think the most appropriate place out of the two is the OperationsApiServiceImpl::addParameterValues method. The method just before these two, which is supposed to get the predefined parameters does not know anything about the specific parameters the user could provide in the request's body - its only purpose is to get the parameters that correspond to the type of operation - DEPLOY, BLUE_GREEN_DEPLOY, CTS_DEPLOY and etc.

theghost5800 · 2025-08-08T08:41:30Z

    }

+    private Set<String> getNamesOfServiceParameters() {
+        return new HashSet<>(


You can use the feature since java 11 and use Set.of method to create immutable Set without nesting of different collections.

thanks for the suggestion, fixed!

theghost5800 · 2025-08-08T08:43:33Z

                        .anyMatch(message -> message.getType() == MessageType.ERROR);
    }

+    private Set<String> getNamesOfServiceParameters() {


Can you make this method constant located in Constants class of multiapps-controller-web package and reuse this constant also in addServiceParameters method. Making it as constant and reuse it in both methods will avoid errors in future where one variable name is added or removed in one place and forgetting to the same in other method.

fixed - I have used an if else because the switch does not allow method calls - thus Variables.USER.getName() can not be used inside the different cases.

theghost5800 · 2025-08-08T08:47:15Z

               .startProcess(Mockito.any(), Mockito.anyMap());
    }

+    void testStartOperationWithInvalidParametersForTheProcess() {


Add test annotation

theghost5800 · 2025-08-08T08:47:25Z

+                                 .size());
+    }
+
+    void testStartOperationWithValidParametersForTheProcess() {


Add test annotation

theghost5800 · 2025-08-08T08:54:36Z

+        assertNotEquals(parameters.size(), operation.getParameters()
+                                                    .size());
+        assertEquals(2, operation.getParameters()
+                                 .size());


When perform unit testing of methods which doesn't return any result, it's not relevant to use assertEquals because there is no returned value. That's why think about how to improve Mockito.verify assertion.

fixed - you are completely right but I was asserting the values returned by the calls of the size() method, but it turned out that the current logic for the testing was not correct at all and I was not asserting what I needed to. Almost done with the new tests.

theghost5800 · 2025-08-08T08:54:53Z

+        operationsApiService.startOperation(mockHttpServletRequest(EXAMPLE_USER), SPACE_GUID, operation);
+        Mockito.verify(flowableFacade)
+               .startProcess(Mockito.any(), Mockito.anyMap());
+        assertEquals(parameters.size(), operation.getParameters()


same comment as previous test assertion

Same as the previous comment

IvanBorislavovDimitrov · 2025-08-08T09:59:32Z

                                 .withParameters(parameters);
    }

+    private void filterUnnecessaryParameters(Set<ParameterMetadata> predefinedParameters, Map<String, Object> parameters) {


try not to use in-parameters. instead of modifying parameters, return a new Map.

fixed, thanks for the suggestion!

IvanBorislavovDimitrov · 2025-08-08T10:02:21Z

+        Set<String> namesOfServiceParameters = getNamesOfServiceParameters();
+        Iterator<Map.Entry<String, Object>> it = parameters.entrySet()
+                                                           .iterator();
+        while (it.hasNext()) {
+            String key = it.next()
+                           .getKey();
+            if (!(namesOfPredefinedParameters.contains(key) || namesOfServiceParameters.contains(key))) {
+                it.remove();
+            }
+        }


you can simplify this method and implicitly use the iterator.
try to use for-each with the entrySet directly and do the filtration in the for-each.
if you use a new map as I mentioned in the other comment this can be simplified

thank you, fixed!

IvanBorislavovDimitrov · 2025-08-08T10:05:35Z

+        operationsApiService.startOperation(mockHttpServletRequest(EXAMPLE_USER), SPACE_GUID, operation);
+        Mockito.verify(flowableFacade)
+               .startProcess(Mockito.any(), Mockito.anyMap());
+        assertNotEquals(parameters.size(), operation.getParameters()


do you need this assertion as the other verifies the exact size?

right, fixed!

IvanBorislavovDimitrov · 2025-08-11T06:35:21Z

    }
+
+    public static final Set<String> NAMES_OF_SERVICE_PARAMETERS = Set.of(
+        org.cloudfoundry.multiapps.controller.persistence.Constants.VARIABLE_NAME_SERVICE_ID, Variables.USER.getName(),


why is the full package name used?

Because I did not want to have a static import. Should I do it this way? import static org.cloudfoundry.multiapps.controller.persistence.Constants.VARIABLE_NAME_SERVICE_ID;

IvanBorislavovDimitrov · 2025-08-11T06:38:31Z

-        String namespace = operation.getNamespace();
-        if (namespace != null) {
-            parameters.put(Variables.MTA_NAMESPACE.getName(), namespace);
+        for (String key : org.cloudfoundry.multiapps.controller.web.Constants.NAMES_OF_SERVICE_PARAMETERS) {


I do not understand why this for loop is needed.
What is the point of iterating and checking for values that have already been added?

The code before that was without any loops, but since Kris has left me a comment about adding this specific set to the Constants class and trying to use it also in the method OperationsApiServiceImpl::addServiceParameters - I was able to come up with only this approach in order to make the method tied to the constant set and use it

IvanBorislavovDimitrov · 2025-08-11T06:39:33Z

+        Set<String> allowedParameters = new HashSet<>();
+        Map<String, Object> filteredParameters = new LinkedHashMap<>();
+
+        for (ParameterMetadata parameterMetadata : predefinedParameters) {


instead of for loop, can you create allowedParameters from the predefinedParameters

fixed, thank you

IvanBorislavovDimitrov · 2025-08-11T06:41:04Z

+
+    private Map<String, Object> filterUnnecessaryParameters(Set<ParameterMetadata> predefinedParameters, Map<String, Object> parameters) {
+        Set<String> allowedParameters = new HashSet<>();
+        Map<String, Object> filteredParameters = new LinkedHashMap<>();


why LinkedHashMap?

Because it was easier for while debugging in terms of the ordering and I thought it would not be a problem to have it like that - but I will revert it back to the regular hashMap. Thanks, fixed!

IvanBorislavovDimitrov · 2025-08-11T06:41:24Z

+        for (Map.Entry<String, Object> entry : parameters.entrySet()) {
+            if (allowedParameters.contains(entry.getKey())) {
+                filteredParameters.put(entry.getKey(), entry.getValue());
+            }
+        }
+        return filteredParameters;


you can use the java stream API to perform the filtering

IvanBorislavovDimitrov · 2025-08-11T06:41:39Z

+        for (ParameterMetadata parameterMetadata : predefinedParameters) {
+            allowedParameters.add(parameterMetadata.getId());
+        }
+        allowedParameters.addAll(org.cloudfoundry.multiapps.controller.web.Constants.NAMES_OF_SERVICE_PARAMETERS);


why full package name needed?

IvanBorislavovDimitrov · 2025-08-11T06:58:13Z

@@ -277,8 +291,26 @@ private Operation addServiceParameters(Operation operation, String spaceGuid, St
    private Operation addParameterValues(Operation operation, Set<ParameterMetadata> predefinedParameters) {
        Map<String, Object> parameters = new HashMap<>(operation.getParameters());


can you make the whole filtration here?

theghost5800 · 2025-08-11T07:24:21Z

+            if (key.equals(Constants.VARIABLE_NAME_SERVICE_ID)) {
+                parameters.put(key, processDefinitionKey);
+            } else if (key.equals(Variables.USER.getName())) {
+                parameters.put(key, user);
+            } else if (key.equals(Variables.USER_GUID.getName())) {
+                parameters.put(key, userGuid);
+            } else if (key.equals(Variables.SPACE_NAME.getName())) {
+                parameters.put(key, space.getName());
+            } else if (key.equals(Variables.SPACE_GUID.getName())) {
+                parameters.put(key, spaceGuid);
+            } else if (key.equals(Variables.ORGANIZATION_NAME.getName())) {
+                parameters.put(key, organization.getName());
+            } else if (key.equals(Variables.ORGANIZATION_GUID.getName())) {
+                parameters.put(key, organization.getGuid()
+                                                .toString());
+            } else if (key.equals(Variables.TIMESTAMP.getName())) {
+                parameters.put(key, DateTimeFormatter.ofPattern("yyyyMMddHHmmss")
+                                                     .format(ZonedDateTime.now()));
+            } else if (key.equals(Variables.MTA_NAMESPACE.getName())) {
+                String namespace = operation.getNamespace();
+                if (namespace != null) {
+                    parameters.put(key, namespace);
+                }


Too much ifs, let's return it in previous way.

theghost5800 · 2025-08-11T07:24:53Z

+        Set<String> allowedParameters = new HashSet<>();
+        Map<String, Object> filteredParameters = new LinkedHashMap<>();
+
+        for (ParameterMetadata parameterMetadata : predefinedParameters) {
+            allowedParameters.add(parameterMetadata.getId());
+        }
+        allowedParameters.addAll(org.cloudfoundry.multiapps.controller.web.Constants.NAMES_OF_SERVICE_PARAMETERS);


Extract collecting of allowedParameters in a new method.

fixed, thanks!

IvanBorislavovDimitrov · 2025-08-11T11:09:19Z

+    private Map<String, Object> filterUnnecessaryParameters(Set<ParameterMetadata> predefinedParameters, Map<String, Object> parameters) {
+        Map<String, Object> filteredParameters = new HashMap<>();
+        Set<String> allowedParameters = getAllowedParameters(predefinedParameters);
+
+        parameters.entrySet()
+                  .stream()
+                  .filter(entry -> allowedParameters.contains(entry.getKey()))
+                  .forEach(entry -> filteredParameters.put(entry.getKey(), entry.getValue()));
+        return filteredParameters;
+    }


**private Map<String, Object> filterUnnecessaryParameters(
Set predefinedParameters,
Map<String, Object> parameters) {

Set<String> allowedParameters = getAllowedParameters(predefinedParameters); return parameters.entrySet() .stream() .filter(entry -> allowedParameters.contains(entry.getKey())) .collect(Collectors.toMap( Map.Entry::getKey, Map.Entry::getValue, (a, b) -> b, // keep the latest value if a duplicate key appears HashMap::new // create a mutable map ));

}**

thank you, fixed!

IvanBorislavovDimitrov

do not forget to squash commits

LMCROSSITXSADEPLOY-3120 Fixes regarding service parameters and the unit tests LMCROSSITXSADEPLOY-3120 Some overall fixes regarding formatting LMCROSSITXSADEPLOY-3120 Fix for the stream LMCROSSITXSADEPLOY-3120

theghost5800 requested changes Aug 8, 2025

View reviewed changes

IvanBorislavovDimitrov reviewed Aug 8, 2025

View reviewed changes

IvanBorislavovDimitrov reviewed Aug 11, 2025

View reviewed changes

theghost5800 reviewed Aug 11, 2025

View reviewed changes

IvanBorislavovDimitrov reviewed Aug 11, 2025

View reviewed changes

IvanBorislavovDimitrov approved these changes Aug 11, 2025

View reviewed changes

theghost5800 approved these changes Aug 11, 2025

View reviewed changes

Ignore irrelevant parameters during start of an operation

36a2936

LMCROSSITXSADEPLOY-3120 Fixes regarding service parameters and the unit tests LMCROSSITXSADEPLOY-3120 Some overall fixes regarding formatting LMCROSSITXSADEPLOY-3120 Fix for the stream LMCROSSITXSADEPLOY-3120

karrgov force-pushed the ignore_irreleavant_parameters_in_context branch from a0601b5 to 36a2936 Compare August 11, 2025 13:32

IvanBorislavovDimitrov merged commit 31eba39 into cloudfoundry:master Aug 11, 2025
4 of 5 checks passed

		@@ -277,8 +291,26 @@ private Operation addServiceParameters(Operation operation, String spaceGuid, St
		private Operation addParameterValues(Operation operation, Set<ParameterMetadata> predefinedParameters) {
		Map<String, Object> parameters = new HashMap<>(operation.getParameters());

Uh oh!

Conversation

karrgov commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karrgov Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

karrgov commented Aug 7, 2025 •

edited

Loading

karrgov Aug 8, 2025 •

edited

Loading