Configure default behaviour for annotations

[ghost]

Consider the following:

@Loggable(Loggable.DEBUG, prepend=true)
public String loadA(URL url) {
  return url.openConnection().getContent();
}

@Loggable(Loggable.DEBUG, prepend=true)
public String loadB(URL url) {
  return url.openConnection().getContent();
}

If this setting is a common occurrence, then it means that the annotation itself becomes duplicated code. Configuring the default annotation should be possible so that the above code becomes:

@Loggable
public String loadA(URL url) {
  return url.openConnection().getContent();
}

@Loggable
public String loadB(URL url) {
  return url.openConnection().getContent();
}

This partially addresses the issue. A full address of the issue would allow for a configuration file wherein the default behaviour can be applied at the package, class, or method level. For example:

<config>
  <annotation scope="class">
    <level>Level.DEBUG</level>
    <prepend>true</prepend>
  </annotation>
</config>

The aforementioned source code snippet then becomes:

@Loggable
public class AllMethodsLogged {
public String loadA(URL url) {
  return url.openConnection().getContent();
}

public String loadB(URL url) {
  return url.openConnection().getContent();
}
}

Alternatively, if using a purely XML driven configuration, the following would be possible:

<config>
  <annotation scope="com.company.ClassName">
    <level>Level.DEBUG</level>
    <prepend>true</prepend>
  </annotation>
</config>

No need to add any annotation, nor explicitly program aspects. This could be further refined:

  <annotation package="com.company"> // all classes in package
  <annotation package="com.company" class="ClassName"> // all methods in class
  <annotation package="com.company" class="ClassName" method="get*"> // all get accessors

[dmarkov]

@yegor256 please do something about this issue

[yegor256]

@DaveJarvis really good idea, thanks. we will try to implement it soon

[dmarkov]

@prondzyn the task is for you now, follow these guidelines. Don't hesitate to ask any technical questions right here; This task's budget is 30 mins. This is exactly how much will be paid when the problem explained above is solved. See this for more information

[prondzyn]

@dmarkov please assign someone else

[dmarkov]

@dmarkov please assign someone else

@prondzyn -30 points to your rating

[dmarkov]

@dmarkov please assign someone else

@prondzyn I will ask somebody else to pick this up

[dmarkov]

@amihaiemil please proceed, it's yours

[amihaiemil]

@DaveJarvis Refactoring can be done and also a new aspect can be created that uses parameters read form a file, no problem.

But, in the new aspect's constructor, where I try to read the xml file, what do I do if there's an exception or the file is not there (the client chose the annotation option)? I cannot control the AspectJ instantiation process, can I? It will result in a ghost aspect running in the backstage not doing anything, or worse, throwing exceptions.

Do you have any idea about this, maybe I'm missing something? :D

[amihaiemil]

@yegor256 What do you think about it? See my previous comment

[ghost]

what do I do if there's an exception or the file is not there (the client chose the annotation option)?

When documenting the feature:

• Create a simple example that demonstrates exactly how to use the configuration file.
• Make it perfectly clear what will happen if the configuration file is invalid.
• Show how to determine whether the configuration file is valid.

No need to protect developers from themselves.

If it is possible to log a warning that indicates an error with the configuration file (reading, parsing, applying behaviours), that might suffice.

[amihaiemil]

@DaveJarvis Ok, that is one side of the matter. What if no file is configured? How do I stop the Aspect, in its constructor? The AspectJ will call that constructor, the file won't be found (because simply the user did not supply it, he wants just annotations) and then what? I cannot control at runtime what AspectJ does with the Aspects? Will it keep trying to instantiate it and fail, or will instantiate it and run it "as a ghost", not doing anything?

From my point of view, this configuration file wouldn't be optional, but in fact mandatory, in order for the aspect lib to work properly.

[amihaiemil]

@DaveJarvis I'm just trying to clarify everything now, since this is a big topic and I'm not going to implement it all, just a portion of it (i'll start with necessary refactoring and layout) and add what we call "puzzles" (TODOs), which will be solved as further tasks. So you see, this issue here will raise a tree of other issues/tasks (up until the whole new functionality is implemented and unit-tested accross all aspects).

So it wouldn't be cool to realize somewhere in the middle that "hey, this cannot actually be done right" :D

[amihaiemil]

@yegor256 can u also have a look here pls?

[ghost]

What if no file is configured? How do I stop the Aspect, in its constructor?

Would writing a small test case help? (It needn't read a configuration file, merely apply an annotation across a few classes. A stepping stone on the way to reading a configuration file.) I had imagined that the configuration file would be optional; the library should "just work" without external dependencies. I don't know enough about the internals of how Aspects are created to be much more help.

[amihaiemil]

@DaveJarvis I did some prototyping now and I found the following:

1) If there's an exception in the Aspect's constructor, then org.aspectj.lang.NoAspectBoundException is thrown. This exception fails the build, as I expected, since AspectJ cannot handle it.

So once you have an exception in the constructor (like when there is no config file since the client chose to use annotations) there is nothing you can do.

Here is the log. I made the LoggableFromConfig aspect and passed a non-existent (null) InputStream resource: log.txt

2) This is a real stopper: value of the @Around() has to be a constant. So this means best you can do is declare a private static final String field of the Aspect class and pass that there.. but this does not help, since we need to change that value at runtime according to the configs read from the file. With the annotations everything is cool since you hardcode @Around(@Loggable) and it knows what to do, but in the case of a config file, that value needs to be changeable.

Here is snipet of the aspect I wrote, to give you a better understanding

@Aspect
...
    /**
     * Constructor.
     * @throws IOException If the config file cannot be read
     */
    public LoggingFromConfig() throws IOException {
        final JcabiAspectsProperties jap = new JcabiAspectsProperties(
            this.getClass().getResourceAsStream("sdfsdf.xml")
        );
       /* Here you would initialize some class fields with the read config, to pass to @Around annotations, but this is not possible since @Around accepts only constants*/
    }

    /**
     * Log individual methods.
     *
     * <p>Try NOT to change the signature of this method, in order to keep
     * it backward compatible.
     *
     * @param point Joint point
     * @return The result of call
     * @throws Throwable If something goes wrong inside
     */
    @Around("THIS VALUE NEEDS TO BE CHANGEABLE AT RUNTIME")
    public Object wrapMethod(final ProceedingJoinPoint point) throws Throwable {
        // wrapper aspect method here...
        ...
    }

So, from my point of view, this task cannot be implemented. You need those annotations in the code, since they are "flags", they guide the aspect and as we can see from this example, joint points cannot be declared dynamically.

And the annotations already have a default behaviour, do they not? You can annotate your class with @Loggable and you have your methods logged with default values that the annotation declares. E.g. default logging level is INFO

Can you close this ticket if you agree?

Best regards, Mihai

[amihaiemil]

@DaveJarvis ping

[ghost]

If annotations are absolutely required and cannot be used to read the configuration file, then yes, close it. Spring AOP has an XML-driven approach, but doesn't use annotations.

http://www.tutorialspoint.com/spring/schema_based_aop_appoach.htm

[amihaiemil]

@DaveJarvis you close it please, I cannot. And besides, you should close it, that'a how we work :D

[ghost]

Thanks for looking into it. :-)

[dmarkov]

@amihaiemil Much appreciated! 30 mins was added to your account, payment ID AP-8D627772T9427752L, time spent is 192 hours and 49 mins... +30 added to your rating, current score is: +270