LOG4J2-997 - Add filter and remove filter were not working properly in AbstractFilterable.
Project: http://git-wip-us.apache.org/repos/asf/logging-log4j2/repo Commit: http://git-wip-us.apache.org/repos/asf/logging-log4j2/commit/9acd705c Tree: http://git-wip-us.apache.org/repos/asf/logging-log4j2/tree/9acd705c Diff: http://git-wip-us.apache.org/repos/asf/logging-log4j2/diff/9acd705c Branch: refs/heads/LOG4J-1181 Commit: 9acd705caaced6c50de0907a5592ea351ba69b27 Parents: d3b5841 Author: Ralph Goers <rgo...@nextiva.com> Authored: Thu Jun 2 23:41:20 2016 -0700 Committer: Ralph Goers <rgo...@nextiva.com> Committed: Thu Jun 2 23:41:20 2016 -0700 ---------------------------------------------------------------------- .../log4j/core/filter/AbstractFilterable.java | 14 +- .../log4j/core/filter/CompositeFilter.java | 27 +- .../core/filter/AbstractFilterableTest.java | 281 +++++++++++++++++++ src/changes/changes.xml | 3 + 4 files changed, 316 insertions(+), 9 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/9acd705c/log4j-core/src/main/java/org/apache/logging/log4j/core/filter/AbstractFilterable.java ---------------------------------------------------------------------- diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/filter/AbstractFilterable.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/filter/AbstractFilterable.java index 859a5aa..4e77c14 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/filter/AbstractFilterable.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/filter/AbstractFilterable.java @@ -54,9 +54,12 @@ public abstract class AbstractFilterable extends AbstractLifeCycle implements Fi */ @Override public synchronized void addFilter(final Filter filter) { + if (filter == null) { + return; + } if (this.filter == null) { this.filter = filter; - } else if (filter instanceof CompositeFilter) { + } else if (this.filter instanceof CompositeFilter) { this.filter = ((CompositeFilter) this.filter).addFilter(filter); } else { final Filter[] filters = new Filter[] {this.filter, filter}; @@ -70,10 +73,13 @@ public abstract class AbstractFilterable extends AbstractLifeCycle implements Fi */ @Override public synchronized void removeFilter(final Filter filter) { - if (this.filter == filter) { + if (this.filter == null || filter == null) { + return; + } + if (this.filter == filter || this.filter.equals(filter)) { this.filter = null; - } else if (filter instanceof CompositeFilter) { - CompositeFilter composite = (CompositeFilter) filter; + } else if (this.filter instanceof CompositeFilter) { + CompositeFilter composite = (CompositeFilter) this.filter; composite = composite.removeFilter(filter); if (composite.size() > 1) { this.filter = composite; http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/9acd705c/log4j-core/src/main/java/org/apache/logging/log4j/core/filter/CompositeFilter.java ---------------------------------------------------------------------- diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/filter/CompositeFilter.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/filter/CompositeFilter.java index 9e5754a..3933fa0 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/filter/CompositeFilter.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/filter/CompositeFilter.java @@ -16,6 +16,7 @@ */ package org.apache.logging.log4j.core.filter; +import java.util.ArrayList; import java.util.Arrays; import java.util.Iterator; import java.util.List; @@ -55,9 +56,19 @@ public final class CompositeFilter extends AbstractLifeCycle implements Iterable // null does nothing return this; } - final Filter[] copy = Arrays.copyOf(this.filters, this.filters.length + 1); - copy[this.filters.length] = filter; - return new CompositeFilter(copy); + if (filter instanceof CompositeFilter) { + int size = this.filters.length + ((CompositeFilter) filter).size(); + final Filter[] copy = Arrays.copyOf(this.filters, size); + int index = this.filters.length; + for (Filter currentFilter : ((CompositeFilter) filter).filters) { + copy[index] = currentFilter; + } + return new CompositeFilter(copy); + } else { + final Filter[] copy = Arrays.copyOf(this.filters, this.filters.length + 1); + copy[this.filters.length] = filter; + return new CompositeFilter(copy); + } } public CompositeFilter removeFilter(final Filter filter) { @@ -68,8 +79,14 @@ public final class CompositeFilter extends AbstractLifeCycle implements Iterable // This is not a great implementation but simpler than copying Apache Commons // Lang ArrayUtils.removeElement() and associated bits (MutableInt), // which is OK since removing a filter should not be on the critical path. - final List<Filter> filterList = Arrays.asList(this.filters); - filterList.remove(filter); + final List<Filter> filterList = new ArrayList<>(Arrays.asList(this.filters)); + if (filter instanceof CompositeFilter) { + for (Filter currentFilter : ((CompositeFilter) filter).filters) { + filterList.remove(currentFilter); + } + } else { + filterList.remove(filter); + } return new CompositeFilter(filterList.toArray(new Filter[this.filters.length - 1])); } http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/9acd705c/log4j-core/src/test/java/org/apache/logging/log4j/core/filter/AbstractFilterableTest.java ---------------------------------------------------------------------- diff --git a/log4j-core/src/test/java/org/apache/logging/log4j/core/filter/AbstractFilterableTest.java b/log4j-core/src/test/java/org/apache/logging/log4j/core/filter/AbstractFilterableTest.java new file mode 100644 index 0000000..332b03c --- /dev/null +++ b/log4j-core/src/test/java/org/apache/logging/log4j/core/filter/AbstractFilterableTest.java @@ -0,0 +1,281 @@ +/* Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.logging.log4j.core.filter; + +import static org.junit.Assert.*; + +import org.apache.logging.log4j.Level; +import org.apache.logging.log4j.core.Filter; +import org.junit.Before; +import org.junit.Test; + +public class AbstractFilterableTest { + + MockedAbstractFilterable filterable; + + @Before + public void setup() { + filterable = new MockedAbstractFilterable(); + } + + @Test + public void testAddSimpleFilter() throws Exception { + Filter filter = ThresholdFilter.createFilter(Level.ERROR, null, null); + + filterable.addFilter(filter); + assertSame(filter, filterable.getFilter()); + } + + @Test + public void testAddMultipleSimpleFilters() throws Exception { + Filter filter = ThresholdFilter.createFilter(Level.ERROR, null, null); + + filterable.addFilter(filter); + assertSame(filter, filterable.getFilter()); + // adding a second filter converts the filter + // into a CompositeFilter.class + filterable.addFilter(filter); + assertTrue(filterable.getFilter() instanceof CompositeFilter); + assertEquals(2, ((CompositeFilter) filterable.getFilter()).getFilters().size()); + } + + @Test + public void testAddMultipleEqualSimpleFilter() throws Exception { + Filter filter = new EqualFilter("test"); + + filterable.addFilter(filter); + assertSame(filter, filterable.getFilter()); + // adding a second filter converts the filter + // into a CompositeFilter.class + filterable.addFilter(filter); + assertTrue(filterable.getFilter() instanceof CompositeFilter); + assertEquals(2, ((CompositeFilter) filterable.getFilter()).getFilters().size()); + } + + @Test + public void testAddCompositeFilter() throws Exception { + Filter filter1 = ThresholdFilter.createFilter(Level.ERROR, null, null); + Filter filter2 = ThresholdFilter.createFilter(Level.ERROR, null, null); + Filter compositeFilter = CompositeFilter.createFilters(new Filter[] {filter1, filter2}); + + filterable.addFilter(compositeFilter); + assertSame(compositeFilter, filterable.getFilter()); + } + + @Test + public void testAddMultipleCompositeFilters() throws Exception { + Filter filter1 = ThresholdFilter.createFilter(Level.ERROR, null, null); + Filter filter2 = ThresholdFilter.createFilter(Level.ERROR, null, null); + Filter filter3 = ThresholdFilter.createFilter(Level.ERROR, null, null); + Filter compositeFilter = CompositeFilter.createFilters(new Filter[]{filter1, filter2, filter3}); + + filterable.addFilter(compositeFilter); + assertSame(compositeFilter, filterable.getFilter()); + // adding a second filter converts the filter + // into a CompositeFilter.class + filterable.addFilter(compositeFilter); + assertTrue(filterable.getFilter() instanceof CompositeFilter); + assertEquals(6, ((CompositeFilter) filterable.getFilter()).getFilters().size()); + } + + @Test + public void testAddSimpleFilterAndCompositeFilter() throws Exception { + Filter filter1 = ThresholdFilter.createFilter(Level.ERROR, null, null); + Filter filter2 = ThresholdFilter.createFilter(Level.ERROR, null, null); + Filter notInCompositeFilterFilter = ThresholdFilter.createFilter(Level.ERROR, null, null); + Filter compositeFilter = CompositeFilter.createFilters(new Filter[]{filter1, filter2}); + + filterable.addFilter(notInCompositeFilterFilter); + assertSame(notInCompositeFilterFilter, filterable.getFilter()); + // adding a second filter converts the filter + // into a CompositeFilter.class + filterable.addFilter(compositeFilter); + assertTrue(filterable.getFilter() instanceof CompositeFilter); + assertEquals(2, ((CompositeFilter) filterable.getFilter()).getFilters().size()); + } + + @Test + public void testAddCompositeFilterAndSimpleFilter() throws Exception { + Filter filter1 = ThresholdFilter.createFilter(Level.ERROR, null, null); + Filter filter2 = ThresholdFilter.createFilter(Level.ERROR, null, null); + Filter notInCompositeFilterFilter = ThresholdFilter.createFilter(Level.ERROR, null, null); + Filter compositeFilter = CompositeFilter.createFilters(new Filter[]{filter1, filter2}); + + filterable.addFilter(compositeFilter); + assertSame(compositeFilter, filterable.getFilter()); + // adding a second filter converts the filter + // into a CompositeFilter.class + filterable.addFilter(notInCompositeFilterFilter); + assertTrue(filterable.getFilter() instanceof CompositeFilter); + assertEquals(3, ((CompositeFilter) filterable.getFilter()).getFilters().size()); + } + + @Test + public void testRemoveSimpleFilterFromSimpleFilter() throws Exception { + Filter filter = ThresholdFilter.createFilter(Level.ERROR, null, null); + + filterable.addFilter(filter); + filterable.removeFilter(filter); + assertNull(filterable.getFilter()); + } + + @Test + public void testRemoveSimpleEqualFilterFromSimpleFilter() throws Exception { + Filter filterOriginal = new EqualFilter("test"); + Filter filterCopy = new EqualFilter("test"); + + filterable.addFilter(filterOriginal); + filterable.removeFilter(filterCopy); + assertNull(filterable.getFilter()); + } + + @Test + public void testRemoveSimpleEqualFilterFromTwoSimpleFilters() throws Exception { + Filter filterOriginal = new EqualFilter("test"); + Filter filterCopy = new EqualFilter("test"); + + filterable.addFilter(filterOriginal); + filterable.addFilter(filterOriginal); + filterable.removeFilter(filterCopy); + assertSame(filterOriginal, filterable.getFilter()); + filterable.removeFilter(filterCopy); + assertNull(filterable.getFilter()); + } + + @Test + public void testRemoveSimpleEqualFilterFromMultipleSimpleFilters() throws Exception { + Filter filterOriginal = new EqualFilter("test"); + Filter filterCopy = new EqualFilter("test"); + + filterable.addFilter(filterOriginal); + filterable.addFilter(filterOriginal); + filterable.addFilter(filterCopy); + filterable.removeFilter(filterCopy); + assertTrue(filterable.getFilter() instanceof CompositeFilter); + assertEquals(2, ((CompositeFilter) filterable.getFilter()).getFilters().size()); + filterable.removeFilter(filterCopy); + assertEquals(filterOriginal, filterable.getFilter()); + filterable.removeFilter(filterOriginal); + assertNull(filterable.getFilter()); + } + + @Test + public void testRemoveNullFromSingleSimpleFilter() throws Exception { + Filter filter = ThresholdFilter.createFilter(Level.ERROR, null, null); + + filterable.addFilter(filter); + filterable.removeFilter(null); + assertSame(filter, filterable.getFilter()); + } + + + @Test + public void testRemoveNonExistingFilterFromSingleSimpleFilter() throws Exception { + Filter filter = ThresholdFilter.createFilter(Level.ERROR, null, null); + Filter newFilter = ThresholdFilter.createFilter(Level.WARN, null, null); + + filterable.addFilter(filter); + filterable.removeFilter(newFilter); + assertSame(filter, filterable.getFilter()); + } + + @Test + public void testRemoveSimpleFilterFromCompositeFilter() { + Filter filter1 = ThresholdFilter.createFilter(Level.ERROR, null, null); + Filter filter2 = ThresholdFilter.createFilter(Level.ERROR, null, null); + Filter compositeFilter = CompositeFilter.createFilters(new Filter[]{filter1, filter2}); + + filterable.addFilter(compositeFilter); + + // should remove internal filter of compositeFilter + filterable.removeFilter(filter1); + assertFalse(filterable.getFilter() instanceof CompositeFilter); + + assertEquals(filter2, filterable.getFilter()); + } + + @Test + public void testRemoveSimpleFilterFromCompositeAndSimpleFilter() { + Filter filter1 = ThresholdFilter.createFilter(Level.ERROR, null, null); + Filter filter2 = ThresholdFilter.createFilter(Level.ERROR, null, null); + Filter compositeFilter = CompositeFilter.createFilters(new Filter[]{filter1, filter2}); + Filter anotherFilter = ThresholdFilter.createFilter(Level.WARN, null, null); + + + filterable.addFilter(compositeFilter); + filterable.addFilter(anotherFilter); + + // should not remove internal filter of compositeFilter + filterable.removeFilter(anotherFilter); + assertTrue(filterable.getFilter() instanceof CompositeFilter); + assertEquals(2, ((CompositeFilter) filterable.getFilter()).getFilters().size()); + } + + @Test + public void testRemoveCompositeFilterFromCompositeFilter() { + Filter filter1 = ThresholdFilter.createFilter(Level.ERROR, null, null); + Filter filter2 = ThresholdFilter.createFilter(Level.ERROR, null, null); + Filter compositeFilter = CompositeFilter.createFilters(new Filter[]{filter1, filter2}); + + filterable.addFilter(compositeFilter); + filterable.removeFilter(compositeFilter); + assertNull(filterable.getFilter()); + } + + @Test + public void testRemoveFiltersFromComposite() { + Filter filter1 = ThresholdFilter.createFilter(Level.ERROR, null, null); + Filter filter2 = ThresholdFilter.createFilter(Level.ERROR, null, null); + Filter compositeFilter = CompositeFilter.createFilters(new Filter[]{filter1, filter2}); + Filter anotherFilter = ThresholdFilter.createFilter(Level.WARN, null, null); + + filterable.addFilter(compositeFilter); + filterable.addFilter(anotherFilter); + assertEquals(3, ((CompositeFilter) filterable.getFilter()).getFilters().size()); + filterable.removeFilter(filter1); + assertEquals(2, ((CompositeFilter) filterable.getFilter()).getFilters().size()); + filterable.removeFilter(filter2); + assertSame(anotherFilter, filterable.getFilter()); + } + + private class MockedAbstractFilterable extends AbstractFilterable {} + + private class EqualFilter extends AbstractFilter { + private final String key; + public EqualFilter(String key) { + this.key = key; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof EqualFilter)) return false; + + EqualFilter that = (EqualFilter) o; + + if (key != null ? !key.equals(that.key) : that.key != null) return false; + + return true; + } + + @Override + public int hashCode() { + return key != null ? key.hashCode() : 0; + } + } +} http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/9acd705c/src/changes/changes.xml ---------------------------------------------------------------------- diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 046e4a1..a0d1493 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -24,6 +24,9 @@ </properties> <body> <release version="2.6.1" date="2016-MM-DD" description="GA Release 2.6.1"> + <action issue="LOG4J2-997" dev="rgoers" type="fix" due-to="Maytee Chinavanichkit"> + Add filter and remove filter were not working properly in AbstractFilterable. + </action> <action issue="LOG4J2-1032" dev="rgoers" type="fix"> Change RenameAction to use java.nio to better report rename failures. </action>
