eclipse-aspectj / aspectj

Other
303 stars 86 forks source link

Access to original field value for `set()` #154

Open kriegaex opened 2 years ago

kriegaex commented 2 years ago

The original enhancement request Bugzilla #95716 exists since 2005. I think it should be implemented. I needed this many times myself, and e.g. it also came up on Stack Overflow lately and last year.

package de.scrum_master.app;

public class Application {
  int number;
  private boolean isActive;
  private boolean isNice;

  public static void main(String[] args) {
    Application app = new Application();

    // Value change -> trigger logging aspect
    app.number = 11;
    // Value change -> trigger logging aspect
    app.isActive = true;
    // No value change
    app.isNice = false;

    // Value change -> trigger logging aspect
    app.number = 22;
    // No value change
    app.isActive = true;
    // Value change -> trigger logging aspect
    app.isNice = true;

    // No value change
    app.number = 22;
    // No value change
    app.isActive = true;
    // No value change
    app.isNice = true;
  }
}

Now we want to create an aspect logging something if the write access changes the field value, i.e. we need access to the old value. Another example might be to use the old value in order to calculate something from it for validation, say the new value must be no more than 25% higher than the old one.

The current workaround for around() looks like this:

package de.scrum_master.aspect;

import java.lang.reflect.Field;

import org.aspectj.lang.SoftException;
import org.aspectj.lang.reflect.FieldSignature;

public privileged aspect MyAspect {
  void around(Object instance, Object newValue) :
    set(* *) && target(instance) && args(newValue)
  {
    Object oldValue;
    try {
      oldValue = ((FieldSignature) thisJoinPointStaticPart.getSignature()).getField().get(instance);
    } catch (IllegalArgumentException | IllegalAccessException e) {
      throw new SoftException(e);
    }
    if (oldValue == null && newValue != null || oldValue != null && !oldValue.equals(newValue))
      System.out.println(thisJoinPoint + ": " + oldValue + " -> " + newValue);
    proceed(instance, newValue);
  }
}

For before(), we need an additional field.setAccessible(true), because the code accessing the field is generated in the aspect rather than in the target class:

  before(Object instance, Object newValue) :
    set(* *) && target(instance) && args(newValue)
  {
    Object oldValue;
    try {
      Field field = ((FieldSignature) thisJoinPointStaticPart.getSignature()).getField();
      field.setAccessible(true);
      oldValue = field.get(instance);
    } catch (IllegalArgumentException | IllegalAccessException e) {
      throw new SoftException(e);
    }
    if (oldValue == null && newValue != null || oldValue != null && !oldValue.equals(newValue))
      System.out.println(thisJoinPoint + ": " + oldValue + " -> " + newValue);
  }

Of course, in an AspectJ implementation there would be no reflection from the user's perspective, and a privileged aspect would be able to access non-public fields.

Syntax-wise I would keep it simple and not use something new like oldargs(oldValue), but instead simply an optional second argument, i.e. set(* *) && args(newValue, oldValue), which would, if present, bind the old value.

BTW, a reminder to myself and whoever else might need it: The way to get the field value in get() is to

FedericoBruzzone commented 1 year ago

Hi @kriegaex, I am a student attending the master's degree in computer science. I am studying and using aspectJ on a daily basis, the idea behind it is brilliant. I have a background in writing reflective code and bytecode engineering as well.

I was wondering if this part was implemented? Taking time away from other things, I wanted to start studying the repo, I saw that you are currently the maintainer, is it right?

kriegaex commented 1 year ago

I was wondering if this part was implemented?

@FedericoBruzzone: This has not been implemented yet, hence the open issue.

I saw that you are currently the maintainer, is it right?

Yes, this is basically correct. While not being the official project lead - that would be @aclement - I am currently the most active committer. Do you have any specific questions?

FedericoBruzzone commented 1 year ago

Yes, this is basically correct. While not being the official project lead - that would be @aclement - I am currently the most active committer. Do you have any specific questions?

Thanks for your time. @kriegaex I have not run into the problem of this issue. But in the next few days I will study the repo and maybe I'll ask you some question if I'm allowed.