From c77afba757f88d116eb5a9ee5dff72ba1849a606 Mon Sep 17 00:00:00 2001 From: Peeter Karolin Date: Fri, 22 Sep 2023 15:00:32 +0300 Subject: [PATCH] APPENG-656: change twContext.put() to always update values regardless if the old and new values are equal. Change TwContextAttributeChangeListener to TwContextAttributePutListener as a result. --- CHANGELOG.md | 8 ++++ docs/index.md | 2 +- gradle.properties | 2 +- ...EntryPointOwnerAttributesPutListener.java} | 10 +++-- .../TwContextOwnershipAutoConfiguration.java | 8 ++-- .../context/ownership/OwnershipIntTest.java | 6 +-- .../common/context/TwContext.java | 40 +++++++++---------- .../TwContextAttributeChangeListener.java | 9 ----- .../TwContextAttributePutListener.java | 9 +++++ ...=> TwContextAttributePutListenerTest.java} | 14 +++---- 10 files changed, 59 insertions(+), 49 deletions(-) rename tw-context-ownership-starter/src/main/java/com/transferwise/common/context/ownership/{EntryPointOwnerAttributesChangeListener.java => EntryPointOwnerAttributesPutListener.java} (87%) delete mode 100644 tw-context/src/main/java/com/transferwise/common/context/TwContextAttributeChangeListener.java create mode 100644 tw-context/src/main/java/com/transferwise/common/context/TwContextAttributePutListener.java rename tw-context/src/test/java/com/transferwise/common/context/{TwContextAttributeChangeListenerTest.java => TwContextAttributePutListenerTest.java} (81%) diff --git a/CHANGELOG.md b/CHANGELOG.md index f7f47f6..2f25622 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,14 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [1.0.0] - 2023-09-22 + +### Changed +* When using `twContext.put()` it now always stores the new value to attributes. It no longer checks if old value is not equal to the new one. + This is needed to support use cases where the objects might be equal (like an empty map), but we want to create a new separate object anyway in the sub contexts. +* As a result also change the `TwContextAttributeChangeListener` to `TwContextAttributePutListener`. + `TwContextAttributePutListener` is now always called when the `twContext.put()` is called regardless if the value changed or not. This also enables wider use of the interface. + ## [0.12.2] - 2023-08-03 ### Bumped diff --git a/docs/index.md b/docs/index.md index f47a95e..15b2b58 100644 --- a/docs/index.md +++ b/docs/index.md @@ -12,7 +12,7 @@ Many applications have multiple owners - on endpoints, jobs and other units of w It is useful to correlate Rollbar errors, logs and even metrics by specific owners. -`com.transferwise.common.context.TwContextAttributeChangeListenerTest.ownerCanBeSetWhenNameIsChanged` describes how +`com.transferwise.common.context.TwContextAttributePutListenerTest.ownerCanBeSetWhenNameIsChanged` describes how to make the application set the owner field. But for services, to set an owner it is recommended to use `tw-context-ownership-starter` module instead. diff --git a/gradle.properties b/gradle.properties index a545267..beb72cc 100644 --- a/gradle.properties +++ b/gradle.properties @@ -1 +1 @@ -version=0.12.2 \ No newline at end of file +version=1.0.0 \ No newline at end of file diff --git a/tw-context-ownership-starter/src/main/java/com/transferwise/common/context/ownership/EntryPointOwnerAttributesChangeListener.java b/tw-context-ownership-starter/src/main/java/com/transferwise/common/context/ownership/EntryPointOwnerAttributesPutListener.java similarity index 87% rename from tw-context-ownership-starter/src/main/java/com/transferwise/common/context/ownership/EntryPointOwnerAttributesChangeListener.java rename to tw-context-ownership-starter/src/main/java/com/transferwise/common/context/ownership/EntryPointOwnerAttributesPutListener.java index 71660f6..2e85b82 100644 --- a/tw-context-ownership-starter/src/main/java/com/transferwise/common/context/ownership/EntryPointOwnerAttributesChangeListener.java +++ b/tw-context-ownership-starter/src/main/java/com/transferwise/common/context/ownership/EntryPointOwnerAttributesPutListener.java @@ -1,9 +1,10 @@ package com.transferwise.common.context.ownership; import com.transferwise.common.context.TwContext; -import com.transferwise.common.context.TwContextAttributeChangeListener; +import com.transferwise.common.context.TwContextAttributePutListener; import java.util.LinkedHashMap; import java.util.Map; +import java.util.Objects; import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReentrantLock; import lombok.extern.slf4j.Slf4j; @@ -11,7 +12,7 @@ import org.springframework.beans.factory.annotation.Autowired; @Slf4j -public class EntryPointOwnerAttributesChangeListener implements TwContextAttributeChangeListener { +public class EntryPointOwnerAttributesPutListener implements TwContextAttributePutListener { @Autowired private EntryPointOwnerProviderRegistry entryPointOwnerProviderRegistry; @@ -31,7 +32,10 @@ public boolean removeEldestEntry(Map.Entry, Boolean> eldest private final Lock defaultOwnersLock = new ReentrantLock(); @Override - public void attributeChanged(TwContext context, String key, Object oldValue, Object newValue) { + public void attributePut(TwContext context, String key, Object oldValue, Object newValue) { + if (Objects.equals(oldValue, newValue)) { + return; + } if (TwContext.NAME_KEY.equals(key)) { String epGroup = context.getGroup(); String epName = context.getName(); diff --git a/tw-context-ownership-starter/src/main/java/com/transferwise/common/context/ownership/TwContextOwnershipAutoConfiguration.java b/tw-context-ownership-starter/src/main/java/com/transferwise/common/context/ownership/TwContextOwnershipAutoConfiguration.java index 8b9706a..f34c5a4 100644 --- a/tw-context-ownership-starter/src/main/java/com/transferwise/common/context/ownership/TwContextOwnershipAutoConfiguration.java +++ b/tw-context-ownership-starter/src/main/java/com/transferwise/common/context/ownership/TwContextOwnershipAutoConfiguration.java @@ -1,7 +1,7 @@ package com.transferwise.common.context.ownership; import com.transferwise.common.context.TwContext; -import com.transferwise.common.context.TwContextAttributeChangeListener; +import com.transferwise.common.context.TwContextAttributePutListener; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.boot.context.properties.ConfigurationProperties; @@ -18,9 +18,9 @@ public EntryPointOwnerProviderRegistry entryPointOwnerProviderRegistry() { } @Bean - public TwContextAttributeChangeListener twContextOwnershipAttributesChangeListener() { - EntryPointOwnerAttributesChangeListener listener = new EntryPointOwnerAttributesChangeListener(); - TwContext.addAttributeChangeListener(listener); + public TwContextAttributePutListener twContextOwnershipAttributesChangeListener() { + EntryPointOwnerAttributesPutListener listener = new EntryPointOwnerAttributesPutListener(); + TwContext.addAttributePutListener(listener); return listener; } diff --git a/tw-context-ownership-starter/src/test/java/com/transferwise/common/context/ownership/OwnershipIntTest.java b/tw-context-ownership-starter/src/test/java/com/transferwise/common/context/ownership/OwnershipIntTest.java index e2d8add..1285dad 100644 --- a/tw-context-ownership-starter/src/test/java/com/transferwise/common/context/ownership/OwnershipIntTest.java +++ b/tw-context-ownership-starter/src/test/java/com/transferwise/common/context/ownership/OwnershipIntTest.java @@ -17,7 +17,7 @@ public class OwnershipIntTest { @Autowired - private EntryPointOwnerAttributesChangeListener entryPointOwnerAttributesChangeListener; + private EntryPointOwnerAttributesPutListener entryPointOwnerAttributesPutListener; @Test public void ownerShipIsMappedByConfiguration() { @@ -33,11 +33,11 @@ public void ownerShipIsMappedByConfiguration() { @Test void entrypointsWithoutOwnerShouldBeLoggedOnce() { - entryPointOwnerAttributesChangeListener.clearDefaultOwners(); + entryPointOwnerAttributesPutListener.clearDefaultOwners(); TwContext twContext = TwContext.current().createSubContext().asEntryPoint("Jobs", "testJob1"); - Logger logger = (Logger) LoggerFactory.getLogger(EntryPointOwnerAttributesChangeListener.class); + Logger logger = (Logger) LoggerFactory.getLogger(EntryPointOwnerAttributesPutListener.class); ListAppender listAppender = new ListAppender<>(); listAppender.start(); logger.addAppender(listAppender); diff --git a/tw-context/src/main/java/com/transferwise/common/context/TwContext.java b/tw-context/src/main/java/com/transferwise/common/context/TwContext.java index 777f308..4074e0f 100644 --- a/tw-context/src/main/java/com/transferwise/common/context/TwContext.java +++ b/tw-context/src/main/java/com/transferwise/common/context/TwContext.java @@ -7,7 +7,6 @@ import java.util.Iterator; import java.util.List; import java.util.Map; -import java.util.Objects; import java.util.Optional; import java.util.concurrent.CopyOnWriteArrayList; import java.util.function.Function; @@ -35,7 +34,7 @@ public class TwContext { private static final ThreadLocal> contextTl = new ThreadLocal<>(); private static final List interceptors = new CopyOnWriteArrayList<>(); - private static final List attributeChangeListeners = new CopyOnWriteArrayList<>(); + private static final List attributePutListeners = new CopyOnWriteArrayList<>(); private static final TwContext ROOT_CONTEXT = new TwContext(null, true); private static final RateLimiter throwableLoggingRateLimiter = RateLimiter.create(2); @@ -57,17 +56,17 @@ public static List getExecutionInterceptors() { return interceptors; } - public static void addAttributeChangeListener(@NonNull TwContextAttributeChangeListener listener) { - attributeChangeListeners.add(listener); + public static void addAttributePutListener(@NonNull TwContextAttributePutListener listener) { + attributePutListeners.add(listener); } - public static boolean removeAttributeChangeListener(TwContextAttributeChangeListener listener) { - return attributeChangeListeners.remove(listener); + public static boolean removeAttributePutListener(TwContextAttributePutListener listener) { + return attributePutListeners.remove(listener); } @SuppressFBWarnings(value = "MS", justification = "Performance") - public static List getAttributeChangeListeners() { - return attributeChangeListeners; + public static List getAttributePutListeners() { + return attributePutListeners; } public static void putCurrentMdc(@NonNull String key, String value) { @@ -204,17 +203,16 @@ public TwContext put(String key, Object newValue) { } Object oldValue = attributes.get(key); - if (!Objects.equals(oldValue, newValue)) { - if (newValue == null) { - attributes.remove(key); - newAttributes.remove(key); - } else { - attributes.put(key, newValue); - newAttributes.put(key, newValue); - } - fireAttributeChangeEvent(key, oldValue, newValue); + if (newValue == null) { + attributes.remove(key); + newAttributes.remove(key); + } else { + attributes.put(key, newValue); + newAttributes.put(key, newValue); } + fireAttributePutEvent(key, oldValue, newValue); + return this; } @@ -306,15 +304,15 @@ private T executeWithInterceptors(Supplier supplier, Iterator l.attributeChanged(this, key, oldValue, newValue)); + attributePutListeners.forEach((l) -> l.attributePut(this, key, oldValue, newValue)); } catch (Throwable t) { // This is just a safety net, every listener needs to be bullet-proof by themselves. if (throwableLoggingRateLimiter.tryAcquire()) { // Don't log value, could be PII. - log.error("Attribute change listener failed for key '" + key + "'.", t); + log.error("Attribute put listener failed for key '" + key + "'.", t); } } } diff --git a/tw-context/src/main/java/com/transferwise/common/context/TwContextAttributeChangeListener.java b/tw-context/src/main/java/com/transferwise/common/context/TwContextAttributeChangeListener.java deleted file mode 100644 index 6baf82d..0000000 --- a/tw-context/src/main/java/com/transferwise/common/context/TwContextAttributeChangeListener.java +++ /dev/null @@ -1,9 +0,0 @@ -package com.transferwise.common.context; - -import javax.annotation.concurrent.NotThreadSafe; - -@NotThreadSafe -public interface TwContextAttributeChangeListener { - - void attributeChanged(TwContext context, String key, Object oldValue, Object newValue); -} diff --git a/tw-context/src/main/java/com/transferwise/common/context/TwContextAttributePutListener.java b/tw-context/src/main/java/com/transferwise/common/context/TwContextAttributePutListener.java new file mode 100644 index 0000000..5cc42fc --- /dev/null +++ b/tw-context/src/main/java/com/transferwise/common/context/TwContextAttributePutListener.java @@ -0,0 +1,9 @@ +package com.transferwise.common.context; + +import javax.annotation.concurrent.NotThreadSafe; + +@NotThreadSafe +public interface TwContextAttributePutListener { + + void attributePut(TwContext context, String key, Object oldValue, Object newValue); +} diff --git a/tw-context/src/test/java/com/transferwise/common/context/TwContextAttributeChangeListenerTest.java b/tw-context/src/test/java/com/transferwise/common/context/TwContextAttributePutListenerTest.java similarity index 81% rename from tw-context/src/test/java/com/transferwise/common/context/TwContextAttributeChangeListenerTest.java rename to tw-context/src/test/java/com/transferwise/common/context/TwContextAttributePutListenerTest.java index 8a2dd06..3a01d78 100644 --- a/tw-context/src/test/java/com/transferwise/common/context/TwContextAttributeChangeListenerTest.java +++ b/tw-context/src/test/java/com/transferwise/common/context/TwContextAttributePutListenerTest.java @@ -6,13 +6,13 @@ import org.junit.jupiter.api.Test; import org.slf4j.MDC; -public class TwContextAttributeChangeListenerTest { +public class TwContextAttributePutListenerTest { @Test void nameAttributeIsChangedAfterGroupAttribute() { MutableObject groupChanged = new MutableObject<>(false); MutableObject nameChanged = new MutableObject<>(false); - TwContextAttributeChangeListener listener = (context, key, oldValue, newValue) -> { + TwContextAttributePutListener listener = (context, key, oldValue, newValue) -> { if (TwContext.GROUP_KEY.equals(key)) { groupChanged.setValue(true); } else if (TwContext.NAME_KEY.equals(key)) { @@ -22,20 +22,20 @@ void nameAttributeIsChangedAfterGroupAttribute() { nameChanged.setValue(true); } }; - TwContext.addAttributeChangeListener(listener); + TwContext.addAttributePutListener(listener); try { TwContext.current().createSubContext().asEntryPoint("SRE", "Task123"); assertThat(nameChanged.getValue()).isTrue(); assertThat(groupChanged.getValue()).isTrue(); } finally { - TwContext.removeAttributeChangeListener(listener); + TwContext.removeAttributePutListener(listener); } } @Test void ownerCanBeSetWhenNameIsChanged() { - TwContextAttributeChangeListener listener = (context, key, oldValue, newValue) -> { + TwContextAttributePutListener listener = (context, key, oldValue, newValue) -> { // Here we are making an assumption (covered by test in TwContext), that the name key is always changed after group key. // We could remove that assumption by deciding owner both on group key change and on name key change, but that would incur double work. if (TwContext.NAME_KEY.equals(key)) { @@ -47,7 +47,7 @@ void ownerCanBeSetWhenNameIsChanged() { } }; - TwContext.addAttributeChangeListener(listener); + TwContext.addAttributePutListener(listener); try { TwContext twContext = TwContext.current().createSubContext().asEntryPoint("Test", "/ep1"); assertThat(twContext.getOwner()).isEqualTo("Kristo"); @@ -59,7 +59,7 @@ void ownerCanBeSetWhenNameIsChanged() { assertThat(MDC.get(TwContext.MDC_KEY_EP_OWNER)).isEqualTo("Yurii"); }); } finally { - TwContext.removeAttributeChangeListener(listener); + TwContext.removeAttributePutListener(listener); } } }