-
Notifications
You must be signed in to change notification settings - Fork 34
feat: add --preview flag to preview aviator remediations before applying #1076
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.
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
base: feat/v3.x/aviator/26.4
Are you sure you want to change the base?
Changes from all commits
3864b09
205ecde
7bcba4a
7333299
9318199
00ec633
85bc159
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| /* | ||
| * Copyright 2021-2026 Open Text. | ||
| * | ||
| * The only warranties for products and services of Open Text | ||
| * and its affiliates and licensors ("Open Text") are as may | ||
| * be set forth in the express warranty statements accompanying | ||
| * such products and services. Nothing herein should be construed | ||
| * as constituting an additional warranty. Open Text shall not be | ||
| * liable for technical or editorial errors or omissions contained | ||
| * herein. The information contained herein is subject to change | ||
| * without notice. | ||
| */ | ||
| package com.fortify.cli.aviator._common.cli.mixin; | ||
|
|
||
| import java.util.List; | ||
|
|
||
| import com.fortify.cli.aviator._common.remediations_cache.IApplyRemediationsOptions; | ||
|
|
||
| import lombok.Getter; | ||
| import picocli.CommandLine.Option; | ||
|
|
||
| /** Shared apply-remediations options; used as a @Mixin in AbstractAviatorApplyRemediationsCommand. */ | ||
| @Getter | ||
| public final class ApplyRemediationsOptionsMixin implements IApplyRemediationsOptions { | ||
| @Option(names = {"--source-dir"}) | ||
| private String sourceCodeDirectory = System.getProperty("user.dir"); | ||
| @Option(names = {"--issue-ids"}, split = ",") | ||
| private List<String> issueIds; | ||
| @Option(names = {"--preview"}) | ||
| private boolean previewMode = false; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| /* | ||
| * Copyright 2021-2026 Open Text. | ||
| * | ||
| * The only warranties for products and services of Open Text | ||
| * and its affiliates and licensors ("Open Text") are as may | ||
| * be set forth in the express warranty statements accompanying | ||
| * such products and services. Nothing herein should be construed | ||
| * as constituting an additional warranty. Open Text shall not be | ||
| * liable for technical or editorial errors or omissions contained | ||
| * herein. The information contained herein is subject to change | ||
| * without notice. | ||
| */ | ||
| package com.fortify.cli.aviator._common.output.cli.cmd; | ||
|
|
||
| import java.util.List; | ||
| import java.util.Set; | ||
|
|
||
| import com.fasterxml.jackson.databind.JsonNode; | ||
| import com.fortify.cli.aviator._common.cli.mixin.ApplyRemediationsOptionsMixin; | ||
| import com.fortify.cli.aviator._common.remediations_cache.IRemediationsFprSource; | ||
| import com.fortify.cli.aviator._common.remediations_cache.RemediationsApplyHelper; | ||
| import com.fortify.cli.aviator._common.remediations_cache.RemediationsApplyHelper.ApplyResult; | ||
| import com.fortify.cli.aviator._common.util.AviatorIssueIdFilterUtils; | ||
| import com.fortify.cli.aviator.config.AviatorLoggerImpl; | ||
| import com.fortify.cli.common.exception.FcliSimpleException; | ||
| import com.fortify.cli.common.output.cli.cmd.AbstractOutputCommand; | ||
| import com.fortify.cli.common.output.cli.cmd.IJsonNodeSupplier; | ||
| import com.fortify.cli.common.output.cli.mixin.OutputHelperMixins; | ||
| import com.fortify.cli.common.output.transform.IRecordTransformer; | ||
| import com.fortify.cli.common.progress.cli.mixin.ProgressWriterFactoryMixin; | ||
| import com.fortify.cli.common.progress.helper.IProgressWriter; | ||
|
|
||
| import lombok.Getter; | ||
| import picocli.CommandLine.Mixin; | ||
|
|
||
| public abstract class AbstractAviatorApplyRemediationsCommand extends AbstractOutputCommand | ||
| implements IJsonNodeSupplier, IRecordTransformer { | ||
|
|
||
| @Getter @Mixin private OutputHelperMixins.DetailsNoQuery outputHelper; | ||
| @Mixin private ProgressWriterFactoryMixin progressWriterFactoryMixin; | ||
| @Mixin protected ApplyRemediationsOptionsMixin applyOptions; | ||
|
|
||
| @Override | ||
| public final JsonNode getJsonNode() { | ||
| validateSourceSelector(); | ||
| requireSourceDir(); | ||
| requireIssueIdsCacheOnly(); | ||
| Set<String> issueIdFilter = AviatorIssueIdFilterUtils.normalizeIssueIds(applyOptions.getIssueIds()); | ||
| try (IProgressWriter progressWriter = progressWriterFactoryMixin.create()) { | ||
| AviatorLoggerImpl logger = new AviatorLoggerImpl(progressWriter); | ||
| try (IRemediationsFprSource source = openFprSource(logger, progressWriter)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| ApplyResult result = RemediationsApplyHelper.apply(source, applyOptions, issueIdFilter, logger); | ||
| return buildResultNode(source, result, issueIdFilter); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** Override to validate source selector state before options are checked; no-op by default. */ | ||
| protected void validateSourceSelector() {} | ||
|
|
||
| protected abstract boolean isCacheMode(); | ||
|
|
||
| protected abstract IRemediationsFprSource openFprSource(AviatorLoggerImpl logger, IProgressWriter progressWriter); | ||
|
|
||
| protected abstract JsonNode buildResultNode(IRemediationsFprSource source, ApplyResult result, Set<String> issueIdFilter); | ||
|
|
||
| @Override | ||
| public final boolean isSingular() { return true; } | ||
|
|
||
| @Override | ||
| public JsonNode transformRecord(JsonNode record) { return record; } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As this is a no-op, better to remove this method and the corresponding |
||
|
|
||
| private void requireSourceDir() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given the default value for |
||
| FcliSimpleException.throwIf(applyOptions.getSourceCodeDirectory() == null || applyOptions.getSourceCodeDirectory().isBlank(), | ||
| "--source-dir must specify a valid directory path"); | ||
| } | ||
|
|
||
| private void requireIssueIdsCacheOnly() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to the above; as exception references option names, better to move this validation to the mixin. |
||
| List<String> issueIds = applyOptions.getIssueIds(); | ||
| FcliSimpleException.throwIf( | ||
| issueIds != null && !issueIds.isEmpty() && !isCacheMode(), | ||
| "--issue-ids can only be used with --from-cache; " | ||
| + "create a cache with download-remediations-cache and rerun with --from-cache"); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| /* | ||
| * Copyright 2021-2026 Open Text. | ||
| * | ||
| * The only warranties for products and services of Open Text | ||
| * and its affiliates and licensors ("Open Text") are as may | ||
| * be set forth in the express warranty statements accompanying | ||
| * such products and services. Nothing herein should be construed | ||
| * as constituting an additional warranty. Open Text shall not be | ||
| * liable for technical or editorial errors or omissions contained | ||
| * herein. The information contained herein is subject to change | ||
| * without notice. | ||
| */ | ||
| package com.fortify.cli.aviator._common.remediations_cache; | ||
|
|
||
| import java.util.List; | ||
|
|
||
| /** Abstraction over the shared apply-remediations CLI options, allowing RemediationsApplyHelper | ||
| * to remain independent of concrete Picocli types. */ | ||
| public interface IApplyRemediationsOptions { | ||
| String getSourceCodeDirectory(); | ||
| List<String> getIssueIds(); | ||
| boolean isPreviewMode(); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,8 +19,6 @@ | |
| import java.util.List; | ||
| import java.util.Set; | ||
|
|
||
| import org.slf4j.Logger; | ||
|
|
||
| import com.fortify.cli.aviator._common.exception.AviatorSimpleException; | ||
| import com.fortify.cli.aviator._common.util.AviatorRemediationMetricsHelper; | ||
| import com.fortify.cli.aviator.applyRemediation.ApplyAutoRemediationOnSource; | ||
|
|
@@ -29,10 +27,13 @@ | |
| import com.fortify.cli.aviator.util.FprHandle; | ||
| import com.fortify.cli.common.exception.FcliTechnicalException; | ||
|
|
||
| import lombok.extern.slf4j.Slf4j; | ||
|
|
||
| /** | ||
| * Single apply-remediations loop for any {@link IRemediationsFprSource} | ||
| * (cache zip entries or online downloads). Soft-skips on {@link AviatorSimpleException}. | ||
| */ | ||
| @Slf4j | ||
| public final class RemediationsApplyHelper { | ||
| private RemediationsApplyHelper() {} | ||
|
|
||
|
|
@@ -43,22 +44,23 @@ public record ApplyResult( | |
| List<RemediationMetric> metrics) {} | ||
|
|
||
| /** | ||
| * Applies remediations for each source entry until done or the issue-id filter is exhausted. | ||
| * Applies or previews remediations for each source entry until done or the issue-id filter is exhausted. | ||
| * Caller owns {@code source} lifecycle (try-with-resources). | ||
| */ | ||
| public static ApplyResult apply( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As the number of parameters keep growing, maybe better to combine in a single object that implements builder pattern (through Lombok Same could potentially be applied to other methods in Aviator code to reduce number of parameters. |
||
| IRemediationsFprSource source, | ||
| String sourceCodeDirectory, | ||
| IAviatorLogger logger, | ||
| IApplyRemediationsOptions options, | ||
| Set<String> issueIdFilter, | ||
| Logger skipLog) { | ||
| IAviatorLogger logger) { | ||
| Accumulator acc = new Accumulator(issueIdFilter); | ||
| source.forEachEntry((fprPath, label, id, index, total) -> { | ||
| if (acc.remaining != null && acc.remaining.isEmpty()) { | ||
| return false; | ||
| } | ||
| RemediationMetric metric = applyOne( | ||
| fprPath, label, index, total, sourceCodeDirectory, logger, acc.remaining, skipLog); | ||
| fprPath, label, index, total, | ||
| options.getSourceCodeDirectory(), logger, acc.remaining, | ||
| options.isPreviewMode()); | ||
| if (metric == null) { | ||
| acc.skipped++; | ||
| } else { | ||
|
|
@@ -85,13 +87,13 @@ private static RemediationMetric applyOne( | |
| String sourceCodeDirectory, | ||
| IAviatorLogger logger, | ||
| Set<String> issueFilter, | ||
| Logger skipLog) { | ||
| boolean previewMode) { | ||
| logger.progress("Processing FPR " + index + "/" + total + " (" + entryLabel + ")"); | ||
| logger.progress("Status: Processing FPR with Aviator for Applying Auto Remediations"); | ||
| logger.progress("Status: Processing FPR with Aviator for " + (previewMode ? "Previewing" : "Applying") + " Auto Remediations"); | ||
| try (FprHandle fprHandle = new FprHandle(fprPath)) { | ||
| return ApplyAutoRemediationOnSource.applyRemediations(fprHandle, sourceCodeDirectory, logger, issueFilter); | ||
| return ApplyAutoRemediationOnSource.applyRemediations(fprHandle, sourceCodeDirectory, logger, issueFilter, previewMode); | ||
| } catch (AviatorSimpleException e) { | ||
| skipLog.warn("Skipping entry {} as {}", entryLabel, e.getMessage()); | ||
| log.warn("Skipping entry {} as {}", entryLabel, e.getMessage()); | ||
| return null; | ||
| } catch (IOException e) { | ||
| throw new FcliTechnicalException("Failed to close FPR handle for entry " + entryLabel, e); | ||
|
|
||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,7 @@ | |
| import com.fasterxml.jackson.databind.node.ArrayNode; | ||
| import com.fasterxml.jackson.databind.node.ObjectNode; | ||
| import com.fortify.cli.aviator.fpr.processor.RemediationProcessor.RemediationMetric; | ||
| import com.fortify.cli.aviator.fpr.processor.preview.PreviewDetail; | ||
| import com.fortify.cli.common.json.JsonHelper; | ||
| import com.fortify.cli.common.output.transform.IActionCommandResultSupplier; | ||
|
|
||
|
|
@@ -38,25 +39,49 @@ private AviatorRemediationMetricsHelper() {} | |
| * aggregation (XML totals); non-null selects filtered aggregation (requested IDs). | ||
| */ | ||
| public static RemediationMetric aggregateMetrics(Set<String> requestedIssueIds, Collection<RemediationMetric> metrics) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems a fairly long method; can we improve this through self-describing sub-methods, and/or through utility (builder) methods on |
||
| Collection<RemediationMetric> safeMetrics = metrics == null ? List.of() : metrics; | ||
| return requestedIssueIds == null | ||
| ? aggregateUnfiltered(safeMetrics) | ||
| : aggregateFiltered(requestedIssueIds, safeMetrics); | ||
| } | ||
|
|
||
| private static RemediationMetric aggregateUnfiltered(Collection<RemediationMetric> metrics) { | ||
| int totalRemediations = 0, appliedRemediations = 0; | ||
| Set<String> modifiedFiles = new LinkedHashSet<>(); | ||
| Map<String, Integer> skippedByReason = new LinkedHashMap<>(); | ||
| Collection<RemediationMetric> safeMetrics = metrics == null ? List.of() : metrics; | ||
| if (requestedIssueIds == null) { | ||
| int totalRemediations = 0; | ||
| int appliedRemediations = 0; | ||
| for (RemediationMetric metric : safeMetrics) { | ||
| totalRemediations += metric.totalRemediations(); | ||
| appliedRemediations += metric.appliedRemediations(); | ||
| accumulateFilesAndSkips(metric, modifiedFiles, skippedByReason); | ||
| List<PreviewDetail> previewDetails = new ArrayList<>(); | ||
| boolean previewMode = false; | ||
| for (RemediationMetric metric : metrics) { | ||
| totalRemediations += metric.totalRemediations(); | ||
| appliedRemediations += metric.appliedRemediations(); | ||
| accumulateFilesAndSkips(metric, modifiedFiles, skippedByReason); | ||
| if (metric instanceof RemediationMetric.Preview preview) { | ||
| previewMode = true; | ||
| previewDetails.addAll(preview.previewDetails()); | ||
| } | ||
| return RemediationMetric.unfiltered(totalRemediations, appliedRemediations, modifiedFiles, skippedByReason); | ||
| } | ||
| return previewMode | ||
| ? RemediationMetric.previewUnfiltered(totalRemediations, appliedRemediations, modifiedFiles, skippedByReason, previewDetails) | ||
| : RemediationMetric.unfiltered(totalRemediations, appliedRemediations, modifiedFiles, skippedByReason); | ||
| } | ||
|
|
||
| private static RemediationMetric aggregateFiltered(Set<String> requestedIssueIds, Collection<RemediationMetric> metrics) { | ||
| Set<String> appliedIssueIds = new LinkedHashSet<>(); | ||
| for (RemediationMetric metric : safeMetrics) { | ||
| Set<String> modifiedFiles = new LinkedHashSet<>(); | ||
| Map<String, Integer> skippedByReason = new LinkedHashMap<>(); | ||
| List<PreviewDetail> previewDetails = new ArrayList<>(); | ||
| boolean previewMode = false; | ||
| for (RemediationMetric metric : metrics) { | ||
| appliedIssueIds.addAll(metric.appliedIssueIds()); | ||
| accumulateFilesAndSkips(metric, modifiedFiles, skippedByReason); | ||
| if (metric instanceof RemediationMetric.Preview preview) { | ||
| previewMode = true; | ||
| previewDetails.addAll(preview.previewDetails()); | ||
| } | ||
| } | ||
| return RemediationMetric.filtered(requestedIssueIds, appliedIssueIds, modifiedFiles, skippedByReason); | ||
| return previewMode | ||
| ? RemediationMetric.previewFiltered(requestedIssueIds, appliedIssueIds, modifiedFiles, skippedByReason, previewDetails) | ||
| : RemediationMetric.filtered(requestedIssueIds, appliedIssueIds, modifiedFiles, skippedByReason); | ||
| } | ||
|
|
||
| private static void accumulateFilesAndSkips( | ||
|
|
@@ -92,7 +117,12 @@ public static String formatSkippedReasons(Map<String, Integer> skippedByReason) | |
| } | ||
|
|
||
| public static String actionLabel(RemediationMetric metric) { | ||
| return metric != null && metric.appliedRemediations() > 0 ? "Remediation-Applied" : "No-Remediation-Applied"; | ||
| boolean previewMode = metric instanceof RemediationMetric.Preview; | ||
| if (metric != null && metric.appliedRemediations() > 0) { | ||
| return previewMode ? "Remediation-Previewed" : "Remediation-Applied"; | ||
| } else { | ||
| return previewMode ? "No-Remediation-Previewed" : "No-Remediation-Applied"; | ||
| } | ||
| } | ||
|
|
||
| public static String na(String value) { | ||
|
|
@@ -118,10 +148,22 @@ public static void putRemediationMetricFields(ObjectNode result, RemediationMetr | |
| result.set("modifiedFiles", toArrayNode(modifiedFiles)); | ||
| } | ||
|
|
||
| /** Metric fields plus {@code __action__} (shared by SSC/FoD result builders). */ | ||
| /** Metric fields plus {@code __action__} and, for preview results, preview details (shared by SSC/FoD result builders). */ | ||
| public static void putMetricAndAction(ObjectNode result, RemediationMetric metric) { | ||
| putRemediationMetricFields(result, metric); | ||
| result.put(IActionCommandResultSupplier.actionFieldName, actionLabel(metric)); | ||
|
|
||
| if (metric instanceof RemediationMetric.Preview preview) { | ||
| result.set("previewDetails", toPreviewDetailsArray(preview.previewDetails())); | ||
| } | ||
| } | ||
|
|
||
| private static ArrayNode toPreviewDetailsArray(List<?> previewDetails) { | ||
| ArrayNode array = JsonHelper.getObjectMapper().createArrayNode(); | ||
| if (previewDetails != null) { | ||
| previewDetails.forEach(detail -> array.add(JsonHelper.getObjectMapper().valueToTree(detail))); | ||
| } | ||
| return array; | ||
| } | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although much better than before, I think definitions and responsibilities between abstract base class, concrete sub-classes, and options/mixins can be further improved.
Maybe something like the following?
ApplyRemediationsOptionsMixintoAbstractApplyRemediationsOptionsMixinvalidatemethod to this abstract mixin and its interface, calling individual validation methods for validating source selection, source dir,--issue-ids/--from-cacheinterdependency, ...So, basically, generic & command-specific options are provided through single mixin on FoD/SSC command classes, and those mixins provide the necessary validation logic. This allows the abstract base class to focus on process, instead of handling both process and data (validation).