Skip to content

Commit

Permalink
[WFCORE-2958] Add an addtional runtime step to remove formatters from…
Browse files Browse the repository at this point in the history 
… a handler. In the case of a composite operation the formatter may be changed in a undefine-attribute operation followed by a removal of the formatter in a write-attribute handler for the named-formatter attribute. This causes the commit of the log managers configuration as the handler no longer exists after the write-attribute.
  • Loading branch information
@jamezp
jamezp committed Jun 16, 2017
1 parent 9a0e145 commit d4730e6
Show file tree
Hide file tree
Showing 3 changed files with 102 additions and 9 deletions.
31 changes: 24 additions & 7 deletions logging/src/main/java/org/jboss/as/logging/HandlerOperations.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,7 @@ protected HandlerConfiguration createHandlerConfiguration(final String className
*/
static class LogHandlerWriteAttributeHandler extends LoggingOperations.LoggingWriteAttributeHandler {
private final PropertySorter propertySorter;
private final Collection<OperationStepHandler> afterCommitRuntimeSteps;

protected LogHandlerWriteAttributeHandler(final AttributeDefinition... attributes) {
this(PropertySorter.NO_OP, attributes);
Expand All @@ -346,6 +347,7 @@ protected LogHandlerWriteAttributeHandler(final AttributeDefinition... attribute
protected LogHandlerWriteAttributeHandler(final PropertySorter propertySorter, final AttributeDefinition... attributes) {
super(attributes);
this.propertySorter = propertySorter;
this.afterCommitRuntimeSteps = new ArrayList<>();
}

@Override
Expand Down Expand Up @@ -400,7 +402,7 @@ protected boolean applyUpdate(final OperationContext context, final String attri
} else {
for (AttributeDefinition attribute : getAttributes()) {
if (attribute.getName().equals(attributeName)) {
handleProperty(attribute, context, value, logContextConfiguration, configuration, false);
afterCommitRuntimeSteps.addAll(handleProperty(attribute, context, value, logContextConfiguration, configuration, false));
restartRequired = Logging.requiresReload(attribute.getFlags());
break;
}
Expand Down Expand Up @@ -441,6 +443,11 @@ protected void finishModelStage(final OperationContext context, final ModelNode
}
}
}

@Override
Collection<OperationStepHandler> afterCommitRuntimeSteps() {
return afterCommitRuntimeSteps;
}
}

/**
Expand Down Expand Up @@ -622,9 +629,10 @@ private static void handleProperty(final AttributeDefinition attribute, final Op
*
* @throws OperationFailedException if an error occurs
*/
private static void handleProperty(final AttributeDefinition attribute, final OperationContext context, final ModelNode model,
final LogContextConfiguration logContextConfiguration, final HandlerConfiguration configuration, final boolean resolveValue)
private static Collection<OperationStepHandler> handleProperty(final AttributeDefinition attribute, final OperationContext context, final ModelNode model,
final LogContextConfiguration logContextConfiguration, final HandlerConfiguration configuration, final boolean resolveValue)
throws OperationFailedException {
final Collection<OperationStepHandler> afterCommitSteps = new ArrayList<>();

if (attribute.getName().equals(ENABLED.getName())) {
final boolean value = ((resolveValue ? ENABLED.resolveModelAttribute(context, model).asBoolean() : model.asBoolean()));
Expand All @@ -644,9 +652,13 @@ private static void handleProperty(final AttributeDefinition attribute, final Op
final ModelNode valueNode = (resolveValue ? NAMED_FORMATTER.resolveModelAttribute(context, model) : model);
final String resolvedValue = (valueNode.isDefined() ? valueNode.asString() : null);
configuration.setFormatterName(resolvedValue);
// Check the current formatter name, if it's the same name as the handler, remove the old formatter
// Check the current formatter name, if it's the same name as the handler, remove the old formatter.
// This is done in additional step as there are issues with composite operations. For example if a
// composite operation undefines a formatter then adds a named-formatter on a handler the undefined
// may be attempted to be set on the handler. This will result in a failure because the named-formatter
// removes the previous handler.
if (!formatterName.equals(resolvedValue) && logContextConfiguration.getFormatterNames().contains(formatterName)) {
logContextConfiguration.removeFormatterConfiguration(formatterName);
afterCommitSteps.add((c, operation) -> logContextConfiguration.removeFormatterConfiguration(formatterName));
}
} else {
// Use a formatter only if a named-formatter is not defined or the named-formatter was explicitly undefined
Expand All @@ -667,9 +679,13 @@ private static void handleProperty(final AttributeDefinition attribute, final Op
if (valueNode.isDefined()) {
final String resolvedValue = valueNode.asString();
configuration.setFormatterName(resolvedValue);
// Check the current formatter name, if it's the same name as the handler, remove the old formatter
// Check the current formatter name, if it's the same name as the handler, remove the old formatter.
// This is done in additional step as there are issues with composite operations. For example if a
// composite operation undefines a formatter then adds a named-formatter on a handler the undefined
// may be attempted to be set on the handler. This will result in a failure because the named-formatter
// removes the previous handler.
if (!formatterName.equals(resolvedValue) && logContextConfiguration.getFormatterNames().contains(formatterName)) {
logContextConfiguration.removeFormatterConfiguration(formatterName);
afterCommitSteps.add((c, operation) -> logContextConfiguration.removeFormatterConfiguration(formatterName));
}
} else {
// If the current formatter name already equals the name defined in the configuration, there is no need to process
Expand Down Expand Up @@ -746,6 +762,7 @@ private static void handleProperty(final AttributeDefinition attribute, final Op
LoggingLogger.ROOT_LOGGER.invalidPropertyAttribute(attribute.getName());
}
}
return afterCommitSteps;
}

/**
Expand Down
41 changes: 40 additions & 1 deletion logging/src/main/java/org/jboss/as/logging/LoggingOperations.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@

import static org.jboss.as.controller.descriptions.ModelDescriptionConstants.OP_ADDR;

import java.util.Collection;
import java.util.Collections;

import org.jboss.as.controller.AbstractWriteAttributeHandler;
import org.jboss.as.controller.AttributeDefinition;
import org.jboss.as.controller.OperationContext;
Expand Down Expand Up @@ -78,7 +81,32 @@ public static PathAddress getAddress(final ModelNode operation) {
* @param configurationPersistence the configuration to commit
*/
static void addCommitStep(final OperationContext context, final ConfigurationPersistence configurationPersistence) {
addCommitStep(context, configurationPersistence, Collections.emptyList());
}

/**
* Adds a {@link Stage#RUNTIME runtime} step to the context that will commit or rollback any logging changes. Also
* if not a logging profile writes the {@code logging.properties} file.
* <p>
* If the {@code afterCommitRuntimeSteps} collections is not {@linkplain Collection#isEmpty() empty} an additional
* commit step will be added to execute after all the additional steps have been executed.
* </p>
*
* @param context the context to add the step to
* @param configurationPersistence the configuration to commit
* @param afterCommitRuntimeSteps a collection of additional handlers to add at the
* {@linkplain Stage#RUNTIME runtime stage} after the commit step
*/
static void addCommitStep(final OperationContext context, final ConfigurationPersistence configurationPersistence,
final Collection<OperationStepHandler> afterCommitRuntimeSteps) {
context.addStep(new CommitOperationStepHandler(configurationPersistence), Stage.RUNTIME);
for (OperationStepHandler step : afterCommitRuntimeSteps) {
context.addStep(step, Stage.RUNTIME);
}
if (!afterCommitRuntimeSteps.isEmpty()) {
context.addStep(new CommitOperationStepHandler(configurationPersistence), Stage.RUNTIME);
}

}

private static final class CommitOperationStepHandler implements OperationStepHandler {
Expand Down Expand Up @@ -316,7 +344,7 @@ protected final boolean applyUpdateToRuntime(final OperationContext context, fin
final LogContextConfiguration logContextConfiguration = configurationPersistence.getLogContextConfiguration();
handbackHolder.setHandback(configurationPersistence);
final boolean restartRequired = applyUpdate(context, attributeName, name, resolvedValue, logContextConfiguration);
addCommitStep(context, configurationPersistence);
addCommitStep(context, configurationPersistence, afterCommitRuntimeSteps());
return restartRequired;
}

Expand Down Expand Up @@ -361,5 +389,16 @@ protected void validateUpdatedModel(final OperationContext context, final Resour
public final AttributeDefinition[] getAttributes() {
return attributes;
}

/**
* Defines a collection of {@linkplain OperationStepHandler handlers} to be executed after the commit step.
* Note that if the collection is not empty an additional commit step will be added after all the additional
* steps are added.
*
* @return a collection of steps to add after the commit step or an empty collection
*/
Collection<OperationStepHandler> afterCommitRuntimeSteps() {
return Collections.emptyList();
}
}
}
39 changes: 38 additions & 1 deletion logging/src/test/java/org/jboss/as/logging/HandlerOperationsTestCase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@
import static org.jboss.as.controller.client.helpers.Operations.createAddOperation;
import static org.jboss.as.controller.client.helpers.Operations.createRemoveOperation;
import static org.jboss.as.subsystem.test.SubsystemOperations.OperationBuilder;
import static org.junit.Assert.*;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;

import java.io.File;
import java.io.IOException;
Expand All @@ -37,6 +39,8 @@

import org.apache.commons.io.FileUtils;
import org.jboss.as.controller.PathAddress;
import org.jboss.as.controller.client.Operation;
import org.jboss.as.controller.client.helpers.Operations;
import org.jboss.as.controller.client.helpers.Operations.CompositeOperationBuilder;
import org.jboss.as.controller.services.path.PathResourceDefinition;
import org.jboss.as.subsystem.test.KernelServices;
Expand Down Expand Up @@ -275,6 +279,39 @@ public void testFormatsNoColor() throws Exception {

}

/**
* Tests a composite operation of undefining a {@code formatter} attribute and defining a {@code named-formatter}
* attribute in a composite operation. These two specific attributes have strange behavior. If the
* {@code named-formatter} is defined it removes the formatter, named the same as the handler, which was created
* as part of the {@code undefine-attribute} operation of the {@code formatter} attribute.
*
* @throws Exception if an error occurs
*/
@Test
public void testCompositeOperations() throws Exception {
final KernelServices kernelServices = boot();

final ModelNode address = createFileHandlerAddress("FILE").toModelNode();
final String filename = "test-file.log";

// Add the handler
ModelNode addOp = OperationBuilder.createAddOperation(address)
.addAttribute(CommonAttributes.FILE, createFileValue("jboss.server.log.dir", filename))
.build();
executeOperation(kernelServices, addOp);
final ModelNode patternFormatterAddress = createPatternFormatterAddress("PATTERN").toModelNode();
addOp = SubsystemOperations.createAddOperation(patternFormatterAddress);
addOp.get(PatternFormatterResourceDefinition.PATTERN.getName()).set("%d{HH:mm:ss,SSS} %-5p [%c] %s%e%n");
executeOperation(kernelServices, addOp);

// Create a composite operation to undefine
final Operation op = CompositeOperationBuilder.create()
.addStep(Operations.createUndefineAttributeOperation(address, "formatter"))
.addStep(Operations.createWriteAttributeOperation(address, "named-formatter", "PATTERN"))
.build();
executeOperation(kernelServices, op.getOperation());
}

private void testAsyncHandler(final KernelServices kernelServices, final String profileName) throws Exception {
final ModelNode address = createAsyncHandlerAddress(profileName, "async").toModelNode();
final ModelNode subhandlers = new ModelNode().setEmptyList().add("CONSOLE");
Expand Down

0 comments on commit d4730e6

Please sign in to comment.