Skip to content

Commit 0bc2de7

Browse files
somiljain2006somiljain2006
authored
Include YAML line number and attribute path in CasC errors (#2775)
1 parent 7d1515a commit 0bc2de7

10 files changed

Lines changed: 186 additions & 58 deletions

File tree

‎plugin/src/main/java/io/jenkins/plugins/casc/BaseConfigurator.java‎

Lines changed: 30 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -349,24 +349,39 @@ protected void configure(Mapping config, T instance, boolean dryrun, Configurati
349349
final Configurator configurator = context.lookupOrFail(k);
350350

351351
final Object valueToSet;
352-
if (attribute.isMultiple()) {
353-
List<Object> values = new ArrayList<>();
354-
for (CNode o : sub.asSequence()) {
355-
Object value = dryrun ? configurator.check(o, context) : configurator.configure(o, context);
356-
values.add(value);
352+
try {
353+
if (attribute.isMultiple()) {
354+
List<Object> values = new ArrayList<>();
355+
for (CNode o : sub.asSequence()) {
356+
Object value = dryrun ? configurator.check(o, context) : configurator.configure(o, context);
357+
values.add(value);
358+
}
359+
valueToSet = values;
360+
} else {
361+
valueToSet = dryrun ? configurator.check(sub, context) : configurator.configure(sub, context);
357362
}
358-
valueToSet = values;
359-
} else {
360-
valueToSet = dryrun ? configurator.check(sub, context) : configurator.configure(sub, context);
361-
}
362363

363-
if (!dryrun) {
364-
try {
365-
((Attribute) attribute)
366-
.setValue(instance, valueToSet); // require type erasure to set Object vs ?
367-
} catch (Exception ex) {
368-
throw new ConfiguratorException(configurator, "Failed to set attribute " + attribute, ex);
364+
if (!dryrun) {
365+
((Attribute) attribute).setValue(instance, valueToSet);
366+
}
367+
} catch (ConfiguratorException ex) {
368+
if (ex instanceof UnknownAttributesException) {
369+
throw ex;
369370
}
371+
String childMessage = ex.getErrorMessage();
372+
373+
String message = StringUtils.isNotBlank(childMessage)
374+
? "Failed to configure '" + attribute.getName() + "': " + childMessage
375+
: "Failed to configure attribute '" + attribute.getName() + "'";
376+
377+
throw ConfiguratorException.from(sub, configurator, attribute.getName(), message, ex.getCause());
378+
} catch (Exception ex) {
379+
throw ConfiguratorException.from(
380+
sub,
381+
this,
382+
attribute.getName(),
383+
"Failed to set attribute '" + attribute.getName() + "'",
384+
ex);
370385
}
371386
}
372387
}

‎plugin/src/main/java/io/jenkins/plugins/casc/ConfigurationAsCode.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -305,7 +305,7 @@ private JSONArray collectProblems(Map<Source, String> issues, String severity) {
305305
.accumulate(
306306
"line",
307307
Optional.ofNullable(e.getKey())
308-
.map(it -> it.line)
308+
.map(Source::line)
309309
.orElse(-1))
310310
.accumulate(severity, e.getValue()))
311311
.forEach(problems::add);
@@ -469,7 +469,7 @@ public void doCheck(StaplerRequest2 req, StaplerResponse2 res) throws Exception
469469
// If there are validation issues, add them to the response array
470470
validationIssues.entrySet().stream()
471471
.map(e -> new JSONObject()
472-
.accumulate("line", e.getKey() != null ? e.getKey().line : -1)
472+
.accumulate("line", e.getKey() != null ? e.getKey().line() : -1)
473473
.accumulate("message", e.getValue()))
474474
.forEach(response::add);
475475

‎plugin/src/main/java/io/jenkins/plugins/casc/ConfiguratorException.java‎

Lines changed: 81 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323

2424
import edu.umd.cs.findbugs.annotations.CheckForNull;
2525
import io.jenkins.plugins.casc.model.CNode;
26+
import io.jenkins.plugins.casc.model.Source;
2627
import java.util.Collections;
2728
import java.util.List;
2829
import jenkins.model.Jenkins;
@@ -44,41 +45,73 @@ public class ConfiguratorException extends RuntimeException {
4445
@CheckForNull
4546
private final String invalidAttribute;
4647

47-
public ConfiguratorException(
48+
@CheckForNull
49+
private final Source source;
50+
51+
@CheckForNull
52+
private final String path;
53+
54+
private ConfiguratorException(
4855
@CheckForNull Configurator configurator,
4956
@CheckForNull String message,
50-
String invalidAttribute,
57+
@CheckForNull String invalidAttribute,
5158
List<String> validAttributes,
52-
@CheckForNull Throwable cause) {
59+
@CheckForNull Throwable cause,
60+
@CheckForNull Source source,
61+
@CheckForNull String path) {
5362
super(message, cause);
5463
this.configurator = configurator;
5564

5665
this.invalidAttribute = invalidAttribute;
57-
this.validAttributes = validAttributes;
66+
this.validAttributes = validAttributes == null ? Collections.emptyList() : validAttributes;
67+
this.source = source;
68+
this.path = path;
5869
}
5970

60-
public ConfiguratorException(
61-
@CheckForNull Configurator configurator, @CheckForNull String message, @CheckForNull Throwable cause) {
62-
super(message, cause);
63-
this.configurator = configurator;
64-
this.invalidAttribute = null;
65-
this.validAttributes = Collections.emptyList();
71+
public static ConfiguratorException from(CNode node, String message) {
72+
return new ConfiguratorException(
73+
null, message, null, Collections.emptyList(), null, node != null ? node.getSource() : null, null);
6674
}
6775

68-
public ConfiguratorException(@CheckForNull String message, @CheckForNull Throwable cause) {
69-
this(null, message, null, Collections.emptyList(), cause);
76+
public static ConfiguratorException from(CNode node, String message, Throwable cause) {
77+
return new ConfiguratorException(
78+
null, message, null, Collections.emptyList(), cause, node != null ? node.getSource() : null, null);
7079
}
7180

72-
public ConfiguratorException(@CheckForNull Configurator configurator, @CheckForNull String message) {
73-
this(configurator, message, null, Collections.emptyList(), null);
81+
public static ConfiguratorException from(
82+
CNode node, Configurator configurator, String path, String message, Throwable cause) {
83+
Source source = node != null ? node.getSource() : null;
84+
if (source == null && cause instanceof ConfiguratorException) {
85+
source = ((ConfiguratorException) cause).getSource();
86+
}
87+
88+
return new ConfiguratorException(configurator, message, null, Collections.emptyList(), cause, source, path);
7489
}
7590

7691
public ConfiguratorException(@CheckForNull String message) {
7792
this(null, message, null, Collections.emptyList(), null);
7893
}
7994

80-
public ConfiguratorException(@CheckForNull Throwable cause) {
81-
this(null, null, null, Collections.emptyList(), cause);
95+
public ConfiguratorException(@CheckForNull String message, @CheckForNull Throwable cause) {
96+
this(null, message, null, Collections.emptyList(), cause, null, null);
97+
}
98+
99+
public ConfiguratorException(@CheckForNull Configurator configurator, @CheckForNull String message) {
100+
this(configurator, message, null, Collections.emptyList(), null, null, null);
101+
}
102+
103+
public ConfiguratorException(
104+
@CheckForNull Configurator configurator, @CheckForNull String message, @CheckForNull Throwable cause) {
105+
this(configurator, message, null, Collections.emptyList(), cause, null, null);
106+
}
107+
108+
public ConfiguratorException(
109+
@CheckForNull Configurator configurator,
110+
@CheckForNull String message,
111+
String invalidAttribute,
112+
List<String> validAttributes,
113+
@CheckForNull Throwable cause) {
114+
this(configurator, message, invalidAttribute, validAttributes, cause, null, null);
82115
}
83116

84117
@CheckForNull
@@ -90,22 +123,48 @@ public List<String> getValidAttributes() {
90123
return validAttributes;
91124
}
92125

126+
@CheckForNull
93127
public String getInvalidAttribute() {
94128
return invalidAttribute;
95129
}
96130

131+
@CheckForNull
132+
public Source getSource() {
133+
return source;
134+
}
135+
136+
@CheckForNull
137+
public String getPath() {
138+
return path;
139+
}
140+
97141
public String getErrorMessage() {
98142
return super.getMessage();
99143
}
100144

101145
@Override
102146
public String getMessage() {
103-
if (configurator != null) {
104-
return String.format(
105-
"%s: %s",
106-
Jenkins.getInstanceOrNull() == null ? configurator.getClass() : configurator.getName(),
107-
super.getMessage());
147+
final String base = (configurator != null)
148+
? String.format(
149+
"%s: %s",
150+
Jenkins.getInstanceOrNull() == null ? configurator.getClass() : configurator.getName(),
151+
super.getMessage())
152+
: super.getMessage();
153+
154+
if (source == null && path == null) {
155+
return base;
108156
}
109-
return super.getMessage();
157+
158+
StringBuilder sb = new StringBuilder(base);
159+
sb.append(System.lineSeparator()).append(" at ");
160+
if (source != null) {
161+
sb.append(source);
162+
} else {
163+
sb.append("unknown source");
164+
}
165+
if (path != null) {
166+
sb.append(" (path: ").append(path).append(")");
167+
}
168+
return sb.toString();
110169
}
111170
}

‎plugin/src/main/java/io/jenkins/plugins/casc/cli/CheckConfigurationCommand.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ protected int run() throws Exception {
3131

3232
final Map<Source, String> issues = ConfigurationAsCode.get().checkWith(YamlSource.of(stdin));
3333
for (Map.Entry<Source, String> entry : issues.entrySet()) {
34-
stderr.printf("warning: line %d %s", entry.getKey().line, entry.getValue());
34+
stderr.printf("warning: line %d %s", entry.getKey().line(), entry.getValue());
3535
}
3636
return 0;
3737
}

‎plugin/src/main/java/io/jenkins/plugins/casc/model/CNode.java‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,15 +18,15 @@ enum Type {
1818
Type getType();
1919

2020
default Mapping asMapping() throws ConfiguratorException {
21-
throw new ConfiguratorException("Item isn't a Mapping");
21+
throw new ConfiguratorException("Item isn't a Mapping" + getSourceLocation());
2222
}
2323

2424
default Sequence asSequence() throws ConfiguratorException {
25-
throw new ConfiguratorException("Item isn't a Sequence");
25+
throw new ConfiguratorException("Item isn't a Sequence" + getSourceLocation());
2626
}
2727

2828
default Scalar asScalar() throws ConfiguratorException {
29-
throw new ConfiguratorException("Item isn't a Scalar");
29+
throw new ConfiguratorException("Item isn't a Scalar" + getSourceLocation());
3030
}
3131

3232
/** @deprecated sensitive data are identified based on target attribute being a ${@link hudson.util.Secret} */
@@ -49,5 +49,10 @@ default boolean isPrintableWhenEmpty() {
4949
*/
5050
Source getSource();
5151

52+
default String getSourceLocation() {
53+
Source s = getSource();
54+
return s != null ? " " + s : "";
55+
}
56+
5257
CNode clone();
5358
}

‎plugin/src/main/java/io/jenkins/plugins/casc/model/Mapping.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,8 +68,9 @@ public Source getSource() {
6868

6969
@Override
7070
public Mapping clone() {
71-
final Mapping clone = new Mapping();
72-
forEach((key, value) -> {
71+
Mapping clone = (Mapping) super.clone();
72+
clone.clear();
73+
this.forEach((key, value) -> {
7374
if (value != null) {
7475
clone.put(key, value.clone());
7576
}

‎plugin/src/main/java/io/jenkins/plugins/casc/model/Scalar.java‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,8 @@ public final class Scalar implements CNode, CharSequence {
1010

1111
private static final String SECRET_VALUE_STRING = "****";
1212

13-
private String value;
14-
private Format format;
13+
private final String value;
14+
private final Format format;
1515
private boolean raw;
1616
private Source source;
1717
private boolean sensitive;
@@ -174,8 +174,12 @@ public void setPrintableWhenEmpty(boolean print) {
174174
}
175175

176176
@Override
177-
public CNode clone() {
178-
return new Scalar(this);
177+
public Scalar clone() {
178+
try {
179+
return (Scalar) super.clone();
180+
} catch (CloneNotSupportedException e) {
181+
throw new AssertionError(e);
182+
}
179183
}
180184

181185
private Scalar(Scalar it) {

‎plugin/src/main/java/io/jenkins/plugins/casc/model/Sequence.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,8 @@ public Source getSource() {
3737

3838
@Override
3939
public Sequence clone() {
40-
final Sequence clone = new Sequence();
40+
Sequence clone = (Sequence) super.clone();
41+
clone.clear();
4142
stream().map(CNode::clone).forEach(clone::add);
4243
return clone;
4344
}

‎plugin/src/main/java/io/jenkins/plugins/casc/model/Source.java‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,20 @@
11
package io.jenkins.plugins.casc.model;
22

3+
import edu.umd.cs.findbugs.annotations.NonNull;
4+
import java.io.Serial;
35
import java.io.Serializable;
46

57
/**
68
* @author <a href="mailto:[email protected]">Nicolas De Loof</a>
79
*/
8-
public class Source implements Serializable {
10+
public record Source(String file, int line) implements Serializable {
911

10-
static final long serialVersionUID = 1L;
12+
@Serial
13+
private static final long serialVersionUID = 1L;
1114

12-
public final String file;
13-
public final int line;
14-
15-
public Source(String file, int line) {
16-
this.file = file;
17-
this.line = line;
15+
@NonNull
16+
@Override
17+
public String toString() {
18+
return (file != null ? file + ":" : "line ") + line;
1819
}
1920
}

‎plugin/src/test/java/io/jenkins/plugins/casc/ConfigurationErrorTest.java‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
package io.jenkins.plugins.casc;
2+
3+
import static org.hamcrest.MatcherAssert.assertThat;
4+
import static org.hamcrest.Matchers.containsString;
5+
import static org.hamcrest.Matchers.equalTo;
6+
import static org.hamcrest.Matchers.notNullValue;
7+
import static org.junit.jupiter.api.Assertions.assertThrows;
8+
9+
import io.jenkins.plugins.casc.yaml.YamlSource;
10+
import java.io.File;
11+
import java.nio.file.Files;
12+
import java.nio.file.Path;
13+
import org.junit.jupiter.api.Test;
14+
import org.junit.jupiter.api.extension.ExtendWith;
15+
import org.junit.jupiter.api.io.TempDir;
16+
import org.jvnet.hudson.test.JenkinsRule;
17+
import org.jvnet.hudson.test.junit.jupiter.JenkinsExtension;
18+
19+
@ExtendWith(JenkinsExtension.class)
20+
class ConfigurationErrorTest {
21+
22+
@TempDir
23+
Path tempDir;
24+
25+
@Test
26+
@SuppressWarnings("unused")
27+
void shouldReportLineNumberAndAttributeForTypeMismatch(JenkinsRule j) throws Exception {
28+
String yaml = "jenkins:\n" + " systemMessage: [\"Wrong Type\"]";
29+
30+
File configFile = tempDir.resolve("bad-config.yaml").toFile();
31+
Files.writeString(configFile.toPath(), yaml);
32+
33+
YamlSource<String> source = YamlSource.of(configFile.toURI().toString());
34+
35+
ConfiguratorException ex = assertThrows(
36+
ConfiguratorException.class, () -> ConfigurationAsCode.get().configureWith(source));
37+
38+
assertThat(ex.getPath(), equalTo("systemMessage"));
39+
assertThat(ex.getSource(), notNullValue());
40+
assertThat(ex.getSource().toString(), containsString("2"));
41+
}
42+
}

0 commit comments

Comments
 (0)