openrewrite / rewrite-logging-frameworks

OpenRewrite recipes for assisting with Java logging migration tasks.
Apache License 2.0
25 stars 20 forks source link

Create a custom recipe to add loglevel guards #111

Closed bhavyasripenumarthi closed 8 months ago

bhavyasripenumarthi commented 1 year ago

What problem are you trying to solve?

Create a custom recipe to add log level guards.

What precondition(s) should be checked before applying this recipe?

check if there is a log statement available, if true then surround with a guard.

Describe the situation before applying the recipe

class A {
    void foo(String bar) {
        log.info("This is info");
    }
}

Describe the situation after applying the recipe

class A {
    void foo(String bar) {
        if(log.isInfoEnabled()) {
           log.info("This is info");
       }

    }
}

Have you considered any alternatives or workarounds?

I am facing an issue while constructing the code block snippet using java parser.

Any additional context

I want to use this, inorder to get rid of my sonarqube issues.

Are you interested in contributing this recipe to OpenRewrite?

yes

timtebeek commented 1 year ago

Hi @bhavyasripenumarthi, thank you for proposing this recipe & offer to contribute! I've moved it to the appropriate module; feel free to ask here, or drop by our Slack, if you have any questions regarding the implementation.

bhavyasripenumarthi commented 1 year ago

Hi, I tried writing some code for the recipe but facing an issue while creating the if block snippet. I will attach the code here for your reference. This is the visitor method. private static class LogLevelGuardsVisitor extends JavaIsoVisitor { public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) { J.MethodInvocation invocation = super.visitMethodInvocation(method, ctx);

        // Check if the method invocation is a log statement (e.g., logger.info(...), logger.error(...), etc.)
        if (isLogInvocation(invocation)) {
            J.MethodInvocation guardedInvocation = addLogLevelGuard(invocation);
            return guardedInvocation;
        }

        return invocation;
    }

    private boolean isLogInvocation(J.MethodInvocation methodInvocation) {
        String methodName = methodInvocation.getSimpleName();
        String[] logLevels = {"trace", "debug", "info", "warn", "error"};
        for (String level : logLevels) {
            if (methodName.equalsIgnoreCase(level)) {
                return true;
            }
        }
        return false;
    }

    private J.MethodInvocation addLogLevelGuard(J.MethodInvocation logInvocation) {
        String logLevel = logInvocation.getSimpleName().toLowerCase();
        Expression condition = JavaParser.buildSnippet(
                String.format("if (logger.%sEnabled())", logLevel)
        ).get(0);

        J.If ifStatement = new J.If(
                null,
                condition,
                J.Block.createEmptyBlock().getMarkers(JavaParser.buildSnippet("{" + logInvocation.printTrimmed() + "}").get(0)),
                null
        );

        return logInvocation.withName(logInvocation.getName().withName("debug")).withArguments(ifStatement);
    }

}

Can someone please help me in fixing the addLogLevelGuard() method. as I having issue with the JavaParser.buildSnippet() method.

timtebeek commented 1 year ago

You'll want to have a look at JavaTemplate instead of JavaParser. That has an API to actually produce new LST elements that you need.

bhavyasripenumarthi commented 1 year ago

I tried with JavaTemplate too, but couldnt able to achieve it, as I am not sure on how to build a template for it. Can you please help me if there is any sample on how to achieve it.

            JavaTemplate logTemplate = JavaTemplate.builder(
                    "@{{logClass}}.log(Level.{{level}}, {{any}})"
            ).build();
            JavaTemplate guardTemplate = JavaTemplate.builder(
                    "if (@{{logClass}}.isLoggable(Level.{{level}})) {\n" +
                            " @{{logClass}}.log(Level.{{level}}, {{any}});\n" +
                            "}"
            ).build();
            JavaTemplate template = left == null ? logTemplate : guardTemplate;
            return template.withParameters("logClass",left,"level",level,"any",args.toArray(new J[0]).apply(method,getCursor()));
        }
timtebeek commented 1 year ago

The typed substitution parameters are quite different from what I see you using; https://docs.openrewrite.org/concepts-explanations/javatemplate#typed-substitution-indicators

Object o = #{any()}; boolean b = #{any(boolean)}; foo.acceptsACollection(#{any(java.util.Collection)}); String[] = #{anyArray(java.lang.String)};

Be sure to replace those {{}} elements that you have with the #{} form that we expect.

bhavyasripenumarthi commented 1 year ago

https://docs.openrewrite.org/concepts-explanations/javatemplate#typed-substitution-indicators

Okay, May I know if the approach I am doing is correct or wrong, beacuse I cant able to generate the code block snippet and facing issue. withParameters() method is not available too, so how can I achieve that. The issue is with the return statement.

timtebeek commented 1 year ago

Instead of template.withParameters you'll want to use template.apply as also seen in the docs. Or open a draft pull request such that it's easier for me to provide you with feedback and push code suggestions.

bhavyasripenumarthi commented 1 year ago

Hi, Finally, My recipe class looks like this: package org.example;

import org.openrewrite.Cursor; import org.openrewrite.ExecutionContext; import org.openrewrite.Recipe; import org.openrewrite.TreeVisitor; import org.openrewrite.java.JavaTemplate; import org.openrewrite.java.JavaVisitor; import org.openrewrite.java.tree.J;

public class LogLevelGuardVisitor extends Recipe { private static final String LOGGER_NAME = "logger"; @Override public String getDisplayName() { return "Add Log Level Guards"; }

@Override
public String getDescription() {
    return "Add Guards";
}

public TreeVisitor<?, ExecutionContext> getSingleSourceApplicableTest() {
    return new JavaVisitor<ExecutionContext>() {
        @Override
        public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) {
            if (method.getSelect() instanceof J.Identifier &&
                    ((J.Identifier) method.getSelect()).getSimpleName().equals(LOGGER_NAME) &&
                    method.getSimpleName().equals("debug")) {
                return method;
            }
            return super.visitMethodInvocation(method, ctx);
        }
    };
}
@Override
public JavaVisitor<ExecutionContext> getVisitor() {
    Cursor cursor = this.getVisitor().getCursor();
    JavaTemplate logLevelGuardTemplate = JavaTemplate.builder("${loggerName}.isDebugEnabled() ? ${cursor} : null")
            .imports("org.slf4j.Logger")
            .build();

    return new JavaVisitor<ExecutionContext>() {
        @Override
        public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) {
            if (method.getSelect() instanceof J.Identifier &&
                    ((J.Identifier) method.getSelect()).getSimpleName().equals(LOGGER_NAME) &&
                    method.getSimpleName().equals("debug")) {
                return logLevelGuardTemplate.apply(getCursor(),method.getCoordinates().replace(),method.getArguments().get(0));

// .withTemplateParameter("loggerName", method.getSelect()) // .withCursor(method); } return super.visitMethodInvocation(method, ctx); } }; } }

and the build.gradle file is the. code below plugins{ id'org.openrewrite.rewrite' version'6.1.15'

}

repositories{ mavenCentral() }

rewrite{ // activeRecipe( // "org.openrewrite.java.logging.SystemOutToLogging", // "io.moderne.builder.YourRecipe") // //configFile = project.getRootProject().file("rewrite.yml") activeRecipe( "org.example.LogLevelGuardVisitor")

} sourceSets { main { java { srcDirs = ['src/main/java'] } } } dependencies{ rewrite("org.openrewrite.recipe:rewrite-logging-frameworks:2.0.2") implementation(platform("org.openrewrite.recipe:rewrite-recipe-bom:2.0.7")) implementation 'org.openrewrite:rewrite-core:8.1.10' runtimeOnly 'org.openrewrite:rewrite-java:8.1.10' implementation 'org.openrewrite.recipe:rewrite-logging-frameworks:2.0.2' implementation("org.openrewrite:rewrite-java") runtimeOnly("org.openrewrite:rewrite-java-17") annotationProcessor("org.projectlombok:lombok:latest.release") testImplementation("org.openrewrite:rewrite-test") testImplementation("org.junit.jupiter:junit-jupiter-api:latest.release") testImplementation("org.junit.jupiter:junit-jupiter-params:latest.release") testRuntimeOnly("org.junit.jupiter:junit-jupiter-engine:latest.release") testImplementation group: 'org.slf4j', name: 'slf4j-simple', version: '1.6.1' implementation group: 'org.slf4j', name: 'slf4j-api', version: '2.0.7' }

Now I am unable to run the recipe. I cant push the code to the repo due. to some constraints so can you please help me with this.

timtebeek commented 1 year ago

Hi @bhavyasripenumarthi ; You can't push directly to this repository, but you should be able to create a fork that will allow you to push changes there, and then open a pull request here. The steps are:

  1. Create a fork https://github.com/openrewrite/rewrite-logging-frameworks/fork
  2. Clone your fork; this will create bhavyasripenumarthi/rewrite-logging-frameworks
  3. Create and push the changes in your fork
  4. Go to https://github.com/bhavyasripenumarthi/rewrite-logging-frameworks and follow the prompts to create a draft pull request

This flow will make it such that you get credited for any code changes, and makes it easier for us to review and provide you with feedback. There's some more instructions on this flow if you aren't already familiar here: https://docs.github.com/en/get-started/quickstart/fork-a-repo

bhavyasripenumarthi commented 1 year ago

Ok, will clone and push the code.

bhavyasripenumarthi commented 1 year ago

hi, I pushed the code to my local repo and made it public. Can you please check there. Attached the link below. Also, I am not able to run the recipe, it was showing me recipes not found. I did publish to maven local too. Since, once after I test this I will add make PR for the rewrite-logging-frameworks repo.

Recipe is not getting applied to any class. No changes are reflected Recipe Class https://github.com/bhavyasripenumarthi/recipe-addlogger/blob/main/src/main/java/org/example/AddLogLevelGuard.java

timtebeek commented 1 year ago

Hi @bhavyasripenumarthi ; It really works best if you clone rewrite-logging-frameworks and add your recipe in your cloned repository. That way we can rule out any other issues quickly, without having to spend much time debugging a temporary project.

On a fork you will also be able to add a unit test for your recipe based on the examples already available in this repository, and I would be able to add small fixes to get you going.

bhavyasripenumarthi commented 1 year ago

Created the PR: https://github.com/openrewrite/rewrite-logging-frameworks/pull/112

timtebeek commented 8 months ago

Looking at the current implementation in #112 I have quite some concerns about common cases that make me doubt we should proceed where

  1. there's already a matching logging guard
  2. there's already a mismatched logging guard
  3. there's multiple the same log level statements
  4. there's multiple different intermixed log level statements; say info, error, info, with guard configured for info.
  5. the log statements are in a method, with a guard on calling that method

Covering all the above scenarios correctly will be complex to get right, yet any imperfect code changes would undermine trust in the automated code changes. I therefor think we're better off not making changes just yet until we're confident we can correctly scope these changes as needed, while still adding value to the code base.

Closing this issue for now, but will keep the comments open for feedback below in case anyone has any promising ideas.