SED-4906 Support string interpolation in plan values by jeromecomte · Pull Request #700 · exense/step

jeromecomte · 2026-08-30T17:17:00Z

No description provided.

…n DynamiceBeanResolver

…verage for yaml plans

…igration tasks

…caping

…igration task for parameters

gemini-code-assist

Code Review

This pull request introduces string interpolation support for plain (non-dynamic) string values using the ${...} syntax across plans and parameters, updating the schema version to 1.3.0. It includes migration tasks to escape pre-existing literals, core classes for parsing and interpolation, and the @NoStringInterpolation annotation to protect structured JSON containers. Feedback on the changes highlights a potential 'Halloween Problem' in MongoDB during migration when saving documents while iterating with a lazy cursor, which can be resolved by sorting by _id. Additionally, reviewers recommended optimizing performance by avoiding anonymous TypeReference instantiations and caching classpath scans, as well as improving code safety by not reassigning method parameters inside loops.

gemini-code-assist · 2026-08-30T17:18:57Z

+        AtomicLong errorCount = new AtomicLong();
+
+        logger.info("Escaping the string interpolation placeholders of the plain values in '{}'...", collectionName);
+        try (Stream<Document> documents = collection.findLazy(Filters.empty(), null, null, null, 0)) {


In MongoDB, modifying and saving a document while iterating over the collection using a lazy cursor (findLazy) can lead to the Halloween Problem. If the document's size increases (which is likely here as we are replacing ${ with $${), MongoDB may relocate the document to the end of the collection. The cursor will then encounter the modified document again, leading to duplicate processing and infinite loops (e.g., escaping it multiple times to $$${ or $$$${).\n\nTo prevent this, you should sort the query by an immutable indexed field like _id (e.g., new Document("_id", 1)), which guarantees that each document is visited exactly once regardless of physical relocation.

Suggested change

try (Stream<Document> documents = collection.findLazy(Filters.empty(), null, null, null, 0)) {

try (Stream<Document> documents = collection.findLazy(Filters.empty(), new Document("_id", 1), null, null, 0)) {

The Halloween problem should not occur in MongoDB 5+with WiredTiger. Didn't find any place in the code where we sort by _id, so I prefer let this untouched. @david-stephan what do you think?

Should not impact this query since we don't modify the index or use unique index. According to the doc :
Ref: https://www.mongodb.com/docs/manual/core/read-isolation-consistency-recency/?language-no-dependencies=python#cursor-snapshot
Cursor Snapshot
MongoDB cursors can return the same document more than once in some situations. As a cursor returns documents, other operations may interleave with the query. If one of these operations changes the indexed field on the index used by the query, then the cursor could return the same document more than once.

Queries that use unique indexes can, in some cases, return duplicate values. If a cursor using a unique index interleaves with a delete and insert of documents sharing the same unique value, the cursor may return the same unique value twice from different documents.

gemini-code-assist · 2026-08-30T17:18:57Z

+            parsed = OBJECT_MAPPER.readValue(json, new TypeReference<>() {
+            });


Creating a new anonymous TypeReference instance on every call to escapeNestedDocument can lead to excessive class loading and memory overhead, especially when processing a large number of documents. Reusing Map.class directly is cleaner and avoids any anonymous class instantiation.

parsed = OBJECT_MAPPER.readValue(json, Map.class);

This code is only used as part of the migration and only for documents that have to be escaped, which probably represents a very small amount. Keeping as is to avoid new iterations

gemini-code-assist · 2026-08-30T17:18:57Z

+                        fragment = getOrCreateDescriptorReader().readAutomationPackageFragment(fragmentYamlStream, importedFragmentReference, archive.getAutomationPackageName(), packageVersion);
+                        fillAutomationPackageWithImportedFragments(targetPackage, fragment, archive, packageVersion);


Reassigning the method parameter fragment inside the loop is highly discouraged and can lead to subtle bugs or confusion (for instance, if the parameter is accessed later or if readers assume it refers to the original input). It is cleaner and safer to use a local variable for the imported fragment.

Suggested change

fragment = getOrCreateDescriptorReader().readAutomationPackageFragment(fragmentYamlStream, importedFragmentReference, archive.getAutomationPackageName(), packageVersion);

fillAutomationPackageWithImportedFragments(targetPackage, fragment, archive, packageVersion);

AutomationPackageFragmentYaml importedFragment = getOrCreateDescriptorReader().readAutomationPackageFragment(fragmentYamlStream, importedFragmentReference, archive.getAutomationPackageName(), packageVersion);

fillAutomationPackageWithImportedFragments(targetPackage, importedFragment, archive, packageVersion);

Agree. Changed

gemini-code-assist · 2026-08-30T17:18:57Z

+     */
+    protected MigrationManager initMigrationManager() {
+        MigrationManager migrationManager = new MigrationManager();
+        try (AnnotationScanner annotationScanner = AnnotationScanner.forAllClassesFromClassLoader(AutomationPackageMigration.LOCATION, Thread.currentThread().getContextClassLoader())) {


Classpath scanning using AnnotationScanner is a heavy operation that can take hundreds of milliseconds or more. Performing this scan inside the constructor initMigrationManager() means it will run every time an AutomationPackageDescriptorReader is instantiated. Consider caching the scanned migration classes statically or initializing the migration manager once to avoid performance degradation.

The AutomationPackageDescriptorReader is cached in the AutomationPackageReader which again should be instantiated once in the controller. But it's a valid point and we should check how it behaves, especially in the context of the CLI and Junit runner. Keeping open

david-stephan

The implementation of the feature looks good with some comments. For the migration part is looks complex and more difficult to assess the risks. Also seeing the size of the migration notes might scare / confuse some users. I wonder, given that values with such syntax "${" are probably very rare, if we should not have just notified users in the migration notes and add a step property to opt out (i.e. disable interpolation) ....

david-stephan · 2026-08-31T12:17:41Z

@@ -1,4 +1,4 @@
-schemaVersion: 1.0.0
+version: 1.3.0


why did we update all versions of our test resources, they were written for that version, and since there are no content changes they should still be compatible with that version. If we really want to keep it we should ensure we have tests for all previous version.

david-stephan · 2026-08-31T12:45:22Z


+    /**
+     * Applies the migrations of the automation package format to a descriptor or fragment declaring an older schema
+     * version. This concerns the body of the file itself, the plans it contains are migrated by the yaml plan reader.


concretely the plans are part of the body too, not sure what a better world would be.

david-stephan · 2026-08-31T12:54:06Z

@@ -80,15 +97,28 @@ protected Class<? extends AutomationPackageDescriptorYaml> getDescriptorClass()
    }

    public AutomationPackageFragmentYaml readAutomationPackageFragment(InputStream yamlFragment, String fragmentName, String packageName) throws AutomationPackageReadingException {


no usages found

david-stephan · 2026-08-31T13:22:16Z

+ * of the values reaching a DynamicValue is wider, see the migration documentation.
+ */
+@YamlPlanMigration
+public class EscapeStringInterpolationYamlMigrationTask extends AbstractYamlPlanMigrationTask {


We should start using the same convention as for the controller DB migration task V1_3_0_...

david-stephan · 2026-08-31T13:24:33Z

+ * it did. Authors opt into the interpolation by bumping the {@code version} of their descriptor and removing the
+ * escaping where they actually want a placeholder.
+ * <p>
+ * <b>The field set below is frozen.</b> It describes where a value sits in the schemas up to 1.2.0, which is closed:


I had the same issue when reading the javadoc for the DB migration tasks, this is generic for any migration tasks, it's written for the targeted version and should never be modified later, right?

david-stephan · 2026-08-31T14:35:28Z

+        if (cached != null) {
+            return cached;
+        }
+        // Malformed strings aren't cached. They are rare and are re-parsed on every evaluation


comment is a bit weird, next lines does the parsing (always) but the cache is only updated if no StringInterpolationException is thrown

david-stephan · 2026-08-31T14:42:00Z

+        int literalOffset = 0;
+        int length = source.length();
+        int i = 0;
+        while (i < length) {


Feels a bit old school, but I guess it's fine

david-stephan · 2026-08-31T14:44:34Z

+                    clearValue.append(hasProtectedAccess ? String.valueOf(protectedVariable.value) : protectedVariable.obfuscatedValue);
+                    containsProtectedValues = true;
+                } else {
+                    // Covers GString results as well, whose toString() renders the interpolated value


we are in the else block of ProtectedVariable, so I'm not sure I get the comment

david-stephan · 2026-08-31T14:51:51Z

-                Object protectedResult = null;
+            dynamicValue.evalutationResult = getEvaluationResult(dynamicValue.expression, bindings, dynamicValue.hasProtectedAccess());
+        } else {
+            // A null result means that the value requires no interpolation and is to be returned as is


comment is not clear or not adapted for the next line of code

david-stephan · 2026-08-31T14:54:03Z

+        } catch (Exception e) {
+            result.setEvaluationException(e);
+        }
+        return result;


I would expect to return null when there is nothing to interpolate, something is not clear to me.

jeromecomte added 7 commits August 28, 2026 09:44

SED-4906 Support string interpolation in plan values - Step 1 - 3

26cd03d

SED-4906 Support string interpolation in plan values - Step 4

a21df29

SED-4906 Support string interpolation in plan values - Fixing issue i…

0277d4a

…n DynamiceBeanResolver

SED-4906 Support string interpolation in plan values - Adding test co…

1913905

…verage for yaml plans

SED-4906 Support string interpolation in plan values - Implementing m…

7925bd9

…igration tasks

SED-4906 Support string interpolation in plan values - Simplifying es…

ee3bfa5

…caping

SED-4906 Support string interpolation in plan values - Implementing m…

ef6087a

…igration task for parameters

gemini-code-assist Bot reviewed Aug 30, 2026

View reviewed changes

SED-4906 Support string interpolation in plan values - PR Feedbacks

9afa6db

jeromecomte marked this pull request as ready for review August 31, 2026 07:16

jeromecomte requested a review from david-stephan August 31, 2026 07:16

david-stephan reviewed Aug 31, 2026

View reviewed changes

	try (Stream<Document> documents = collection.findLazy(Filters.empty(), null, null, null, 0)) {
	try (Stream<Document> documents = collection.findLazy(Filters.empty(), new Document("_id", 1), null, null, 0)) {

		parsed = OBJECT_MAPPER.readValue(json, new TypeReference<>() {
		});

		fragment = getOrCreateDescriptorReader().readAutomationPackageFragment(fragmentYamlStream, importedFragmentReference, archive.getAutomationPackageName(), packageVersion);
		fillAutomationPackageWithImportedFragments(targetPackage, fragment, archive, packageVersion);

		@@ -80,15 +97,28 @@ protected Class<? extends AutomationPackageDescriptorYaml> getDescriptorClass()
		}

		public AutomationPackageFragmentYaml readAutomationPackageFragment(InputStream yamlFragment, String fragmentName, String packageName) throws AutomationPackageReadingException {

Uh oh!

Conversation

jeromecomte commented Aug 30, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Aug 30, 2026

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!

gemini-code-assist Bot Aug 30, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Aug 30, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Aug 30, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-stephan left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants