This is an automated email from the ASF dual-hosted git repository. btellier pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/james-project.git
commit 09dd546216307dc84ab5db8262d4c5ef47f1e1ff Author: Tran Tien Duc <[email protected]> AuthorDate: Tue Dec 17 10:06:52 2019 +0700 JAMES-3007 MetricContract enforce getCount() return non-negative value --- .../java/org/apache/james/metrics/api/Metric.java | 5 ++++ .../apache/james/metrics/api/MetricContract.java | 27 ++++++++++++++++------ .../james/metrics/api/MetricFactoryContract.java | 6 ++--- .../james/metrics/dropwizard/DropWizardMetric.java | 16 +++++++++++-- .../dropwizard/DropWizardMetricFactory.java | 2 +- .../metrics/dropwizard/DropWizardMetricTest.java | 13 ++++++++++- .../apache/james/metrics/logger/DefaultMetric.java | 18 ++++++++++++--- .../james/metrics/logger/DefaultMetricFactory.java | 2 +- .../james/metrics/logger/DefaultMetricTest.java | 13 ++++++++++- .../james/metrics/tests/RecordingMetric.java | 15 +++++++++--- .../james/metrics/tests/RecordingMetricTest.java | 18 +++++++++++++++ 11 files changed, 113 insertions(+), 22 deletions(-) diff --git a/metrics/metrics-api/src/main/java/org/apache/james/metrics/api/Metric.java b/metrics/metrics-api/src/main/java/org/apache/james/metrics/api/Metric.java index 1b2e879..8f35998 100644 --- a/metrics/metrics-api/src/main/java/org/apache/james/metrics/api/Metric.java +++ b/metrics/metrics-api/src/main/java/org/apache/james/metrics/api/Metric.java @@ -29,5 +29,10 @@ public interface Metric { void remove(int value); + /** + * @return A long guaranteed to be positive. + * + * Implementation might be doing strict validation (throwing on negative value) or lenient (log and sanitize to 0) + */ long getCount(); } diff --git a/metrics/metrics-api/src/test/java/org/apache/james/metrics/api/MetricContract.java b/metrics/metrics-api/src/test/java/org/apache/james/metrics/api/MetricContract.java index 4c6bf07..59a975d 100644 --- a/metrics/metrics-api/src/test/java/org/apache/james/metrics/api/MetricContract.java +++ b/metrics/metrics-api/src/test/java/org/apache/james/metrics/api/MetricContract.java @@ -47,20 +47,24 @@ public interface MetricContract { @Test default void decrementShouldDecreaseCounter() { + testee().add(2); + testee().decrement(); assertThat(testee().getCount()) - .isEqualTo(-1); + .isEqualTo(1); } @Test default void decrementShouldDecreaseCounterAfterMultipleCalls() { + testee().add(6); + testee().decrement(); testee().decrement(); testee().decrement(); assertThat(testee().getCount()) - .isEqualTo(-3); + .isEqualTo(3); } @Test @@ -73,10 +77,11 @@ public interface MetricContract { @Test default void addShouldDecreaseCounterWhenNegativeNumber() { + testee().add(10); testee().add(-9); assertThat(testee().getCount()) - .isEqualTo(-9); + .isEqualTo(1); } @Test @@ -90,10 +95,11 @@ public interface MetricContract { @Test default void removeShouldDecreaseCounter() { - testee().remove(10); + testee().add(10); + testee().remove(9); assertThat(testee().getCount()) - .isEqualTo(-10); + .isEqualTo(1); } @Test @@ -106,11 +112,11 @@ public interface MetricContract { @Test default void removeShouldKeepCounterBeTheSameWhenZero() { - testee().remove(888); + testee().add(888); testee().remove(0); assertThat(testee().getCount()) - .isEqualTo(-888); + .isEqualTo(888); } @Test @@ -118,4 +124,11 @@ public interface MetricContract { assertThat(testee().getCount()) .isEqualTo(0); } + + @Test + default void getCountShouldReturnValueWhenCounterIsPositive() { + testee().add(19); + assertThat(testee().getCount()) + .isEqualTo(19); + } } \ No newline at end of file diff --git a/metrics/metrics-api/src/test/java/org/apache/james/metrics/api/MetricFactoryContract.java b/metrics/metrics-api/src/test/java/org/apache/james/metrics/api/MetricFactoryContract.java index 5db31fa..1cbc06d 100644 --- a/metrics/metrics-api/src/test/java/org/apache/james/metrics/api/MetricFactoryContract.java +++ b/metrics/metrics-api/src/test/java/org/apache/james/metrics/api/MetricFactoryContract.java @@ -47,12 +47,12 @@ public interface MetricFactoryContract { Metric metric1 = testee().generate(NAME_1); Metric metric2 = testee().generate(NAME_2); - metric1.increment(); - metric2.decrement(); + metric1.add(1); + metric2.add(2); SoftAssertions.assertSoftly(softly -> { softly.assertThat(metric1.getCount()).isEqualTo(1); - softly.assertThat(metric2.getCount()).isEqualTo(-1); + softly.assertThat(metric2.getCount()).isEqualTo(2); }); } } \ No newline at end of file diff --git a/metrics/metrics-dropwizard/src/main/java/org/apache/james/metrics/dropwizard/DropWizardMetric.java b/metrics/metrics-dropwizard/src/main/java/org/apache/james/metrics/dropwizard/DropWizardMetric.java index 21a192d..58a0332 100644 --- a/metrics/metrics-dropwizard/src/main/java/org/apache/james/metrics/dropwizard/DropWizardMetric.java +++ b/metrics/metrics-dropwizard/src/main/java/org/apache/james/metrics/dropwizard/DropWizardMetric.java @@ -20,15 +20,21 @@ package org.apache.james.metrics.dropwizard; import org.apache.james.metrics.api.Metric; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import com.codahale.metrics.Counter; public class DropWizardMetric implements Metric { + private static final Logger LOGGER = LoggerFactory.getLogger(DropWizardMetric.class); + private final Counter counter; + private final String metricName; - public DropWizardMetric(Counter counter) { + public DropWizardMetric(Counter counter, String metricName) { this.counter = counter; + this.metricName = metricName; } @Override @@ -53,6 +59,12 @@ public class DropWizardMetric implements Metric { @Override public long getCount() { - return counter.getCount(); + long value = counter.getCount(); + if (value < 0) { + LOGGER.error("counter value({}) of the metric '{}' should not be a negative number", value, metricName); + return 0; + } + + return value; } } diff --git a/metrics/metrics-dropwizard/src/main/java/org/apache/james/metrics/dropwizard/DropWizardMetricFactory.java b/metrics/metrics-dropwizard/src/main/java/org/apache/james/metrics/dropwizard/DropWizardMetricFactory.java index 015c21d..2d040d4 100644 --- a/metrics/metrics-dropwizard/src/main/java/org/apache/james/metrics/dropwizard/DropWizardMetricFactory.java +++ b/metrics/metrics-dropwizard/src/main/java/org/apache/james/metrics/dropwizard/DropWizardMetricFactory.java @@ -45,7 +45,7 @@ public class DropWizardMetricFactory implements MetricFactory, Startable { @Override public Metric generate(String name) { - return new DropWizardMetric(metricRegistry.counter(name)); + return new DropWizardMetric(metricRegistry.counter(name), name); } @Override diff --git a/metrics/metrics-dropwizard/src/test/java/org/apache/james/metrics/dropwizard/DropWizardMetricTest.java b/metrics/metrics-dropwizard/src/test/java/org/apache/james/metrics/dropwizard/DropWizardMetricTest.java index 1183bc4..f7ab436 100644 --- a/metrics/metrics-dropwizard/src/test/java/org/apache/james/metrics/dropwizard/DropWizardMetricTest.java +++ b/metrics/metrics-dropwizard/src/test/java/org/apache/james/metrics/dropwizard/DropWizardMetricTest.java @@ -19,9 +19,12 @@ package org.apache.james.metrics.dropwizard; +import static org.assertj.core.api.Assertions.assertThat; + import org.apache.james.metrics.api.Metric; import org.apache.james.metrics.api.MetricContract; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; import com.codahale.metrics.MetricRegistry; @@ -34,11 +37,19 @@ class DropWizardMetricTest implements MetricContract { @BeforeEach void setUp() { MetricRegistry registry = new MetricRegistry(); - testee = new DropWizardMetric(registry.counter(METRIC_NAME)); + testee = new DropWizardMetric(registry.counter(METRIC_NAME), METRIC_NAME); } @Override public Metric testee() { return testee; } + + @Test + void getCountShouldReturnZeroWhenCounterIsNegative() { + testee().remove(9); + + assertThat(testee().getCount()) + .isEqualTo(0); + } } \ No newline at end of file diff --git a/metrics/metrics-logger/src/main/java/org/apache/james/metrics/logger/DefaultMetric.java b/metrics/metrics-logger/src/main/java/org/apache/james/metrics/logger/DefaultMetric.java index ed4d7d3..d9b11db 100644 --- a/metrics/metrics-logger/src/main/java/org/apache/james/metrics/logger/DefaultMetric.java +++ b/metrics/metrics-logger/src/main/java/org/apache/james/metrics/logger/DefaultMetric.java @@ -21,12 +21,18 @@ package org.apache.james.metrics.logger; import java.util.concurrent.atomic.AtomicInteger; import org.apache.james.metrics.api.Metric; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public class DefaultMetric implements Metric { - private AtomicInteger value; + private static final Logger LOGGER = LoggerFactory.getLogger(DefaultMetric.class); - public DefaultMetric() { + private final AtomicInteger value; + private final String metricName; + + public DefaultMetric(String metricName) { + this.metricName = metricName; value = new AtomicInteger(); } @@ -52,6 +58,12 @@ public class DefaultMetric implements Metric { @Override public long getCount() { - return value.get(); + long counter = value.longValue(); + if (counter < 0) { + LOGGER.error("counter value({}) of the metric '{}' should not be a negative number", value, metricName); + return 0; + } + + return counter; } } diff --git a/metrics/metrics-logger/src/main/java/org/apache/james/metrics/logger/DefaultMetricFactory.java b/metrics/metrics-logger/src/main/java/org/apache/james/metrics/logger/DefaultMetricFactory.java index b00e3f3..db64916 100644 --- a/metrics/metrics-logger/src/main/java/org/apache/james/metrics/logger/DefaultMetricFactory.java +++ b/metrics/metrics-logger/src/main/java/org/apache/james/metrics/logger/DefaultMetricFactory.java @@ -30,7 +30,7 @@ public class DefaultMetricFactory implements MetricFactory { @Override public Metric generate(String name) { - return new DefaultMetric(); + return new DefaultMetric(name); } @Override diff --git a/metrics/metrics-logger/src/test/java/org/apache/james/metrics/logger/DefaultMetricTest.java b/metrics/metrics-logger/src/test/java/org/apache/james/metrics/logger/DefaultMetricTest.java index 3ab0d6d..8e8ccd2 100644 --- a/metrics/metrics-logger/src/test/java/org/apache/james/metrics/logger/DefaultMetricTest.java +++ b/metrics/metrics-logger/src/test/java/org/apache/james/metrics/logger/DefaultMetricTest.java @@ -19,9 +19,12 @@ package org.apache.james.metrics.logger; +import static org.assertj.core.api.Assertions.assertThat; + import org.apache.james.metrics.api.Metric; import org.apache.james.metrics.api.MetricContract; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; class DefaultMetricTest implements MetricContract { @@ -29,11 +32,19 @@ class DefaultMetricTest implements MetricContract { @BeforeEach void setUp() { - testee = new DefaultMetric(); + testee = new DefaultMetric("metricName"); } @Override public Metric testee() { return testee; } + + @Test + void getCountShouldReturnZeroWhenCounterIsNegative() { + testee().remove(9); + + assertThat(testee().getCount()) + .isEqualTo(0); + } } \ No newline at end of file diff --git a/metrics/metrics-tests/src/main/java/org/apache/james/metrics/tests/RecordingMetric.java b/metrics/metrics-tests/src/main/java/org/apache/james/metrics/tests/RecordingMetric.java index e004e86..5c08c76 100644 --- a/metrics/metrics-tests/src/main/java/org/apache/james/metrics/tests/RecordingMetric.java +++ b/metrics/metrics-tests/src/main/java/org/apache/james/metrics/tests/RecordingMetric.java @@ -41,7 +41,7 @@ public class RecordingMetric implements Metric { @Override public void decrement() { - value.decrementAndGet(); + value.updateAndGet(currentValue -> subtractFrom(currentValue, 1)); } @Override @@ -51,11 +51,20 @@ public class RecordingMetric implements Metric { @Override public void remove(int i) { - value.addAndGet(-1 * i); + value.updateAndGet(currentValue -> subtractFrom(currentValue, i)); } @Override public long getCount() { - return value.get(); + return value.longValue(); + } + + private int subtractFrom(int currentValue, int minus) { + int result = currentValue - minus; + if (result < 0) { + throw new UnsupportedOperationException("metric counter is supposed to be a non-negative number," + + " thus this operation cannot be applied"); + } + return result; } } diff --git a/metrics/metrics-tests/src/test/java/org/apache/james/metrics/tests/RecordingMetricTest.java b/metrics/metrics-tests/src/test/java/org/apache/james/metrics/tests/RecordingMetricTest.java index 682fa31..636ea69 100644 --- a/metrics/metrics-tests/src/test/java/org/apache/james/metrics/tests/RecordingMetricTest.java +++ b/metrics/metrics-tests/src/test/java/org/apache/james/metrics/tests/RecordingMetricTest.java @@ -19,9 +19,12 @@ package org.apache.james.metrics.tests; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + import org.apache.james.metrics.api.Metric; import org.apache.james.metrics.api.MetricContract; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; class RecordingMetricTest implements MetricContract { @@ -36,4 +39,19 @@ class RecordingMetricTest implements MetricContract { public Metric testee() { return testee; } + + @Test + void decrementShouldThrowWhenCounterIsZero() { + assertThatThrownBy(() -> testee.decrement()) + .isInstanceOf(UnsupportedOperationException.class) + .hasMessage("metric counter is supposed to be a non-negative number, thus this operation cannot be applied"); + } + + @Test + void removeShouldThrowWhenCounterIsLessThanPassedParam() { + testee.add(10); + assertThatThrownBy(() -> testee.remove(11)) + .isInstanceOf(UnsupportedOperationException.class) + .hasMessage("metric counter is supposed to be a non-negative number, thus this operation cannot be applied"); + } } \ No newline at end of file --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
