diff --git a/src/main/java/net/logstash/logback/marker/LogstashBasicMarker.java b/src/main/java/net/logstash/logback/marker/LogstashBasicMarker.java index 5575c089..72324d6b 100755 --- a/src/main/java/net/logstash/logback/marker/LogstashBasicMarker.java +++ b/src/main/java/net/logstash/logback/marker/LogstashBasicMarker.java @@ -18,11 +18,11 @@ import java.util.Collections; import java.util.Iterator; import java.util.List; -import java.util.Vector; +import java.util.concurrent.CopyOnWriteArrayList; import org.slf4j.Marker; -/* Copy of {@link org.slf4j.helpers.BasicMarker} from slf4j-api v1.7.12, with minor changes: +/* Copy of {@link org.slf4j.helpers.BasicMarker} from slf4j-api v1.7.31, with minor changes: * 1. make the constructor public so that it can be extended in other packages * 2. add getReferences() method @@ -62,9 +62,13 @@ @SuppressWarnings("serial") public class LogstashBasicMarker implements Marker { + private static final String OPEN = "[ "; + private static final String CLOSE = " ]"; + private static final String SEP = ", "; + private final String name; - private List refereceList; - + private final List referenceList = new CopyOnWriteArrayList<>(); + /* * BEGIN Modification in logstash-logback-encoder to make this constructor public */ @@ -78,77 +82,81 @@ public LogstashBasicMarker(String name) { this.name = name; } + /** + * {@inheritDoc} + */ + @Override public String getName() { return name; } - public synchronized void add(Marker reference) { + /** + * {@inheritDoc} + */ + @Override + public void add(Marker reference) { if (reference == null) { - throw new IllegalArgumentException( - "A null value cannot be added to a Marker as reference."); + throw new IllegalArgumentException("A null value cannot be added to a Marker as reference."); } // no point in adding the reference multiple times if (this.contains(reference)) { return; - - } else if (reference.contains(this)) { // avoid recursion - // a potential reference should not its future "parent" as a reference + } + if (reference.contains(this)) { // avoid recursion, a potential reference should not hold its future "parent" as a reference return; - } else { - // let's add the reference - if (refereceList == null) { - refereceList = new Vector(); - } - refereceList.add(reference); } - + + referenceList.add(reference); } - public synchronized boolean hasReferences() { - return ((refereceList != null) && (refereceList.size() > 0)); + /** + * {@inheritDoc} + */ + @Override + public boolean hasReferences() { + return !referenceList.isEmpty(); } + /** + * {@inheritDoc} + */ + @Deprecated + @Override public boolean hasChildren() { return hasReferences(); } - public synchronized Iterator iterator() { - if (refereceList != null) { - return refereceList.iterator(); - } else { - return Collections.emptyIterator(); - } + /** + * {@inheritDoc} + */ + @Override + public Iterator iterator() { + return referenceList.iterator(); } /* * BEGIN Modification in logstash-logback-encoder to add this method */ protected List getReferences() { - return refereceList == null - ? Collections.emptyList() - : Collections.unmodifiableList(refereceList); + return Collections.unmodifiableList(referenceList); } /* * END Modification in logstash-logback-encoder to add this method */ - public synchronized boolean remove(Marker referenceToRemove) { - if (refereceList == null) { - return false; - } - - int size = refereceList.size(); - for (int i = 0; i < size; i++) { - Marker m = (Marker) refereceList.get(i); - if (referenceToRemove.equals(m)) { - refereceList.remove(i); - return true; - } - } - return false; + /** + * {@inheritDoc} + */ + @Override + public boolean remove(Marker referenceToRemove) { + return referenceList.remove(referenceToRemove); } + /** + * {@inheritDoc} + */ + @Override public boolean contains(Marker other) { if (other == null) { throw new IllegalArgumentException("Other cannot be null"); @@ -159,19 +167,20 @@ public boolean contains(Marker other) { } if (hasReferences()) { - for (int i = 0; i < refereceList.size(); i++) { - Marker ref = (Marker) refereceList.get(i); + for (Marker ref : referenceList) { if (ref.contains(other)) { return true; } } } + return false; } /** - * This method is mainly used with Expression Evaluators. + * {@inheritDoc} */ + @Override public boolean contains(String name) { if (name == null) { throw new IllegalArgumentException("Other cannot be null"); @@ -182,21 +191,21 @@ public boolean contains(String name) { } if (hasReferences()) { - for (int i = 0; i < refereceList.size(); i++) { - Marker ref = (Marker) refereceList.get(i); + for (Marker ref : referenceList) { if (ref.contains(name)) { return true; } } } + return false; } - private static String OPEN = "[ "; - private static String CLOSE = " ]"; - private static String SEP = ", "; - + /** + * {@inheritDoc} + */ + @Override public boolean equals(Object obj) { if (this == obj) { return true; @@ -212,6 +221,10 @@ public boolean equals(Object obj) { return name.equals(other.getName()); } + /** + * {@inheritDoc} + */ + @Override public int hashCode() { return name.hashCode(); } @@ -220,13 +233,12 @@ public String toString() { if (!this.hasReferences()) { return this.getName(); } + StringBuilder sb = new StringBuilder(this.getName()) + .append(' ') + .append(OPEN); Iterator it = this.iterator(); - Marker reference; - StringBuffer sb = new StringBuffer(this.getName()); - sb.append(' ').append(OPEN); while (it.hasNext()) { - reference = (Marker) it.next(); - sb.append(reference.getName()); + sb.append(it.next().getName()); if (it.hasNext()) { sb.append(SEP); } @@ -236,4 +248,3 @@ public String toString() { return sb.toString(); } } - diff --git a/src/main/java/net/logstash/logback/marker/LogstashMarker.java b/src/main/java/net/logstash/logback/marker/LogstashMarker.java index 69e14da5..d93b1dc2 100755 --- a/src/main/java/net/logstash/logback/marker/LogstashMarker.java +++ b/src/main/java/net/logstash/logback/marker/LogstashMarker.java @@ -16,7 +16,6 @@ package net.logstash.logback.marker; import java.io.IOException; -import java.util.Objects; import com.fasterxml.jackson.core.JsonGenerator; import org.slf4j.Marker; @@ -79,7 +78,7 @@ public T with(Marker reference) { public abstract void writeTo(JsonGenerator generator) throws IOException; @Override - public synchronized void add(Marker reference) { + public void add(Marker reference) { if (reference instanceof EmptyLogstashMarker) { for (Marker m : (EmptyLogstashMarker) reference) { add(m); @@ -89,31 +88,6 @@ public synchronized void add(Marker reference) { } } - @Override - public int hashCode() { - final int prime = 31; - int result = 1; - result = prime * result + super.hashCode(); - result = prime * result + this.getReferences().hashCode(); - return result; - } - - @Override - public boolean equals(Object obj) { - if (this == obj) { - return true; - } - if (!super.equals(obj)) { - return false; - } - if (!(obj instanceof LogstashMarker)) { - return false; - } - - LogstashMarker other = (LogstashMarker) obj; - return Objects.equals(this.getReferences(), other.getReferences()); - } - /** * Returns a String in the form of * 
@@ -142,8 +116,10 @@ public String toString() {
                 sb.append(", ");
             }
             String referenceToString = marker.toString();
-            sb.append(referenceToString);
-            appendSeparator = !referenceToString.isEmpty();
+            if (!referenceToString.isEmpty()) {
+                sb.append(referenceToString);
+                appendSeparator = true;
+            }
         }
 
         return sb.toString();
@@ -160,6 +136,4 @@ public String toString() {
     protected String toStringSelf() {
         return getName();
     }
-
-
 }
diff --git a/src/test/java/net/logstash/logback/marker/MarkersTest.java b/src/test/java/net/logstash/logback/marker/MarkersTest.java
index 1728e112..d3bf481d 100644
--- a/src/test/java/net/logstash/logback/marker/MarkersTest.java
+++ b/src/test/java/net/logstash/logback/marker/MarkersTest.java
@@ -16,6 +16,7 @@
 package net.logstash.logback.marker;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatObject;
 
 import java.util.LinkedHashMap;
 import java.util.Map;
@@ -101,6 +102,47 @@ public void testToString() {
         map.put("fieldName2", "fieldValue2");
         assertThat(Markers.appendEntries(map).toString())
                 .isEqualTo("{fieldName1=fieldValue1, fieldName2=fieldValue2}");
-
+    }
+    
+    
+    /*
+     * LogstashMarkers are equals when same name, irrespective of their references
+     */
+    @Test
+    public void testEqualsAndHashCode() {
+        assertThat(Markers.empty()).isEqualTo(Markers.empty());
+        assertThat(Markers.empty()).hasSameHashCodeAs(Markers.empty());
+        
+        assertThatObject(
+                Markers.empty().and(Markers.empty()))
+            .isEqualTo(
+                Markers.empty());
+        
+        assertThatObject(
+                Markers.empty().and(Markers.empty()))
+            .hasSameHashCodeAs(
+                Markers.empty());
+        
+        
+        assertThat(
+                Markers.aggregate(MarkerFactory.getMarker("m1"), MarkerFactory.getMarker("m2")))
+            .isEqualTo(
+                Markers.aggregate(MarkerFactory.getMarker("m1"), MarkerFactory.getMarker("m2")));
+        
+        assertThat(
+                Markers.aggregate(MarkerFactory.getMarker("m1"), MarkerFactory.getMarker("m2")))
+            .hasSameHashCodeAs(
+                    Markers.aggregate(MarkerFactory.getMarker("m1"), MarkerFactory.getMarker("m2")));
+        
+        
+        assertThat(
+                Markers.aggregate(MarkerFactory.getMarker("m1"), MarkerFactory.getMarker("m2")))
+            .isEqualTo(
+                Markers.aggregate(MarkerFactory.getMarker("m2"), MarkerFactory.getMarker("m1")));
+
+        assertThat(
+                Markers.aggregate(MarkerFactory.getMarker("m1"), MarkerFactory.getMarker("m2")))
+            .hasSameHashCodeAs(
+                Markers.aggregate(MarkerFactory.getMarker("m2"), MarkerFactory.getMarker("m1")));
     }
 }