Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancements to remove feature flag recipes #14

Open
2 of 6 tasks
timtebeek opened this issue Nov 17, 2023 · 5 comments
Open
2 of 6 tasks

Enhancements to remove feature flag recipes #14

timtebeek opened this issue Nov 17, 2023 · 5 comments
Labels
enhancement New feature or request

Comments

@timtebeek
Copy link
Contributor

timtebeek commented Nov 17, 2023

What problem are you trying to solve?

We identified a couple of possible improvements following the first recipe to removed a feature flag.

Describe the solution you'd like

  • RemoveUnusedLocalVariables does not clean up LDContext when the assignment might have side effects, such as when using the builder. Might need a separate recipe that ignores the potential builder side effect.
  • local variables that become true or false (or String literal) are not inlined (no such recipe)
  • the doAfterVisit recipes now affect the entire SourceFile containing a feature flag, but not other SourceFiles. we might want to limit that further, although it'd be more effort
  • possibly reuse FindFeatureFlag to identify feature keys in more places
  • combine with ReplaceConstant to inline constants using the same feature key
  • unused method arguments are not removed and cascade upwards to callers (no such recipe)

Have you considered any alternatives or workarounds?

Some of these can be separate recipes in for instance rewrite-static-analysis, that we then reuse here.

@timtebeek timtebeek added the enhancement New feature or request label Nov 17, 2023
@timtebeek timtebeek moved this to Backlog in OpenRewrite Nov 20, 2023
@kavitha186
Copy link
Contributor

@timtebeek Can I work on this issue?

@timtebeek
Copy link
Contributor Author

@timtebeek Can I work on this issue?

Hi @kavitha186 yes thanks for your offer to help! Which element specifically would you like to tackle first? A draft PR with a failing test case is typically a great way to start

@kavitha186
Copy link
Contributor

kavitha186 commented Oct 21, 2024

@timtebeek Could you please check my draft PR

I have few questions on attempting to tackle "RemoveUnusedLocalVariables does not clean up LDContext when the assignment might have side effects, such as when using the builder. Might need a separate recipe that ignores the potential builder side effect."

Should I expect all to be cleared with respect to LDContext? eg: if there is any methods used from LDCotext https://launchdarkly.github.io/java-server-sdk/com/launchdarkly/sdk/LDContext.html .

@kavitha186
Copy link
Contributor

@timtebeek Can I take up possibly reuse FindFeatureFlag to identify feature keys in more places

Can you please help me to understand where the feature keys are not found currently with findFeature flag?

@timtebeek
Copy link
Contributor Author

@timtebeek Can I take up possibly reuse FindFeatureFlag to identify feature keys in more places

Can you please help me to understand where the feature keys are not found currently with findFeature flag?

Hi @kavitha186 ; appreciate the continued offer to help out! These two PRs already improved what constants we can detect:

You'll also see that reflected in a test like this one 6f90bd4. Where we currently fail is a slight alteration where the constant is in another class:

    @Test
    void removeWhenFeatureFlagIsAnExternalConstant() {
        rewriteRun(
          spec -> spec.recipe(new RemoveBooleanFlag("com.acme.bank.CustomLaunchDarklyWrapper featureFlagEnabled(String, boolean)", "flag-key-123abc", true)),
          // language=java
          java(
            """
              package com.acme.bank;
              
              public class CustomLaunchDarklyWrapper {
                  public static final String FEATURE_TOGGLE = "flag-key-123abc";
                  public boolean featureFlagEnabled(String key, boolean fallback) {
                      return fallback;
                  }
              }
              """,
            SourceSpec::skip
          ),
          // language=java
          java(
            """
              import com.acme.bank.CustomLaunchDarklyWrapper;
              import static com.acme.bank.CustomLaunchDarklyWrapper.FEATURE_TOGGLE;
              class Foo {
              
                  private CustomLaunchDarklyWrapper wrapper = new CustomLaunchDarklyWrapper();
                  void bar() {
                      if (wrapper.featureFlagEnabled(FEATURE_TOGGLE, false)) {
                          // Application code to show the feature
                          System.out.println("Feature is on");
                      }
                      else {
                        // The code to run if the feature is off
                          System.out.println("Feature is off");
                      }
                  }
              }
              """,
            """
              class Foo {
                  void bar() {
                      // Application code to show the feature
                      System.out.println("Feature is on");
                  }
              }
              """
          )
        );
    }

I'd expect that to be fairly common, and worth supporting, as folks might very well extract those feature keys to a shared constant. Perhaps this is something we can solve in ConstantFold, but I don't know the full details there to know if that's indeed possible.

We did previously use this other method to determine the literal value for a given constant; that might work here once we've determined that the first argument is a constant, not a literal.

private boolean isFeatureKey(Expression firstArgument) {
return CursorUtil.findCursorForTree(getCursor(), firstArgument)
.bind(c -> ConstantFold.findConstantLiteralValue(c, String.class))
.map(featureKey::equals)
.orSome(false);
}

So it's a bit up in the air how to approach finding more such feature flags usages, but a welcome addition to phase them out anywhere regardless of how they are used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Backlog
Development

No branches or pull requests

2 participants