Skip to content

Commit

Permalink
Add After callsites support for void methods (#8116)
Browse files Browse the repository at this point in the history
  • Loading branch information
manuel-alvarez-alvarez authored Dec 20, 2024
1 parent 8b2bb8e commit a3e9bda
Show file tree
Hide file tree
Showing 8 changed files with 147 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -570,11 +570,20 @@ public AfterSpecification(

@Override
protected void validateAdvice(@Nonnull final ValidationContext context) {
if (advice.isVoidReturn()) {
context.addError(ErrorCode.ADVICE_AFTER_SHOULD_NOT_RETURN_VOID);
}
if (findReturn() == null) {
context.addError(ErrorCode.ADVICE_AFTER_SHOULD_HAVE_RETURN);
if (shouldNotUseReturn(pointcut)) {
if (!advice.isVoidReturn()) {
context.addError(ErrorCode.ADVICE_AFTER_VOID_METHOD_SHOULD_RETURN_VOID);
}
if (findReturn() != null) {
context.addError(ErrorCode.ADVICE_AFTER_VOID_METHOD_SHOULD_NOT_HAVE_RETURN);
}
} else {
if (advice.isVoidReturn()) {
context.addError(ErrorCode.ADVICE_AFTER_SHOULD_NOT_RETURN_VOID);
}
if (findReturn() == null) {
context.addError(ErrorCode.ADVICE_AFTER_SHOULD_HAVE_RETURN);
}
}
if (!pointcut.isConstructor()) {
if (!isStaticPointcut() && !includeThis()) {
Expand All @@ -588,6 +597,10 @@ protected void validateAdvice(@Nonnull final ValidationContext context) {
super.validateAdvice(context);
}

private boolean shouldNotUseReturn(final MethodType type) {
return !type.isConstructor() && type.isVoidReturn();
}

@Override
public String toString() {
return "@CallSite.After(" + signature + ")";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,20 @@ public String apply(final Object[] objects) {
}
},

ADVICE_AFTER_VOID_METHOD_SHOULD_RETURN_VOID {
@Override
public String apply(final Object[] objects) {
return "After advice for void method should return void";
}
},

ADVICE_AFTER_VOID_METHOD_SHOULD_NOT_HAVE_RETURN {
@Override
public String apply(final Object[] objects) {
return "After advice for void method should not contain @Return annotated parameters";
}
},

ADVICE_AFTER_SHOULD_HAVE_RETURN {
@Override
public String apply(final Object[] objects) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,36 @@ final class AdviceGeneratorTest extends BaseCsiPluginTest {
}
}


@CallSite(spi = CallSites)
class AfterAdviceWithVoidReturn {
@CallSite.After("void java.lang.StringBuilder.setLength(int)")
static void after(@CallSite.This StringBuilder self, @CallSite.Argument(0) int length) {
}
}

void 'test after advice with void return'() {
setup:
final spec = buildClassSpecification(AfterAdviceWithVoidReturn)
final generator = buildAdviceGenerator(buildDir)

when:
final result = generator.generate(spec)

then:
assertNoErrors result
assertCallSites(result.file) {
advices(0) {
pointcut('java/lang/StringBuilder', 'setLength', '(I)V')
statements(
'handler.dupInvoke(owner, descriptor, StackDupMode.COPY);',
'handler.method(opcode, owner, name, descriptor, isInterface);',
'handler.advice("datadog/trace/plugin/csi/impl/AdviceGeneratorTest$AfterAdviceWithVoidReturn", "after", "(Ljava/lang/StringBuilder;I)V");',
)
}
}
}

private static AdviceGenerator buildAdviceGenerator(final File targetFolder) {
return new AdviceGeneratorImpl(targetFolder, pointcutParser())
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -542,4 +542,26 @@ class AdviceSpecificationTest extends BaseCsiPluginTest {
then:
0 * context.addError(_, _)
}


@CallSite(spi = CallSites)
class AfterWithVoidWrongAdvice {
@CallSite.After("void java.lang.String.getChars(int, int, char[], int)")
static String after(@CallSite.AllArguments final Object[] args, @CallSite.Return final String result) {
return result;
}
}

void 'test after advice with void should not use @Return'() {
setup:
final context = mockValidationContext()
final spec = buildClassSpecification(AfterWithVoidWrongAdvice)

when:
spec.advices.each { it.validate(context) }

then:
1 * context.addError(ErrorCode.ADVICE_AFTER_VOID_METHOD_SHOULD_RETURN_VOID, _)
1 * context.addError(ErrorCode.ADVICE_AFTER_VOID_METHOD_SHOULD_NOT_HAVE_RETURN, _)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,10 @@ class BaseCallSiteTest extends DDSpecification {
return buildPointcut(String.getDeclaredMethod('concat', String))
}

protected static Pointcut stringBuilderSetLengthPointcut() {
return buildPointcut(StringBuilder.getDeclaredMethod('setLength', int))
}

protected static Pointcut stringReaderPointcut() {
return buildPointcut(StringReader.getDeclaredConstructor(String))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import net.bytebuddy.dynamic.DynamicType
import net.bytebuddy.jar.asm.Opcodes
import net.bytebuddy.jar.asm.Type

import java.util.function.BiConsumer
import java.util.function.BiFunction
import java.util.function.Consumer

Expand Down Expand Up @@ -227,6 +228,38 @@ class CallSiteTransformerTest extends BaseCallSiteTest {
helperCLass != null
}

void 'test after advice with void return'() {
setup:
final source = Type.getType(StringBuilderSetLengthExample)
final target = renameType(source, 'Test')
final pointcut = stringBuilderSetLengthPointcut()
final advice = Mock(InvokeAdvice)
final callSite = Type.getType(StringBuilderSetLengthCallSite)
final callSiteTransformer = new CallSiteTransformer(mockAdvices([mockCallSites(advice, pointcut)]))
final sb = new StringBuilder("Hello World!")
final int length = 5
StringBuilderSetLengthCallSite.LAST_CALL = null

when:
final transformedClass = transformType(source, target, callSiteTransformer)
final instance = loadType(target, transformedClass) as BiConsumer<StringBuilder, Integer>
instance.accept(sb, length)

then:
1 * advice.apply(_ as MethodHandler, Opcodes.INVOKEVIRTUAL, pointcut.type, pointcut.method, pointcut.descriptor, false) >> { params ->
final args = params as Object[]
final handler = args[0] as MethodHandler
final owner = args[2] as String
final descriptor = args[4] as String
handler.dupInvoke(owner, descriptor, COPY)
handler.method(args[1] as int, owner, args[3] as String, descriptor, args[5] as Boolean)
handler.advice(callSite.getInternalName(), "after", "(Ljava/lang/StringBuilder;I)V")
}
sb.toString() == 'Hello'
StringBuilderSetLengthCallSite.LAST_CALL[0] == sb
StringBuilderSetLengthCallSite.LAST_CALL[1] == length
}

static class InstrumentationHelper {

private static Consumer<Object[]> callback = null // codenarc forces the lowercase name
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package datadog.trace.agent.tooling.csi;

public class StringBuilderSetLengthCallSite {

public static volatile Object[] LAST_CALL;

public static void after(final StringBuilder builder, int length) {
LAST_CALL = new Object[] {builder, length};
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package datadog.trace.agent.tooling.csi;

import java.util.function.BiConsumer;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class StringBuilderSetLengthExample implements BiConsumer<StringBuilder, Integer> {

private static final Logger LOGGER = LoggerFactory.getLogger(StringBuilderSetLengthExample.class);

public void accept(final StringBuilder builder, final Integer length) {
LOGGER.debug("Before apply {} {}", builder, length);
builder.setLength(length);
LOGGER.debug("After apply {} {}", builder, length);
}
}

0 comments on commit a3e9bda

Please sign in to comment.