Skip to content
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

Nicer error pages #2352

Merged
merged 7 commits into from
Nov 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -377,14 +377,16 @@ protected void configure(Mapping config, T instance, boolean dryrun, Configurati
protected final void handleUnknown(Mapping config, ConfigurationContext context) throws ConfiguratorException {
if (!config.isEmpty()) {
final String invalid = StringUtils.join(config.keySet(), ',');
final String message = "Invalid configuration elements for type " + getTarget() + " : " + invalid + ".\n"
List<String> validAttributes =
getAttributes().stream().map(Attribute::getName).collect(Collectors.toList());
String baseErrorMessage = "Invalid configuration elements for type: ";
final String message = baseErrorMessage + getTarget() + " : " + invalid + ".\n"
+ "Available attributes : "
+ StringUtils.join(
getAttributes().stream().map(Attribute::getName).collect(Collectors.toList()), ", ");
+ StringUtils.join(validAttributes, ", ");
context.warning(config, message);
switch (context.getUnknown()) {
case reject:
throw new ConfiguratorException(message);
throw new UnknownAttributesException(this, baseErrorMessage, message, invalid, validAttributes);

case warn:
LOGGER.warning(message);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
import java.util.stream.Stream;
import javax.inject.Inject;
import javax.servlet.ServletContext;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServletResponse;
import jenkins.model.GlobalConfiguration;
import jenkins.model.Jenkins;
Expand Down Expand Up @@ -169,10 +170,42 @@ public void doReload(StaplerRequest request, StaplerResponse response) throws Ex
response.sendError(HttpServletResponse.SC_FORBIDDEN);
return;
}
configure();

try {
configure();
} catch (ConfiguratorException e) {
LOGGER.log(Level.SEVERE, "Failed to reload configuration", e);

Throwable throwableCause = e.getCause();
if (throwableCause instanceof ConfiguratorException) {
ConfiguratorException cause = (ConfiguratorException) throwableCause;

handleExceptionOnReloading(request, response, cause);
} else {
handleExceptionOnReloading(request, response, e);
}
return;
}
response.sendRedirect("");
}

@Restricted(NoExternalUse.class)
public static void handleExceptionOnReloading(
StaplerRequest request, StaplerResponse response, ConfiguratorException cause)
throws ServletException, IOException {
Configurator<?> configurator = cause.getConfigurator();
request.setAttribute("errorMessage", cause.getErrorMessage());
if (configurator != null) {
request.setAttribute("target", configurator.getName());
} else if (cause instanceof UnknownConfiguratorException) {
List<String> configuratorNames = ((UnknownConfiguratorException) cause).getConfiguratorNames();
request.setAttribute("target", String.join(", ", configuratorNames));
}
request.setAttribute("invalidAttribute", cause.getInvalidAttribute());
request.setAttribute("validAttributes", cause.getValidAttributes());
request.getView(ConfigurationAsCode.class, "error.jelly").forward(request, response);
Copy link
Member

Choose a reason for hiding this comment

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

FYI I am getting

java.lang.NullPointerException: Cannot invoke "javax.servlet.RequestDispatcher.forward(javax.servlet.ServletRequest, javax.servlet.ServletResponse)" because the return value of "org.kohsuke.stapler.StaplerRequest.getView(java.lang.Class, String)" is null
	at io.jenkins.plugins.casc.ConfigurationAsCode.handleExceptionOnReloading(ConfigurationAsCode.java:207)
	at io.jenkins.plugins.casc.ConfigurationAsCodeBootFailure.doDynamic(ConfigurationAsCodeBootFailure.java:17)

in CloudBees CI after a CasC error during startup. Not sure yet if this is reproducible.

}

@RequirePOST
@Restricted(NoExternalUse.class)
public void doReplace(StaplerRequest request, StaplerResponse response) throws Exception {
Expand Down Expand Up @@ -303,7 +336,11 @@ private List<YamlSource> getConfigFromSources(List<String> newSources) throws Co
@Initializer(after = InitMilestone.SYSTEM_CONFIG_LOADED, before = InitMilestone.SYSTEM_CONFIG_ADAPTED)
public static void init() throws Exception {
detectVaultPluginMissing();
get().configure();
try {
get().configure();
} catch (ConfiguratorException e) {
throw new ConfigurationAsCodeBootFailure(e);
Copy link
Member Author

Choose a reason for hiding this comment

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

This requires jenkinsci/jenkins#8442 to work properly although it won't do any harm if not present

}
}

/**
Expand Down Expand Up @@ -727,18 +764,9 @@ private static void invokeWith(Mapping entries, ConfiguratorOperation function)
if (!entry.getKey().equalsIgnoreCase(configurator.getName())) {
continue;
}
try {
function.apply(configurator, entry.getValue());
it.remove();
break;
} catch (ConfiguratorException e) {
throw new ConfiguratorException(
configurator,
format(
"error configuring '%s' with %s configurator",
entry.getKey(), configurator.getClass()),
e);
}
function.apply(configurator, entry.getValue());
it.remove();
break;
}
}

Expand All @@ -752,8 +780,7 @@ private static void invokeWith(Mapping entries, ConfiguratorOperation function)
});

if (!unknownKeys.isEmpty()) {
throw new ConfiguratorException(
format("No configurator for the following root elements %s", String.join(", ", unknownKeys)));
throw new UnknownConfiguratorException(unknownKeys, "No configurator for the following root elements:");
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package io.jenkins.plugins.casc;

import hudson.util.BootFailure;
import java.io.IOException;
import javax.servlet.ServletException;
import org.kohsuke.stapler.StaplerRequest;
import org.kohsuke.stapler.StaplerResponse;

public class ConfigurationAsCodeBootFailure extends BootFailure {

public ConfigurationAsCodeBootFailure(ConfiguratorException cause) {
super(cause);
}

public void doDynamic(StaplerRequest req, StaplerResponse rsp) throws IOException, ServletException {
rsp.setStatus(503);
ConfigurationAsCode.handleExceptionOnReloading(req, rsp, (ConfiguratorException) getCause());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@

import edu.umd.cs.findbugs.annotations.CheckForNull;
import io.jenkins.plugins.casc.model.CNode;
import java.io.IOException;
import java.util.Collections;
import java.util.List;

/**
* Exception type for {@link Configurator} issues.
Expand All @@ -32,38 +33,70 @@
* @see Configurator#configure(CNode, ConfigurationContext)
* @see Configurator
*/
public class ConfiguratorException extends IOException {
public class ConfiguratorException extends RuntimeException {

@CheckForNull
private final Configurator configurator;

private final List<String> validAttributes;

@CheckForNull
private final String invalidAttribute;

public ConfiguratorException(
@CheckForNull Configurator configurator,
@CheckForNull String message,
String invalidAttribute,
List<String> validAttributes,
@CheckForNull Throwable cause) {
super(message, cause);
this.configurator = configurator;

this.invalidAttribute = invalidAttribute;
this.validAttributes = validAttributes;
}

public ConfiguratorException(
@CheckForNull Configurator configurator, @CheckForNull String message, @CheckForNull Throwable cause) {
super(message, cause);
this.configurator = configurator;
this.invalidAttribute = null;
this.validAttributes = Collections.emptyList();
}

public ConfiguratorException(@CheckForNull String message, @CheckForNull Throwable cause) {
this(null, message, cause);
this(null, message, null, Collections.emptyList(), cause);
}

public ConfiguratorException(@CheckForNull Configurator configurator, @CheckForNull String message) {
this(configurator, message, null);
this(configurator, message, null, Collections.emptyList(), null);
}

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

public ConfiguratorException(@CheckForNull Throwable cause) {
this(null, null, cause);
this(null, null, null, Collections.emptyList(), cause);
}

@CheckForNull
public Configurator getConfigurator() {
return configurator;
}

public List<String> getValidAttributes() {
return validAttributes;
}

public String getInvalidAttribute() {
return invalidAttribute;
}

public String getErrorMessage() {
return super.getMessage();
}

@Override
public String getMessage() {
if (configurator != null) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package io.jenkins.plugins.casc;

import edu.umd.cs.findbugs.annotations.CheckForNull;
import java.util.List;

public class UnknownAttributesException extends ConfiguratorException {

private final String errorMessage;

public UnknownAttributesException(
@CheckForNull Configurator configurator,
String simpleMessage,
@CheckForNull String message,
String invalidAttribute,
List<String> validAttributes) {
super(configurator, message, invalidAttribute, validAttributes, null);
this.errorMessage = simpleMessage;
}

public String getErrorMessage() {
return errorMessage;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package io.jenkins.plugins.casc;

import java.util.List;

public class UnknownConfiguratorException extends ConfiguratorException {

private final List<String> configuratorNames;
private final String errorMessage;

public UnknownConfiguratorException(List<String> configuratorNames, String errorMessage) {
super(errorMessage + String.join(", ", configuratorNames));
this.errorMessage = errorMessage;
this.configuratorNames = configuratorNames;
}

public String getErrorMessage() {
return errorMessage;
}

public List<String> getConfiguratorNames() {
return configuratorNames;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import io.jenkins.plugins.casc.ConfigurationContext;
import io.jenkins.plugins.casc.Configurator;
import io.jenkins.plugins.casc.ObsoleteConfigurationMonitor;
import io.jenkins.plugins.casc.UnknownAttributesException;
import io.jenkins.plugins.casc.impl.attributes.DescribableAttribute;
import io.jenkins.plugins.casc.model.CNode;
import io.jenkins.plugins.casc.model.Mapping;
Expand All @@ -33,6 +34,7 @@
import java.util.function.Predicate;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.stream.Collectors;
import jenkins.model.Jenkins;
import org.jenkinsci.Symbol;
import org.kohsuke.accmod.Restricted;
Expand Down Expand Up @@ -214,16 +216,26 @@ private Class<T> descriptorClass(Descriptor<T> descriptor) {
}

private Option<Descriptor<T>> lookupDescriptor(String symbol, CNode config) {
return getDescriptors()
Stream<Descriptor<T>> descriptors = getDescriptors();
return descriptors
.filter(descriptor ->
findByPreferredSymbol(descriptor, symbol) || findBySymbols(descriptor, symbol, config))
.map(descriptor -> Tuple.of(preferredSymbol(descriptor), descriptor))
.foldLeft(HashMap.empty(), this::handleDuplicateSymbols)
.values()
.headOption()
.orElse(() -> {
throw new IllegalArgumentException(
"No " + target.getName() + " implementation found for " + symbol);
List<String> availableImplementations = descriptors
.toJavaStream()
.map(d -> DescribableAttribute.getPreferredSymbol(d, getImplementedAPI(), target))
.collect(Collectors.toList());

throw new UnknownAttributesException(
this,
"No implementation found for:",
"No " + target.getName() + " implementation found for " + symbol,
symbol,
availableImplementations);
});
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:l="/lib/layout">
<l:layout type="one-column" title="${%Error loading configuration}">
<l:main-panel>
<l:app-bar title="${%Error loading configuration}" />

<p>${errorMessage}<pre><code>${target}</code></pre></p>

<j:if test="${invalidAttribute != null}">
<p>Attribute was: <pre><code>${invalidAttribute}</code></pre></p>
</j:if>

<j:if test="${!empty(validAttributes)}">
<p>Valid attributes are:</p>

<ul>
<j:forEach var="attribute" items="${validAttributes}">
<li>${attribute}</li>
</j:forEach>
</ul>
</j:if>
</l:main-panel>
</l:layout>
</j:jelly>
Loading