Skip to content

SED-4713 isolated-automation-package-repository-canonical-plan-names-are-not-canonical #657

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
david-stephan wants to merge 2 commits into
base: master
Choose a base branch
from
Open

SED-4713 isolated-automation-package-repository-canonical-plan-names-are-not-canonical #657

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1e86fdc
SED-4713 IsolatedAutomationPackageRepository canonical plan names are…
david-stephan Jun 18, 2026
18f4567
SED-4713 PR feedbacks
david-stephan Jun 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion ...src/main/java/step/automation/packages/execution/IsolatedAutomationPackageRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ public class IsolatedAutomationPackageRepository extends RepositoryWithAutomatio
public static final Logger log = LoggerFactory.getLogger(IsolatedAutomationPackageRepository.class);
public static final String CONTEXT_ID_CUSTOM_FIELD = "contextId";
public static final String LAST_EXECUTION_TIME_CUSTOM_FIELD = "lastExecutionTime";
public static final Set<String> CANONICAL_REPOSITORY_PARAMETER_KEYS = Set.of(RepositoryWithAutomationPackageSupport.AP_NAME,
ArtifactRepositoryConstants.PARAM_INCLUDE_PLANS,
ArtifactRepositoryConstants.PARAM_EXCLUDE_PLANS,
ArtifactRepositoryConstants.PARAM_INCLUDE_CATEGORIES,
ArtifactRepositoryConstants.PARAM_EXCLUDE_CATEGORIES);

@gemini-code-assist gemini-code-assist Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The CANONICAL_REPOSITORY_PARAMETER_KEYS set is missing PARAM_WRAP_PLANS_INTO_TEST_SET and PARAM_ROOT_TYPE. Since these parameters directly affect the structure, name, and type of the imported plan (as seen in getArtefactInfo and isWrapPlansIntoTestSet), omitting them can lead to incorrect plan caching or resolution when these parameters differ between executions.

Suggested change
public static final Set<String> CANONICAL_REPOSITORY_PARAMETER_KEYS = Set.of(RepositoryWithAutomationPackageSupport.AP_NAME,
ArtifactRepositoryConstants.PARAM_INCLUDE_PLANS,
ArtifactRepositoryConstants.PARAM_EXCLUDE_PLANS,
ArtifactRepositoryConstants.PARAM_INCLUDE_CATEGORIES,
ArtifactRepositoryConstants.PARAM_EXCLUDE_CATEGORIES);
public static final Set<String> CANONICAL_REPOSITORY_PARAMETER_KEYS = Set.of(RepositoryWithAutomationPackageSupport.AP_NAME,
ArtifactRepositoryConstants.PARAM_INCLUDE_PLANS,
ArtifactRepositoryConstants.PARAM_EXCLUDE_PLANS,
ArtifactRepositoryConstants.PARAM_INCLUDE_CATEGORIES,
ArtifactRepositoryConstants.PARAM_EXCLUDE_CATEGORIES,
ArtifactRepositoryConstants.PARAM_WRAP_PLANS_INTO_TEST_SET,
ArtifactRepositoryConstants.PARAM_ROOT_TYPE);

@jeromecomte jeromecomte Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree


private final Supplier<String> ttlValueSupplier;
private final Path mavenCachePath;
Expand All @@ -66,7 +71,7 @@ protected IsolatedAutomationPackageRepository(AutomationPackageManager manager,
FunctionAccessor functionAccessor,
Supplier<String> ttlValueSupplier,
Path mavenCachePath) {
super(Set.of(REPOSITORY_PARAM_CONTEXTID), manager, functionTypeRegistry, functionAccessor, resourceManager);
super(CANONICAL_REPOSITORY_PARAMETER_KEYS, manager, functionTypeRegistry, functionAccessor, resourceManager);
this.ttlValueSupplier = ttlValueSupplier;
this.mavenCachePath = mavenCachePath;
}
Expand Down
8 changes: 7 additions & 1 deletion ...positories-artifact/src/main/java/step/repositories/artifact/MavenArtifactRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@
public class MavenArtifactRepository extends AbstractArtifactRepository {

public static final String MAVEN_SETTINGS_PREFIX = ArtifactRepositoryConstants.MAVEN_SETTINGS_PREFIX;
public static final Set<String> CANONICAL_REPOSITORY_PARAMETER_KEYS = Set.of(ArtifactRepositoryConstants.ARTIFACT_PARAM_GROUP_ID,
ArtifactRepositoryConstants.ARTIFACT_PARAM_ARTIFACT_ID, ArtifactRepositoryConstants.ARTIFACT_PARAM_VERSION,
ArtifactRepositoryConstants.PARAM_INCLUDE_PLANS,
ArtifactRepositoryConstants.PARAM_EXCLUDE_PLANS,
ArtifactRepositoryConstants.PARAM_INCLUDE_CATEGORIES,
ArtifactRepositoryConstants.PARAM_EXCLUDE_CATEGORIES);

@gemini-code-assist gemini-code-assist Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The CANONICAL_REPOSITORY_PARAMETER_KEYS set is missing PARAM_WRAP_PLANS_INTO_TEST_SET, ARTIFACT_PARAM_CLASSIFIER, and ARTIFACT_PARAM_TYPE. Since classifier and type uniquely identify the resolved Maven artifact, and wrapPlans determines the structure of the imported plan, omitting them can cause different artifacts or plan structures to share the same canonical parameters, leading to incorrect plan caching or resolution.

Suggested change
public static final Set<String> CANONICAL_REPOSITORY_PARAMETER_KEYS = Set.of(ArtifactRepositoryConstants.ARTIFACT_PARAM_GROUP_ID,
ArtifactRepositoryConstants.ARTIFACT_PARAM_ARTIFACT_ID, ArtifactRepositoryConstants.ARTIFACT_PARAM_VERSION,
ArtifactRepositoryConstants.PARAM_INCLUDE_PLANS,
ArtifactRepositoryConstants.PARAM_EXCLUDE_PLANS,
ArtifactRepositoryConstants.PARAM_INCLUDE_CATEGORIES,
ArtifactRepositoryConstants.PARAM_EXCLUDE_CATEGORIES);
public static final Set<String> CANONICAL_REPOSITORY_PARAMETER_KEYS = Set.of(ArtifactRepositoryConstants.ARTIFACT_PARAM_GROUP_ID,
ArtifactRepositoryConstants.ARTIFACT_PARAM_ARTIFACT_ID, ArtifactRepositoryConstants.ARTIFACT_PARAM_VERSION,
ArtifactRepositoryConstants.PARAM_INCLUDE_PLANS,
ArtifactRepositoryConstants.PARAM_EXCLUDE_PLANS,
ArtifactRepositoryConstants.PARAM_INCLUDE_CATEGORIES,
ArtifactRepositoryConstants.PARAM_EXCLUDE_CATEGORIES,
ArtifactRepositoryConstants.PARAM_WRAP_PLANS_INTO_TEST_SET,
ArtifactRepositoryConstants.ARTIFACT_PARAM_CLASSIFIER,
ArtifactRepositoryConstants.ARTIFACT_PARAM_TYPE);

@jeromecomte jeromecomte Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would feel more consistent to remove the version and keep only groupId + artifactId


private final ControllerSettingAccessor controllerSettingAccessor;
private final File localRepository;
Expand All @@ -49,7 +55,7 @@ public class MavenArtifactRepository extends AbstractArtifactRepository {

public MavenArtifactRepository(AutomationPackageManager manager, FunctionTypeRegistry functionTypeRegistry, FunctionAccessor functionAccessor, Configuration configuration,
ControllerSettingAccessor controllerSettingAccessor, ResourceManager resourceManager) {
super(Set.of(ArtifactRepositoryConstants.ARTIFACT_PARAM_GROUP_ID, ArtifactRepositoryConstants.ARTIFACT_PARAM_ARTIFACT_ID, ArtifactRepositoryConstants.ARTIFACT_PARAM_VERSION), manager, functionTypeRegistry, functionAccessor, resourceManager);
super(CANONICAL_REPOSITORY_PARAMETER_KEYS, manager, functionTypeRegistry, functionAccessor, resourceManager);
localRepository = configuration.getPropertyAsFile(CONFIGURATION_MAVEN_FOLDER, new File(DEFAULT_MAVEN_FOLDER));
this.controllerSettingAccessor = controllerSettingAccessor;
maxAge = Duration.ofMinutes(configuration.getPropertyAsLong(CONFIGURATION_MAVEN_MAX_AGE, DEFAULT_MAVEN_MAX_AGE));
Expand Down