SED-4838 Plan Editor: Normalized Subplan Referencing

[cmisev]

No description provided.

[gemini-code-assist]

Code Review

This pull request introduces support for referencing plans by name rather than ID within automation packages, adding a new plan field to CallPlan and YamlCallPlan, updating the JSON schemas, and implementing a new ScheduleMapper. It also refactors the YAML fragment manager to instantiate mappers as singletons and handle references. The review feedback highlights three critical issues: a potential ConcurrentModificationException and incomplete circular dependency check in the recursive singleton construction of AutomationPackageYamlFragmentManager, a critical logic bug in PlanMapper.setReferences where the referrer's ID is incorrectly assigned to the CallPlan (which would cause infinite recursion), and multiple potential NullPointerExceptions in ScheduleMapper due to a lack of defensive null checks.

[gemini-code-assist]

Using Map.computeIfAbsent recursively on a HashMap (via constructSingleton) will modify the map's structural state (modCount) during the computation, which throws a ConcurrentModificationException at runtime in Java 9+. Additionally, the circular dependency check only compares against the rootClass, which can lead to a StackOverflowError for indirect circular dependencies (e.g., A -> B -> C -> B).

This suggestion refactors the singleton construction to avoid recursive computeIfAbsent calls and uses a LinkedHashSet construction stack to robustly detect any circular dependencies.

    private <T> T constructSingleton(Class<T> annotatedClass, Map<Class<?>, Object> singletons) {
        return (T) constructSingleton(annotatedClass, singletons, new java.util.LinkedHashSet<>());
    }

    // Instantiates a class found by scanning annotations; The class must have exactly one constructor
    // whose arguments can be found in the injectables map.
    @SuppressWarnings("unchecked")
    private <T> T constructSingleton(Class<T> singletonClass, Map<Class<?>, Object> singletons, java.util.Set<Class<?>> constructionStack) {
        if (singletons.containsKey(singletonClass)) {
            return (T) singletons.get(singletonClass);
        }
        if (!constructionStack.add(singletonClass)) {
            throw new IllegalArgumentException("Circular singleton dependency detected: " + constructionStack + " -> " + singletonClass.getName());
        }
        try {
            var constructors = singletonClass.getConstructors();
            if (constructors.length != 1) {
                throw new IllegalStateException("Expected exactly one constructor for singleton class "
                    + singletonClass.getName() + ", but found " + constructors.length);
            }
            var constructor = constructors[0];
            Object[] parameters = Arrays.stream(constructor.getParameterTypes())
                .map(dependentSingletonClass -> constructSingleton(dependentSingletonClass, singletons, constructionStack))
                .toArray();
            T instance = (T) constructor.newInstance(parameters);
            singletons.put(singletonClass, instance);
            return instance;
        } catch (Exception e) {
            throw new RuntimeException(e);
        } finally {
            constructionStack.remove(singletonClass);
        }
    }

[gemini-code-assist]

There is a critical correctness bug in setReferences:

1. callPlan.setPlanId(plan.getId().toString()) sets the planId of the CallPlan to the ID of the parent/referrer plan (plan), rather than the referenced plan (referencedPlan). This will cause every CallPlan to call its own parent plan, resulting in infinite recursion during execution.
2. If referencedPlan is null (e.g., the called plan is not found in the package), calling planToReferencingPlansMap.computeIfAbsent(referencedPlan, ...) will insert a null key into the map, and then setting the plan ID to the parent plan's ID will still occur.

We should only update the references if referencedPlan is successfully resolved, and we must set the planId to referencedPlan.getId().toString().

Suggested change

	private void setReferences(Map<String, Plan> nameToPlanMap, Plan plan, AbstractArtefact node) {
	node.getChildren().forEach(a -> setReferences(nameToPlanMap, plan, a));
	if (node instanceof CallPlan callPlan) {
	Plan referencedPlan = nameToPlanMap.get(callPlan.getPlan());
	planToReferencingPlansMap.computeIfAbsent(referencedPlan, p -> new HashSet<>()).add(plan);
	if (plan != null) {
	callPlan.setPlanId(plan.getId().toString());
	callPlan.setPlan(null);
	}
	}
	}
	private void setReferences(Map<String, Plan> nameToPlanMap, Plan plan, AbstractArtefact node) {
	node.getChildren().forEach(a -> setReferences(nameToPlanMap, plan, a));
	if (node instanceof CallPlan callPlan) {
	Plan referencedPlan = nameToPlanMap.get(callPlan.getPlan());
	if (referencedPlan != null) {
	planToReferencingPlansMap.computeIfAbsent(referencedPlan, p -> new HashSet<>()).add(plan);
	callPlan.setPlanId(referencedPlan.getId().toString());
	callPlan.setPlan(null);
	}
	}
	}

[gemini-code-assist]

The toYamlObject and toBusinessObject methods are prone to NullPointerException under several scenarios:

1. In toYamlObject, if parameters.getExecutionsParameters() or parameters.getExecutionsParameters().getPlan() is null, calling .getPlan() or .getAttribute(...) will throw a NullPointerException.
2. In both toYamlObject and toBusinessObject, if cronExclusions is null, calling .stream() on it will throw a NullPointerException.

This suggestion adds defensive null checks to ensure robust serialization and deserialization of schedules.

    @Override
    public AutomationPackageSchedule toYamlObject(ExecutiontTaskParameters parameters) {
        AutomationPackageSchedule yamlSchedule = new AutomationPackageSchedule(null);
        if (parameters.getExecutionsParameters() != null) {
            yamlSchedule.setExecutionParameters(parameters.getExecutionsParameters().getCustomParameters());
            if (parameters.getExecutionsParameters().getPlan() != null) {
                yamlSchedule.setPlanName(parameters.getExecutionsParameters().getPlan().getAttribute(AbstractOrganizableObject.NAME));
            }
        }
        yamlSchedule.setName(parameters.getAttribute(AbstractOrganizableObject.NAME));
        yamlSchedule.setActive(parameters.isActive());
        yamlSchedule.setCron(parameters.getCronExpression());
        if (parameters.getCronExclusions() != null) {
            yamlSchedule.setCronExclusions(parameters.getCronExclusions().stream().map(CronExclusion::getCronExpression).toList());
        }
        return yamlSchedule;
    }

    @Override
    public ExecutiontTaskParameters toBusinessObject(AutomationPackageSchedule yamlSchedule) {
        ExecutiontTaskParameters parameters = new ExecutiontTaskParameters();

        Map<String, String> attributes = new HashMap<>();
        attributes.put(AbstractOrganizableObject.NAME, yamlSchedule.getName());
        parameters.setAttributes(attributes);

        parameters.setActive(yamlSchedule.getActive());
        parameters.setCronExpression(yamlSchedule.getCron());
        if (yamlSchedule.getCronExclusions() != null) {
            parameters.setCronExclusions(yamlSchedule.getCronExclusions().stream().map(e -> new CronExclusion(e, "")).toList());
        }
        parameters.setExecutionsParameters(new ExecutionParameters());
        return parameters;
    }

[david-stephan]

Summarizing the important comment. We should not change the BO model. To resolve the main issue the FE will be adapted to always add callPlan with selection attributes ni the IDE. CallPlan by planID are only valid when invoking plans external to the AP.
We could introduce a new plan field in the YAML model as an alias/shortcut to call a plan by selection attributes by name.
Supporting the propagation of name change (i.e. renaming a Keyword, propagates to all callKeywords that resolved to it, is something generic impacting also a Step instance and should be implemented separately. This is not a priority for now.

[david-stephan]

That seems like a local dev/test changes please revert.

[david-stephan]

Not sure why we need these dependencies as compile scope now, not seeing related code changes.

[david-stephan]

copyright should remain at the top of the file, that also create unrequired diff

[david-stephan]

idem with copyright

[david-stephan]

idem with copyright

[david-stephan]

This looks more related to "SED-4840 Local Schedule Binding", I'm fine to do both in same PR is the effort doesn't increase drastically and they logically relate to one another

[david-stephan]

should be yamlSchedule.getExecutionParameters

[david-stephan]

I feel we're duplicating part of the logic used when deploying an AP implemented in AutomationPackageSchedulerHook.prepareExecutionTasksParamsStaging. We should create a following to algin all this at some point.

[david-stephan]

could be kept as an alias for selection by attributes by name only as for call keyword

[david-stephan]

If we do change the schema to support an alias, we should decide if we want to go with a minor bump 1.2.1. Also I think there are some constant related to that version that should be udpated, I did not see the change in the PR.