adding a test case for MigrateWebMvcTagsToObservationConvention for t…

[nmck257]

…he case where the iterable passed to Tags.and(...) is not an identifier, and partially fixing it, BUT this commit still has a strange test failure with cyclically duplicating http request/response variables

What's changed?

see subject

What's your motivation?

currently, the scenario in the test case yields a classcast exception and breaks the recipe process

Anything in particular you'd like reviewers to focus on?

here's the result of running this test case.

it seems that these lines are getting added again in a second cycle?

        HttpServletRequest request = context.getCarrier();
        HttpServletResponse response = context.getResponse();

test result:

org.opentest4j.AssertionFailedError: [Expected recipe to complete in 1 cycle, but took at least one more cycle. Between the last two executed cycles there were changes to "CustomWebMvcTagsProvider.java"] 
expected: "import io.micrometer.common.KeyValue;
import io.micrometer.common.KeyValues;
import io.micrometer.core.instrument.Tag;
import io.micrometer.core.instrument.Tags;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
import org.springframework.http.server.observation.DefaultServerRequestObservationConvention;
import org.springframework.http.server.observation.ServerRequestObservationContext;

class CustomWebMvcTagsProvider extends DefaultServerRequestObservationConvention {

    Tags staticTags = Tags.of("a", "b", "c", "d");
    Tag staticTag = Tag.of("a", "b");

    @Override
    public KeyValues getHighCardinalityKeyValues(ServerRequestObservationContext context) {
        HttpServletRequest request = context.getCarrier();
        HttpServletResponse response = context.getResponse();
        KeyValues values = super.getHighCardinalityKeyValues(context);

        String customHeader = request.getHeader("X-Custom-Header");
        if (customHeader != null) {
            values.and(KeyValue.of("custom.header", customHeader));
        }
        if (response.getStatus() >= 400) {
            values.and(KeyValue.of("error", "true"));
        }
        values.and(KeyValue.of("a", "b"), KeyValue.of("c", "d"));
        values.and(KeyValue.of("a", "b"), KeyValue.of(staticTag.getKey(), staticTag.getValue()));
        for (Tag tag : staticTags) {
            values.and(KeyValue.of(tag.getKey(), tag.getValue()));
        }
        for (Tag tag : methodTags()) {
            values.and(KeyValue.of(tag.getKey(), tag.getValue()));
        }
        return values;
    }

    private Tags methodTags() {
        return staticTags;
    }
}"

 but was:

"import io.micrometer.common.KeyValue;
import io.micrometer.common.KeyValues;
import io.micrometer.core.instrument.Tag;
import io.micrometer.core.instrument.Tags;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
import org.springframework.http.server.observation.DefaultServerRequestObservationConvention;
import org.springframework.http.server.observation.ServerRequestObservationContext;

class CustomWebMvcTagsProvider extends DefaultServerRequestObservationConvention {

    Tags staticTags = Tags.of("a", "b", "c", "d");
    Tag staticTag = Tag.of("a", "b");

    @Override
    public KeyValues getHighCardinalityKeyValues(ServerRequestObservationContext context) {
        HttpServletRequest request = context.getCarrier();
        HttpServletResponse response = context.getResponse();
        HttpServletRequest request = context.getCarrier();
        HttpServletResponse response = context.getResponse();
        KeyValues values = super.getHighCardinalityKeyValues(context);

        String customHeader = request.getHeader("X-Custom-Header");
        if (customHeader != null) {
            values.and(KeyValue.of("custom.header", customHeader));
        }
        if (response.getStatus() >= 400) {
            values.and(KeyValue.of("error", "true"));
        }
        values.and(KeyValue.of("a", "b"), KeyValue.of("c", "d"));
        values.and(KeyValue.of("a", "b"), KeyValue.of(staticTag.getKey(), staticTag.getValue()));
        for (Tag tag : staticTags) {
            values.and(KeyValue.of(tag.getKey(), tag.getValue()));
        }
        for (Tag tag : methodTags()) {
            values.and(KeyValue.of(tag.getKey(), tag.getValue()));
        }
        return values;
    }

    private Tags methodTags() {
        return staticTags;
    }
}"

Anyone you would like to review specifically?

@Laurens-W
@timtebeek

Have you considered any alternatives or workarounds?

Any additional context

looks like there's some cursor messaging which tries to control against duplicating these lines; not sure why it's breaking. probably simpler for the original developer to look since it was only a week ago :)

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've used the IntelliJ IDEA auto-formatter on affected files

[Laurens-W]

It's making a second cycle because the precondition still matches (due to the DeclaringType of the methodTags method not being updated). I think changing the precondition to org.openrewrite.java.search.FindImplementations would solve it

[nmck257]

yep, passing tests with that change. thanks!
...though, does that imply there's a (known) gap in this recipe where it should ideally update DeclaringType of all the class' declared methods after refactoring to a different superclass?

[Laurens-W]

yep, passing tests with that change. thanks! ...though, does that imply there's a (known) gap in this recipe where it should ideally update DeclaringType of all the class' declared methods after refactoring to a different superclass?

If we want the LST to be 100% correct maybe we should.
What's odd to me is that it isn't preventing the import from being removed.
I'll take a look at the (unrelated) failing test as I've probably introduced that one

[nmck257]

Import removal probably does not consult implicit usages, which feels correct, since we only need imports for types explicitly named in the source code