From f0bba48acf6294591b7aad8217d2e3dbea69954e Mon Sep 17 00:00:00 2001 From: Ben Donnelly Date: Tue, 19 Sep 2023 14:08:41 +0100 Subject: [PATCH 1/4] change(reflection): reduce duplicate reflection code and proxy style usage --- .../agent/api/plugin/ISnapshotContext.java | 9 ++++ .../com/intergral/deep/agent/AgentImpl.java | 2 +- .../com/intergral/deep/agent/DeepAgent.java | 2 +- .../{ReflectionUtils.java => Reflection.java} | 42 +++++-------------- .../deep/agent/grpc/GrpcService.java | 4 +- .../agent/tracepoint/cf/CFFrameProcessor.java | 33 ++++++++------- .../deep/agent/tracepoint/cf/CFUtils.java | 14 ++++--- .../tracepoint/handler/FrameProcessor.java | 10 +++++ .../tracepoint/handler/VariableProcessor.java | 18 +++++--- .../deep/agent/plugins/PluginLoaderTest.java | 8 ++-- pom.xml | 1 + 11 files changed, 77 insertions(+), 66 deletions(-) rename agent/src/main/java/com/intergral/deep/agent/{ReflectionUtils.java => Reflection.java} (54%) diff --git a/agent-api/src/main/java/com/intergral/deep/agent/api/plugin/ISnapshotContext.java b/agent-api/src/main/java/com/intergral/deep/agent/api/plugin/ISnapshotContext.java index 2ec8354..be0b95d 100644 --- a/agent-api/src/main/java/com/intergral/deep/agent/api/plugin/ISnapshotContext.java +++ b/agent-api/src/main/java/com/intergral/deep/agent/api/plugin/ISnapshotContext.java @@ -17,6 +17,8 @@ package com.intergral.deep.agent.api.plugin; +import com.intergral.deep.agent.api.reflection.IReflection; + /** * This is the context passed to plugins. This allows for the data of a context to be exposed to the plugin in a controlled manor. */ @@ -30,4 +32,11 @@ public interface ISnapshotContext { * @throws EvaluationException if there were any issues evaluating the expression */ String evaluateExpression(String expression) throws EvaluationException; + + /** + * Get the current reflection service. + * + * @return the active reflection service. + */ + IReflection reflectionService(); } diff --git a/agent/src/main/java/com/intergral/deep/agent/AgentImpl.java b/agent/src/main/java/com/intergral/deep/agent/AgentImpl.java index 94c4bd2..8a52566 100644 --- a/agent/src/main/java/com/intergral/deep/agent/AgentImpl.java +++ b/agent/src/main/java/com/intergral/deep/agent/AgentImpl.java @@ -91,7 +91,7 @@ public IDeep deepService() { @Override public IReflection reflectionService() { - return ReflectionUtils.getReflection(); + return Reflection.getInstance(); } }; } diff --git a/agent/src/main/java/com/intergral/deep/agent/DeepAgent.java b/agent/src/main/java/com/intergral/deep/agent/DeepAgent.java index 2c95007..4660899 100644 --- a/agent/src/main/java/com/intergral/deep/agent/DeepAgent.java +++ b/agent/src/main/java/com/intergral/deep/agent/DeepAgent.java @@ -76,7 +76,7 @@ public DeepAgent(final Settings settings, * Start deep. */ public void start() { - final List iLoadedPlugins = PluginLoader.loadPlugins(settings, ReflectionUtils.getReflection()); + final List iLoadedPlugins = PluginLoader.loadPlugins(settings, Reflection.getInstance()); final Resource resource = ResourceDetector.configureResource(settings, DeepAgent.class.getClassLoader()); this.settings.setPlugins(iLoadedPlugins); this.settings.setResource(Resource.DEFAULT.merge(resource)); diff --git a/agent/src/main/java/com/intergral/deep/agent/ReflectionUtils.java b/agent/src/main/java/com/intergral/deep/agent/Reflection.java similarity index 54% rename from agent/src/main/java/com/intergral/deep/agent/ReflectionUtils.java rename to agent/src/main/java/com/intergral/deep/agent/Reflection.java index bce76ad..dd226c8 100644 --- a/agent/src/main/java/com/intergral/deep/agent/ReflectionUtils.java +++ b/agent/src/main/java/com/intergral/deep/agent/Reflection.java @@ -19,54 +19,34 @@ import com.intergral.deep.agent.api.reflection.IReflection; import com.intergral.deep.reflect.ReflectionImpl; -import java.lang.reflect.Field; -import java.lang.reflect.Method; -import java.util.Iterator; -import java.util.Set; /** * A collection of utils that simplify the use of reflection. */ -public final class ReflectionUtils { +public final class Reflection { - private ReflectionUtils() { + private Reflection() { } private static final IReflection reflection; static { + // we need to change the reflection service we use based on version of java if (Utils.getJavaVersion() >= 9) { + // if we are version 9 or more. Then use the version 9 reflection reflection = new com.intergral.deep.reflect.Java9ReflectionImpl(); } else { + // else use the java 8 version reflection = new ReflectionImpl(); } } - public static IReflection getReflection() { + /** + * Get the active version of reflection to use. + * + * @return the reflection service. + */ + public static IReflection getInstance() { return reflection; } - - public static T callMethod(Object target, String methodName, Object... args) { - return getReflection().callMethod(target, methodName, args); - } - - public static Method findMethod(Class clazz, String methodName, Class... argTypes) { - return getReflection().findMethod(clazz, methodName, argTypes); - } - - public static T getFieldValue(Object target, String fieldName) { - return getReflection().getFieldValue(target, fieldName); - } - - public static Iterator getFieldIterator(final Class clazz) { - return getReflection().getFieldIterator(clazz); - } - - public static T callField(final Object target, final Field field) { - return getReflection().callField(target, field); - } - - public static Set getModifiers(final Field field) { - return getReflection().getModifiers(field); - } } diff --git a/agent/src/main/java/com/intergral/deep/agent/grpc/GrpcService.java b/agent/src/main/java/com/intergral/deep/agent/grpc/GrpcService.java index a83326b..ee5b6ba 100644 --- a/agent/src/main/java/com/intergral/deep/agent/grpc/GrpcService.java +++ b/agent/src/main/java/com/intergral/deep/agent/grpc/GrpcService.java @@ -17,7 +17,7 @@ package com.intergral.deep.agent.grpc; -import com.intergral.deep.agent.ReflectionUtils; +import com.intergral.deep.agent.Reflection; import com.intergral.deep.agent.api.auth.AuthProvider; import com.intergral.deep.agent.api.auth.IAuthProvider; import com.intergral.deep.agent.api.settings.ISettings; @@ -152,7 +152,7 @@ public SnapshotServiceGrpc.SnapshotServiceStub snapshotService() { } private Metadata buildMetaData() { - final IAuthProvider provider = AuthProvider.provider(this.settings, ReflectionUtils.getReflection()); + final IAuthProvider provider = AuthProvider.provider(this.settings, Reflection.getInstance()); final Map headers = provider.provide(); final Metadata metadata = new Metadata(); diff --git a/agent/src/main/java/com/intergral/deep/agent/tracepoint/cf/CFFrameProcessor.java b/agent/src/main/java/com/intergral/deep/agent/tracepoint/cf/CFFrameProcessor.java index 86078cb..c43ca80 100644 --- a/agent/src/main/java/com/intergral/deep/agent/tracepoint/cf/CFFrameProcessor.java +++ b/agent/src/main/java/com/intergral/deep/agent/tracepoint/cf/CFFrameProcessor.java @@ -17,9 +17,10 @@ package com.intergral.deep.agent.tracepoint.cf; -import com.intergral.deep.agent.ReflectionUtils; +import com.intergral.deep.agent.Reflection; import com.intergral.deep.agent.Utils; import com.intergral.deep.agent.api.plugin.IEvaluator; +import com.intergral.deep.agent.api.reflection.IReflection; import com.intergral.deep.agent.settings.Settings; import com.intergral.deep.agent.tracepoint.handler.FrameProcessor; import com.intergral.deep.agent.types.TracePointConfig; @@ -87,6 +88,7 @@ Map mapCFScopes(final Map variables) { final Object localScope = variables.get("__localScope"); if (CFUtils.isScope(localScope)) { + //noinspection unchecked,rawtypes final Map lclMap = (Map) localScope; for (Map.Entry entry : lclMap.entrySet()) { final Object name = entry.getKey(); @@ -102,14 +104,15 @@ Map mapCFScopes(final Map variables) { } // handle var scope - final Object varScope = ReflectionUtils.getFieldValue(pageContext, "SymTab_varScope"); + final IReflection reflection = Reflection.getInstance(); + final Object varScope = reflection.getFieldValue(pageContext, "SymTab_varScope"); if (CFUtils.isScope(varScope)) { cfVars.put("VARIABLES", varScope); } // find the other build in scopes - final Map scopes = ReflectionUtils.getFieldValue(pageContext, + final Map scopes = reflection.getFieldValue(pageContext, "SymTab_builtinCFScopes"); if (scopes == null) { return cfVars; @@ -146,13 +149,14 @@ Map convertLuceeScopes(final Map variables) { final Map scopes = new HashMap<>(); // process the scopes from lucee - scopes.put("variables", ReflectionUtils.getFieldValue(param0, "variables")); - scopes.put("argument", ReflectionUtils.getFieldValue(param0, "argument")); - scopes.put("local", getAndCheckLocal("local", param0)); + final IReflection reflection = Reflection.getInstance(); + scopes.put("variables", reflection.getFieldValue(param0, "variables")); + scopes.put("argument", reflection.getFieldValue(param0, "argument")); + scopes.put("local", getAndCheckLocal(param0)); scopes.put("cookie", getAndCheckScope("cookie", param0)); - scopes.put("server", ReflectionUtils.getFieldValue(param0, "server")); + scopes.put("server", reflection.getFieldValue(param0, "server")); scopes.put("session", getAndCheckScope("session", param0)); - scopes.put("application", ReflectionUtils.getFieldValue(param0, "application")); + scopes.put("application", reflection.getFieldValue(param0, "application")); scopes.put("cgi", getAndCheckScope("cgiR", param0)); scopes.put("request", getAndCheckScope("request", param0)); scopes.put("form", getAndCheckScope("_form", param0)); @@ -165,15 +169,13 @@ Map convertLuceeScopes(final Map variables) { /** - * This method will get anc check that the field is a local scope as some parts can have no local - * scope. + * This method will get and check that the field is a local scope as some parts can have no local scope. * - * @param local the name of the field to look for * @param param0 the object to look at * @return {@code null} if the field is not a valid local scope */ - private Object getAndCheckLocal(final String local, final Object param0) { - final Object o = ReflectionUtils.getFieldValue(param0, local); + private Object getAndCheckLocal(final Object param0) { + final Object o = Reflection.getInstance().getFieldValue(param0, "local"); if (o == null || o.getClass().getName() .equals("lucee.runtime.type.scope.LocalNotSupportedScope")) { return null; @@ -192,9 +194,10 @@ private Object getAndCheckLocal(final String local, final Object param0) { * discovered. */ private Object getAndCheckScope(final String name, final Object target) { - final Object local = ReflectionUtils.getFieldValue(target, name); + final IReflection reflection = Reflection.getInstance(); + final Object local = reflection.getFieldValue(target, name); if (local != null) { - final Object isInitalized = ReflectionUtils.callMethod(local, "isInitalized"); + final Object isInitalized = reflection.callMethod(local, "isInitalized"); if (isInitalized == null) { return null; } diff --git a/agent/src/main/java/com/intergral/deep/agent/tracepoint/cf/CFUtils.java b/agent/src/main/java/com/intergral/deep/agent/tracepoint/cf/CFUtils.java index e577807..669a38d 100644 --- a/agent/src/main/java/com/intergral/deep/agent/tracepoint/cf/CFUtils.java +++ b/agent/src/main/java/com/intergral/deep/agent/tracepoint/cf/CFUtils.java @@ -17,9 +17,10 @@ package com.intergral.deep.agent.tracepoint.cf; -import com.intergral.deep.agent.ReflectionUtils; +import com.intergral.deep.agent.Reflection; import com.intergral.deep.agent.Utils; import com.intergral.deep.agent.api.plugin.IEvaluator; +import com.intergral.deep.agent.api.reflection.IReflection; import com.intergral.deep.agent.types.TracePointConfig; import java.lang.reflect.Method; import java.net.URL; @@ -56,9 +57,10 @@ public static IEvaluator findCfEval(final Map variables) { return null; } - Method evaluate = ReflectionUtils.findMethod(page.getClass(), "Evaluate", String.class); + final IReflection reflection = Reflection.getInstance(); + Method evaluate = reflection.findMethod(page.getClass(), "Evaluate", String.class); if (evaluate == null) { - evaluate = ReflectionUtils.findMethod(page.getClass(), "Evaluate", Object.class); + evaluate = reflection.findMethod(page.getClass(), "Evaluate", Object.class); if (evaluate == null) { return null; } @@ -73,7 +75,7 @@ private static IEvaluator findLuceeEvaluator(final Map map) { return null; } - final Method evaluate = ReflectionUtils.findMethod(param0.getClass(), "evaluate", String.class); + final Method evaluate = Reflection.getInstance().findMethod(param0.getClass(), "evaluate", String.class); if (evaluate == null) { return null; } @@ -98,7 +100,7 @@ public static String findUdfName(final Map variables, final Stri return null; } // explicitly set to Object as otherwise the call to String.valueOf can become the char[] version. - return String.valueOf(ReflectionUtils.getFieldValue(aThis, "key")); + return String.valueOf(Reflection.getInstance().getFieldValue(aThis, "key")); } else if (className.startsWith("cf") && className.contains("$func")) { return className.substring(className.indexOf("$func") + 5); } else { @@ -151,7 +153,7 @@ public static Object findPageContext(final Map localVars) { if (page == null) { return null; } - return ReflectionUtils.getFieldValue(page, "pageContext"); + return Reflection.getInstance().getFieldValue(page, "pageContext"); } diff --git a/agent/src/main/java/com/intergral/deep/agent/tracepoint/handler/FrameProcessor.java b/agent/src/main/java/com/intergral/deep/agent/tracepoint/handler/FrameProcessor.java index a36e697..cbd6cc0 100644 --- a/agent/src/main/java/com/intergral/deep/agent/tracepoint/handler/FrameProcessor.java +++ b/agent/src/main/java/com/intergral/deep/agent/tracepoint/handler/FrameProcessor.java @@ -17,10 +17,12 @@ package com.intergral.deep.agent.tracepoint.handler; +import com.intergral.deep.agent.Reflection; import com.intergral.deep.agent.Utils; import com.intergral.deep.agent.api.plugin.EvaluationException; import com.intergral.deep.agent.api.plugin.IEvaluator; import com.intergral.deep.agent.api.plugin.ISnapshotContext; +import com.intergral.deep.agent.api.reflection.IReflection; import com.intergral.deep.agent.api.resource.Resource; import com.intergral.deep.agent.settings.Settings; import com.intergral.deep.agent.types.TracePointConfig; @@ -162,6 +164,14 @@ public String evaluateExpression(final String expression) throws EvaluationExcep } } + /** + * {@inheritDoc} + */ + @Override + public IReflection reflectionService() { + return Reflection.getInstance(); + } + /** * This defines a functional interface to allow for creating difference processors in the Callback. */ diff --git a/agent/src/main/java/com/intergral/deep/agent/tracepoint/handler/VariableProcessor.java b/agent/src/main/java/com/intergral/deep/agent/tracepoint/handler/VariableProcessor.java index 2ed6d08..850bc3d 100644 --- a/agent/src/main/java/com/intergral/deep/agent/tracepoint/handler/VariableProcessor.java +++ b/agent/src/main/java/com/intergral/deep/agent/tracepoint/handler/VariableProcessor.java @@ -17,8 +17,9 @@ package com.intergral.deep.agent.tracepoint.handler; -import com.intergral.deep.agent.ReflectionUtils; +import com.intergral.deep.agent.Reflection; import com.intergral.deep.agent.Utils; +import com.intergral.deep.agent.api.reflection.IReflection; import com.intergral.deep.agent.api.utils.ArrayObjectIterator; import com.intergral.deep.agent.api.utils.CompoundIterator; import com.intergral.deep.agent.tracepoint.handler.bfs.Node; @@ -342,6 +343,7 @@ protected boolean checkDepth(final int depth) { return depth + 1 < this.frameConfig.maxDepth(); } + @SuppressWarnings("BooleanMethodIsAlwaysInverted") protected boolean checkVarCount() { return varCache.size() <= this.frameConfig.maxVariables(); } @@ -496,6 +498,8 @@ public INamedItem next() { final String name = getFieldName(next); seenNames.add(next.getName()); return new INamedItem() { + private final IReflection reflection = Reflection.getInstance(); + @Override public String name() { return name; @@ -503,7 +507,7 @@ public String name() { @Override public Object item() { - return ReflectionUtils.callField(target, next); + return reflection.callField(target, next); } @Override @@ -517,7 +521,7 @@ public String originalName() { @Override public Set modifiers() { - return ReflectionUtils.getModifiers(next); + return reflection.getModifiers(next); } }; } @@ -540,20 +544,22 @@ private String getFieldName(final Field next) { } /** - * A simple iterator that used {@link ReflectionUtils} to create iterators for the {@link Field} that exist on an object. This allows us + * A simple iterator that used {@link Reflection} to create iterators for the {@link Field} that exist on an object. This allows us * to feed the fields into the {@link NamedFieldIterator}. */ private static class FieldIterator implements Iterator { + @SuppressWarnings("FieldCanBeLocal") private final Class clazz; private final Iterator iterator; public FieldIterator(final Class clazz) { this.clazz = clazz; + final IReflection reflection = Reflection.getInstance(); if (this.clazz.getSuperclass() == null || this.clazz.getSuperclass() == Object.class) { - this.iterator = ReflectionUtils.getFieldIterator(clazz); + this.iterator = reflection.getFieldIterator(clazz); } else { - this.iterator = new CompoundIterator<>(ReflectionUtils.getFieldIterator(clazz), + this.iterator = new CompoundIterator<>(reflection.getFieldIterator(clazz), new FieldIterator(this.clazz.getSuperclass())); } } diff --git a/agent/src/test/java/com/intergral/deep/agent/plugins/PluginLoaderTest.java b/agent/src/test/java/com/intergral/deep/agent/plugins/PluginLoaderTest.java index 8c27177..9a66886 100644 --- a/agent/src/test/java/com/intergral/deep/agent/plugins/PluginLoaderTest.java +++ b/agent/src/test/java/com/intergral/deep/agent/plugins/PluginLoaderTest.java @@ -19,7 +19,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; -import com.intergral.deep.agent.ReflectionUtils; +import com.intergral.deep.agent.Reflection; import com.intergral.deep.agent.api.plugin.IPlugin; import com.intergral.deep.agent.api.plugin.ISnapshotContext; import com.intergral.deep.agent.api.resource.Resource; @@ -35,7 +35,7 @@ class PluginLoaderTest { @Test void canLoadNoPlugins() { - final List iPlugins = PluginLoader.loadPlugins(Settings.build(Collections.emptyMap()), ReflectionUtils.getReflection()); + final List iPlugins = PluginLoader.loadPlugins(Settings.build(Collections.emptyMap()), Reflection.getInstance()); assertEquals(1, iPlugins.size()); assertEquals(iPlugins.get(0).name(), JavaPlugin.class.getName()); } @@ -44,7 +44,7 @@ void canLoadNoPlugins() { void canLoadBadPlugin() { final HashMap agentArgs = new HashMap<>(); agentArgs.put(ISettings.PLUGINS, BadPlugin.class.getName()); - final List iPlugins = PluginLoader.loadPlugins(Settings.build(agentArgs), ReflectionUtils.getReflection()); + final List iPlugins = PluginLoader.loadPlugins(Settings.build(agentArgs), Reflection.getInstance()); assertEquals(0, iPlugins.size()); } @@ -52,7 +52,7 @@ void canLoadBadPlugin() { void canLoadGoodPlugin() { final HashMap agentArgs = new HashMap<>(); agentArgs.put(ISettings.PLUGINS, GoodPlugin.class.getName()); - final List iPlugins = PluginLoader.loadPlugins(Settings.build(agentArgs), ReflectionUtils.getReflection()); + final List iPlugins = PluginLoader.loadPlugins(Settings.build(agentArgs), Reflection.getInstance()); assertEquals(1, iPlugins.size()); assertEquals(iPlugins.get(0).name(), GoodPlugin.class.getName()); } diff --git a/pom.xml b/pom.xml index a5bf9f4..d3299d8 100644 --- a/pom.xml +++ b/pom.xml @@ -82,6 +82,7 @@ plugins test-utils it-tests + plugins/otel-plugin From f58e949b9d356181e99ae34378cfd3e5a1be0028 Mon Sep 17 00:00:00 2001 From: Ben Donnelly Date: Tue, 19 Sep 2023 14:37:24 +0100 Subject: [PATCH 2/4] change(reflection): reduce duplicate reflection code and proxy style usage --- pom.xml | 1 - 1 file changed, 1 deletion(-) diff --git a/pom.xml b/pom.xml index d3299d8..a5bf9f4 100644 --- a/pom.xml +++ b/pom.xml @@ -82,7 +82,6 @@ plugins test-utils it-tests - plugins/otel-plugin From 3416c90bdfb124bcf5d1f5221a9b96f3237b8567 Mon Sep 17 00:00:00 2001 From: Ben Donnelly Date: Mon, 25 Sep 2023 15:45:43 +0100 Subject: [PATCH 3/4] fix(test): fix test coverage --- .../deep/agent/tracepoint/handler/FrameProcessorTest.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/agent/src/test/java/com/intergral/deep/agent/tracepoint/handler/FrameProcessorTest.java b/agent/src/test/java/com/intergral/deep/agent/tracepoint/handler/FrameProcessorTest.java index 1b66f9c..c479d42 100644 --- a/agent/src/test/java/com/intergral/deep/agent/tracepoint/handler/FrameProcessorTest.java +++ b/agent/src/test/java/com/intergral/deep/agent/tracepoint/handler/FrameProcessorTest.java @@ -36,6 +36,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import jdk.internal.reflect.Reflection; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.Mockito; @@ -172,4 +173,9 @@ void willCollect_watches_2() { void willEvaluate() throws EvaluationException { assertEquals("100", frameProcessor.evaluateExpression("this.i")); } + + @Test + void reflection() { + assertNotNull(frameProcessor.reflectionService()); + } } \ No newline at end of file From f745fed80a34bdd5bf9d28cd7736ec3ea688e922 Mon Sep 17 00:00:00 2001 From: Ben Donnelly Date: Mon, 25 Sep 2023 15:46:08 +0100 Subject: [PATCH 4/4] fix(test): clean up test --- .../deep/agent/tracepoint/handler/FrameProcessorTest.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/agent/src/test/java/com/intergral/deep/agent/tracepoint/handler/FrameProcessorTest.java b/agent/src/test/java/com/intergral/deep/agent/tracepoint/handler/FrameProcessorTest.java index c479d42..3d9aaec 100644 --- a/agent/src/test/java/com/intergral/deep/agent/tracepoint/handler/FrameProcessorTest.java +++ b/agent/src/test/java/com/intergral/deep/agent/tracepoint/handler/FrameProcessorTest.java @@ -36,16 +36,15 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; -import jdk.internal.reflect.Reflection; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.Mockito; class FrameProcessorTest { - private Settings settings = Mockito.mock(Settings.class); - private IEvaluator evaluator = EvaluatorService.createEvaluator(); - private Collection tracepoints = new ArrayList<>(); + private final Settings settings = Mockito.mock(Settings.class); + private final IEvaluator evaluator = EvaluatorService.createEvaluator(); + private final Collection tracepoints = new ArrayList<>(); private FrameProcessor frameProcessor; @BeforeEach