Skip to content

Commit b581783

Browse files
lcawllcawl
committed
Fix placeholder validation
1 parent 96b506f commit b581783

3 files changed

Lines changed: 171 additions & 12 deletions

File tree

‎src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs‎

Lines changed: 71 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,11 +31,41 @@ internal static class BundleDescriptionSubstitution
3131
/// <summary>
3232
/// Substitutes placeholders in a description string with provided values
3333
/// </summary>
34-
public static string SubstitutePlaceholders(string description, string? version, string? lifecycle, string? owner, string? repo)
34+
public static string SubstitutePlaceholders(string description, string? version, string? lifecycle, string? owner, string? repo) =>
35+
SubstitutePlaceholders(description, version, lifecycle, owner, repo, validateResolvable: false);
36+
37+
/// <summary>
38+
/// Substitutes placeholders in a description string with provided values
39+
/// </summary>
40+
/// <param name="description">The description string containing placeholders</param>
41+
/// <param name="version">Version value for {version} placeholder</param>
42+
/// <param name="lifecycle">Lifecycle value for {lifecycle} placeholder</param>
43+
/// <param name="owner">Owner value for {owner} placeholder</param>
44+
/// <param name="repo">Repository value for {repo} placeholder</param>
45+
/// <param name="validateResolvable">If true, validates that all used placeholders can be resolved</param>
46+
/// <returns>Description with placeholders substituted</returns>
47+
/// <exception cref="InvalidOperationException">When validateResolvable is true and placeholders cannot be resolved</exception>
48+
public static string SubstitutePlaceholders(string description, string? version, string? lifecycle, string? owner, string? repo, bool validateResolvable)
3549
{
3650
if (string.IsNullOrEmpty(description))
3751
return description;
3852

53+
if (validateResolvable)
54+
{
55+
var missingValues = new List<string>();
56+
if (description.Contains("{version}") && string.IsNullOrEmpty(version))
57+
missingValues.Add("version");
58+
if (description.Contains("{lifecycle}") && string.IsNullOrEmpty(lifecycle))
59+
missingValues.Add("lifecycle");
60+
if (description.Contains("{owner}") && string.IsNullOrEmpty(owner))
61+
missingValues.Add("owner");
62+
if (description.Contains("{repo}") && string.IsNullOrEmpty(repo))
63+
missingValues.Add("repo");
64+
65+
if (missingValues.Count > 0)
66+
throw new InvalidOperationException($"Cannot resolve placeholders: {string.Join(", ", missingValues)}");
67+
}
68+
3969
return description
4070
.Replace("{version}", version ?? string.Empty)
4171
.Replace("{lifecycle}", lifecycle ?? string.Empty)
@@ -205,6 +235,9 @@ public async Task<bool> BundleChangelogs(IDiagnosticsCollector collector, Bundle
205235
if (!ValidateInput(collector, input))
206236
return false;
207237

238+
if (!ValidatePlaceholderUsage(collector, input))
239+
return false;
240+
208241
if (!ValidateLinkAllowlist(collector, input))
209242
return false;
210243

@@ -455,16 +488,27 @@ public async Task<bool> BundleChangelogs(IDiagnosticsCollector collector, Bundle
455488
{
456489
// Validate placeholder usage in profile mode
457490
var hasVersionPlaceholder = descriptionTemplate.Contains("{version}") || descriptionTemplate.Contains("{lifecycle}");
491+
var hasOwnerRepoPlaceholder = descriptionTemplate.Contains("{owner}") || descriptionTemplate.Contains("{repo}");
492+
458493
if (hasVersionPlaceholder &&
459494
filterResult.Version == "unknown" &&
460495
string.IsNullOrEmpty(profile.OutputProducts))
461496
{
462497
collector.EmitError(string.Empty,
463-
$"Profile '{input.Profile}' uses placeholders in description but no version is available for substitution. " +
498+
$"Profile '{input.Profile}' uses {{version}} or {{lifecycle}} placeholders in description but no version is available for substitution. " +
464499
"Either provide a version argument, or add 'output_products' pattern to the profile configuration.");
465500
return null;
466501
}
467502

503+
if (hasOwnerRepoPlaceholder &&
504+
(string.IsNullOrEmpty(owner) || string.IsNullOrEmpty(repo)))
505+
{
506+
collector.EmitError(string.Empty,
507+
$"Profile '{input.Profile}' uses {{owner}} or {{repo}} placeholders in description but values are not resolvable. " +
508+
"Ensure repository metadata is available in the configuration.");
509+
return null;
510+
}
511+
468512
profileDescription = BundleDescriptionSubstitution.SubstitutePlaceholders(
469513
descriptionTemplate, filterResult.Version, resolvedLifecycle, owner, repo);
470514
}
@@ -628,6 +672,31 @@ private bool ValidateInput(IDiagnosticsCollector collector, BundleChangelogsArgu
628672
return true;
629673
}
630674

675+
private static bool ValidatePlaceholderUsage(IDiagnosticsCollector collector, BundleChangelogsArguments input)
676+
{
677+
// Only validate in option-based mode (profile mode has separate validation)
678+
if (!string.IsNullOrEmpty(input.Profile))
679+
return true;
680+
681+
if (string.IsNullOrEmpty(input.Description))
682+
return true;
683+
684+
var hasPlaceholders = input.Description.Contains("{version}") ||
685+
input.Description.Contains("{lifecycle}") ||
686+
input.Description.Contains("{owner}") ||
687+
input.Description.Contains("{repo}");
688+
689+
if (hasPlaceholders && (input.OutputProducts == null || input.OutputProducts.Count == 0))
690+
{
691+
collector.EmitError(string.Empty,
692+
"When using placeholders in bundle description in option-based mode, " +
693+
"--output-products must be explicitly specified to ensure predictable substitution values.");
694+
return false;
695+
}
696+
697+
return true;
698+
}
699+
631700
private static bool ValidateLinkAllowlist(IDiagnosticsCollector collector, BundleChangelogsArguments input)
632701
{
633702
if (input.LinkAllowRepos == null)

‎src/tooling/docs-builder/Commands/ChangelogCommand.cs‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -809,16 +809,6 @@ public async Task<int> Bundle(
809809
Description = description
810810
};
811811

812-
// Validate placeholder usage in option-based mode
813-
if (!isProfileMode && description != null &&
814-
(description.Contains("{version}") || description.Contains("{lifecycle}") || description.Contains("{owner}") || description.Contains("{repo}")) &&
815-
outputProducts == null)
816-
{
817-
collector.EmitError(string.Empty,
818-
"When using placeholders in --description in option-based mode, --output-products must be explicitly specified to ensure predictable substitution values.");
819-
return 1;
820-
}
821-
822812
serviceInvoker.AddCommand(service, input,
823813
async static (s, collector, state, ctx) => await s.BundleChangelogs(collector, state, ctx)
824814
);

‎tests/Elastic.Changelog.Tests/Changelogs/BundleChangelogsTests.cs‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
using Elastic.Changelog.Bundling;
88
using Elastic.Documentation.Configuration;
99
using Elastic.Documentation.Diagnostics;
10+
using Microsoft.Extensions.Logging.Abstractions;
1011

1112
namespace Elastic.Changelog.Tests.Changelogs;
1213

@@ -5959,4 +5960,103 @@ public async Task BundleChangelogs_PartialPerProductRules_AllOrNothingReplacemen
59595960
var bundleContent = await FileSystem.File.ReadAllTextAsync(outputPath, TestContext.Current.CancellationToken);
59605961
bundleContent.Should().Contain("1755268204-partial-rule.yaml", "entry should be included - per-product rule ignores global type exclusions");
59615962
}
5963+
5964+
[Fact]
5965+
public async Task BundleChangelogs_OptionModeWithPlaceholdersButNoOutputProducts_ReturnsError()
5966+
{
5967+
// Arrange
5968+
CreateSampleChangelogs();
5969+
var input = new BundleChangelogsArguments
5970+
{
5971+
Directory = _changelogDir,
5972+
All = true,
5973+
Output = "bundle.yaml",
5974+
Description = "Release includes {version} with {lifecycle} features from {owner}/{repo}" // Has placeholders but no --output-products
5975+
};
5976+
5977+
// Act
5978+
var result = await Service.BundleChangelogs(Collector, input, TestContext.Current.CancellationToken);
5979+
5980+
// Assert
5981+
result.Should().BeFalse("bundling should fail when placeholders are used without --output-products");
5982+
Collector.Errors.Should().Be(1, "should have exactly one validation error");
5983+
Collector.Diagnostics.Should().Contain(d => d.Message.Contains(
5984+
"When using placeholders in bundle description in option-based mode, --output-products must be explicitly specified to ensure predictable substitution values."));
5985+
}
5986+
5987+
[Fact]
5988+
public async Task BundleChangelogs_OptionModeWithPlaceholdersAndOutputProducts_Succeeds()
5989+
{
5990+
// Arrange
5991+
CreateSampleChangelogs();
5992+
var outputProducts = new List<ProductArgument>
5993+
{
5994+
new() { Product = "elasticsearch", Target = "9.2.0", Lifecycle = "ga" }
5995+
};
5996+
5997+
var input = new BundleChangelogsArguments
5998+
{
5999+
Directory = _changelogDir,
6000+
All = true,
6001+
Output = "bundle.yaml",
6002+
OutputProducts = outputProducts,
6003+
Description = "Release includes {version} with {lifecycle} features from {owner}/{repo}",
6004+
Owner = "elastic",
6005+
Repo = "elasticsearch"
6006+
};
6007+
6008+
// Act
6009+
var result = await Service.BundleChangelogs(Collector, input, TestContext.Current.CancellationToken);
6010+
6011+
// Assert
6012+
result.Should().BeTrue("bundling should succeed when placeholders have --output-products");
6013+
Collector.Errors.Should().Be(0, "no errors expected when validation passes");
6014+
6015+
var bundleContent = await FileSystem.File.ReadAllTextAsync("bundle.yaml", TestContext.Current.CancellationToken);
6016+
bundleContent.Should().Contain("Release includes 9.2.0 with ga features from elastic/elasticsearch",
6017+
"placeholders should be substituted correctly");
6018+
}
6019+
6020+
[Fact]
6021+
public async Task BundleChangelogs_OptionModeWithConfigDescriptionAndPlaceholders_ReturnsError()
6022+
{
6023+
// Arrange - test validation with description that could come from config (simulated via CLI)
6024+
CreateSampleChangelogs();
6025+
var input = new BundleChangelogsArguments
6026+
{
6027+
Directory = _changelogDir,
6028+
All = true,
6029+
Output = "bundle.yaml",
6030+
Description = "Version {version} includes {lifecycle} updates from {owner}/{repo}" // Simulate config-provided description
6031+
// No OutputProducts - should fail validation
6032+
};
6033+
6034+
// Act
6035+
var result = await Service.BundleChangelogs(Collector, input, TestContext.Current.CancellationToken);
6036+
6037+
// Assert
6038+
result.Should().BeFalse("bundling should fail when description has placeholders without --output-products");
6039+
Collector.Errors.Should().Be(1, "should have exactly one validation error");
6040+
Collector.Diagnostics.Should().Contain(d => d.Message.Contains(
6041+
"When using placeholders in bundle description in option-based mode, --output-products must be explicitly specified to ensure predictable substitution values."));
6042+
}
6043+
6044+
6045+
private void CreateSampleChangelogs()
6046+
{
6047+
// language=yaml
6048+
var changelog1 =
6049+
"""
6050+
title: First changelog
6051+
type: feature
6052+
products:
6053+
- product: elasticsearch
6054+
target: 9.2.0
6055+
lifecycle: ga
6056+
prs:
6057+
- https://github.com/elastic/elasticsearch/pull/100
6058+
""";
6059+
6060+
FileSystem.File.WriteAllText(FileSystem.Path.Join(_changelogDir, "changelog1.yaml"), changelog1);
6061+
}
59626062
}

0 commit comments

Comments
 (0)