raphw / byte-buddy

Runtime code generation for the Java virtual machine.
https://bytebuddy.net
Apache License 2.0
6.29k stars 807 forks source link

Fail to read property due to `IllegalArgumentException` when using it on withX methods #1601

Closed aviadsTaboola closed 1 month ago

aviadsTaboola commented 8 months ago

Hi, I am using lombok in java and make immutable object using the With annotation. tried to use byte-buddy Advice to recognize a change of property in a class , but since it is not a setter it throws an exception: withStatus does not follow Java bean naming conventions maybe java bean naming convention is too strict for this...

It will be great to be able to use @Advice.Origin("#p") String propertyName such that it will work with any method that contains the field name even if someone will call it updateX().

if this bug/feat does not seem proper to you I will be glad for a reference for what I can do it otherwise. my goal is to track any field change, so the best way I saw doing it is by assuming this naming convention + checking that the value has changed by returning a value on @Advice.OnMethodEnter and checking the value on exist is different than the value on enter with @Advice.Enter Object value.

thanks! full code usage:


static class FieldAssignmentInterceptor {
        final static Set<Field> dirtyFields = new HashSet<>();

        @Advice.OnMethodEnter
        public static Object enter(@Advice.Origin Class<?> clazz, // The class that contains the method
                                   @Advice.Origin("#m") String methodName, // The method that is being executed
                                   @Advice.Origin("#p") String propertyName, // The property that is being assigned
                                   @Advice.This Object target // The object that is being modified
        ) {

                try {
                    Field field = clazz.getDeclaredField(propertyName);
                    if (shouldTrackMethod(field, methodName, propertyName)) {
                        field.setAccessible(true);
                        return field.get(target);
                    }
                } catch (NoSuchFieldException | IllegalAccessException e) {

                }

            return new NoValueMarker();
        }

        @Advice.OnMethodExit
        public static void exit(@Advice.Origin Class<?> clazz, // The class that contains the method
                                @Advice.Origin("#m") String methodName, // The method that is being executed
                                @Advice.Origin("#p") String propertyName, // The property that is being assigned
                                @Advice.Enter Object value, // The value before the assignment
                                @Advice.This Object target // The object that is being modified
        ) {
            if (value instanceof NoValueMarker) {
                return;
            }

                try {
                    Field field = clazz.getDeclaredField(propertyName);
                    field.setAccessible(true);
                    Object newValue = field.get(target);
                    if (shouldTrackMethod(field, methodName, propertyName) && valueChanged(value, newValue)) {
                        dirtyFields.add(field);
                    }
                } catch (NoSuchFieldException | IllegalAccessException e) {

                }

        }

        private static boolean shouldTrackMethod(Field field, String methodName, String propertyName) {
            return field.isAnnotationPresent(UpdateOnlyOnExplicitAssignment.class) && isAccessorAdhereToNamingConvention(methodName, propertyName);
        }

        private static boolean valueChanged(Object originValue, Object newValue) {
            return !Objects.equals(originValue, newValue);
        }

        private static boolean isAccessorAdhereToNamingConvention(String methodName, String propertyName) {
            return methodName.toLowerCase().contains(propertyName.toLowerCase());
        }
    }
raphw commented 8 months ago

You can creste your own annotation and bind it to a Field as you want. Check the javadoc for further details.

aviadsTaboola commented 8 months ago

Hi, I need to track down fields that have changed, I already added annotation for it, I still need to find any method that might cause an assignment. to over come this blocker I extract property name from method name myself with:


private static Optional<String> extractPropertyName(String methodName) {
            // Extract property name from method name using regex
            // Skip initial lowercase letters until reaching an uppercase letter, then take the rest of the string
            Pattern pattern = Pattern.compile("[a-z]+([A-Z].*)");
            Matcher matcher = pattern.matcher(methodName);
            if (matcher.find()) {
                String propertyName = matcher.group(1);
                // Ensure the first letter is lowercase
                return Optional.of(Character.toLowerCase(propertyName.charAt(0)) + propertyName.substring(1));
            }
            return Optional.empty();
        }```
raphw commented 8 months ago

So you want to access if a method changes a property? I was thinking that you derive that from the method name.

aviadsTaboola commented 8 months ago

I am deriving this from method name, I want to be able to do it by using @Advice.Origin("#p") only without adding code of my own. currently, the code is like so:

public class DirtyFieldTracker {

    public static void trackFields(Set<Field> fields) {
        Set<Class<?>> classes = getClasses(fields);
        trackClasses(classes);
    }

    public static boolean isDirty(Field field, Object instance) {
        return AssignmentInterceptors.isDirty(field, instance);
    }

    private static void trackClasses(Set<Class<?>> classes) {
        for (Class<?> clazz : classes) {
            instrumentClass(clazz);
        }
    }

    private static Set<Class<?>> getClasses(Set<Field> potentialFields) {
        Set<Class<?>> classes = new HashSet<>();
        for (Field field : potentialFields) {
            classes.add(field.getDeclaringClass());
        }
        return classes;
    }

    private static void instrumentClass(Class<?> clazz) {
        try (val unloadedType = new ByteBuddy()
                .redefine(clazz)
                .visit(Advice.to(AssignmentInterceptors.SetInterceptor.class).on(isSetMethod()))
                .visit(Advice.to(AssignmentInterceptors.WithInterceptor.class).on(isWithMethod()))
                .make()) {

            final byte[] transformed = unloadedType.getBytes();
            // Get the current instrumentation
            Instrumentation instrumentation = ByteBuddyAgent.install();
            // Redefine the class using instrumentation
            ClassDefinition classDefinition = new ClassDefinition(clazz, transformed);
            instrumentation.redefineClasses(classDefinition);
            //unloadedType.load(clazz.getClassLoader(), ClassLoadingStrategy.Default.WRAPPER));
        } catch (IOException | UnmodifiableClassException | ClassNotFoundException e) {
            throw new RuntimeException(e);
        }
    }

    private static ElementMatcher.Junction<MethodDescription> isWithMethod() {
        return isMethod()
                .and(isPublic())
                .and(not(isStatic()))
                .and(not(isAbstract()))
                .and(nameStartsWith("with"));
    }

    static class AssignmentInterceptors {

        private final static Set<TrackedField> dirtyFields = new ConcurrentHashSet<>();

        public static boolean isDirty(Field field, Object instance) {
            return dirtyFields.contains(TrackedField.of(field, instance));
        }

        static class SetInterceptor {
            @Advice.OnMethodEnter
            public static PropertyData enter(@Advice.Origin Class<?> clazz, // The class that contains the method
                                             @Advice.Origin("#m") String methodName, // The method that is being executed
                                             @Advice.This Object target // The object that is being modified
            ) {
                return derivePropertyData(clazz, methodName, target);
            }

            @Advice.OnMethodExit
            public static void exit(@Advice.This Object target, // The object that is being modified
                                    @Advice.Enter PropertyData propertyData) { // The value returned by the enter method
                if (propertyData == null) {
                    return;
                }
                addWhenDirty(propertyData, target);
            }
        }

        static class WithInterceptor {
            @Advice.OnMethodEnter
            public static PropertyData enter(@Advice.Origin Class<?> clazz, // The class that contains the method
                                             @Advice.Origin("#m") String methodName, // The method that is being executed
                                             @Advice.This Object target // The object that is being modified
            ) {
                return derivePropertyData(clazz, methodName, target);
            }

            @Advice.OnMethodExit
            public static void exit(@Advice.Enter PropertyData propertyData,
                                    @Advice.Return(typing = Assigner.Typing.DYNAMIC) Object returnValue) { // The value returned by the enter method
                if (propertyData == null) {
                    return;
                }
                addWhenDirty(propertyData, returnValue);
            }
        }

        private static PropertyData derivePropertyData(Class<?> clazz, String methodName, Object target) {
            return extractPropertyName(methodName)
                    .map(propertyName -> buildPropertyData(clazz, target, propertyName))
                    .orElse(null);
        }

        private static void addWhenDirty(PropertyData propertyData, Object instance) {
            Field field = propertyData.getField();
            if (propertyData.hasNewAssignment(instance)) {
                dirtyFields.add(TrackedField.of(field, instance));
            }
        }

        private static PropertyData buildPropertyData(Class<?> clazz, Object target, String propertyName) {
            try {
                Field field = clazz.getDeclaredField(propertyName);
                field.setAccessible(true);
                return new PropertyData(propertyName, field, field.get(target));
            } catch (NoSuchFieldException | IllegalAccessException e) {

            }
            return null;
        }

        private static Optional<String> extractPropertyName(String methodName) {
            // Extract property name from method name using regex
            // Skip initial lowercase letters until reaching an uppercase letter, then take the rest of the string
            Pattern pattern = Pattern.compile("[a-z]+([A-Z].*)");
            Matcher matcher = pattern.matcher(methodName);
            if (matcher.find()) {
                String propertyName = matcher.group(1);
                // Ensure the first letter is lowercase
                return Optional.of(Character.toLowerCase(propertyName.charAt(0)) + propertyName.substring(1));
            }
            return Optional.empty();
        }

        @Value(staticConstructor = "of")
        static class TrackedField {
            Field field;
            Object target;
        }

        @Value(staticConstructor = "of")
        private static class PropertyData {
            String propertyName;
            Field field;
            Object value;

            boolean hasNewAssignment(Object instance) {
                try {
                    final Object newValue = field.get(instance);
                    return !Objects.equals(value, newValue);
                } catch (IllegalAccessException e) {

                    return false;
                }
            }
        }

    }
}

I also had to create seperate interceptors since @Advice.Return can not work with method that return void as Object type for setters. I would expect it to kind of cast it to Void type and not fail.

raphw commented 1 month ago

Sorry, I overlook this. Please ping me if this still is a problem.