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

imp: MethodRoute stringValues behave as getValues #9552

Open
wants to merge 1 commit into
base: 4.0.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions core/src/main/java/io/micronaut/core/util/ArrayUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,11 @@ public final class ArrayUtils {
@UsedByGeneratedCode
public static final Object[] EMPTY_OBJECT_ARRAY = new Object[0];

/**
* An empty String array.
*/
public static final String[] EMPTY_STRING_ARRAY = new String[0];

/**
* An empty boolean array.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -322,15 +322,18 @@ public String[] stringValues(@NonNull Class<? extends Annotation> annotation, @N
@NonNull
@Override
public String[] stringValues(@NonNull String annotation, @NonNull String member) {
String[] values = hierarchy[0].stringValues(annotation, member);
for (int i = 1; i < hierarchy.length; i++) {
AnnotationMetadata annotationMetadata = hierarchy[i];
final String[] moreValues = annotationMetadata.stringValues(annotation, member);
if (ArrayUtils.isNotEmpty(moreValues)) {
values = ArrayUtils.concat(values, moreValues);
return firstNotEmptyStringValuesInHierarchy(annotation, member);
}

@NonNull
private String[] firstNotEmptyStringValuesInHierarchy(@NonNull String annotation, @NonNull String member) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the previous code is more correct, all the values are aggregated.

Copy link
Contributor Author

@sdelamo sdelamo Jul 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note that with:

     @Controller("/example")
    @RolesAllowed("isAuthenticated()")
    static class ExampleController {

        @Get("/admin")
        @RolesAllowed({"ROLE_ADMIN", "ROLE_X"})
        void  admin(HttpRequest<?> request) {
        }

old stringValues returns "ROLE_ADMIN", "ROLE_X", "isAuthenticated()" while getValues returns "ROLE_ADMIN", "ROLE_X". The latter looks correct to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be some behaviour inconsistency here, but this change has the potential to break a lot of things. I don't think we should be merging this 2 days before GA

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am ok not merging it. Do you want me to target 5.0.x as the PR base?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sdelamo You might need to modify the code that reads the values to only take method annotation metadata

for (AnnotationMetadata annotationMetadata : hierarchy) {
String[] values = annotationMetadata.stringValues(annotation, member);
if (ArrayUtils.isNotEmpty(values)) {
return values;
}
}
return values;
return ArrayUtils.EMPTY_STRING_ARRAY;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
package io.micronaut.docs.web.router.version;

import com.fasterxml.jackson.annotation.JsonInclude;
import io.micronaut.context.annotation.Property;
import io.micronaut.core.annotation.Introspected;
import io.micronaut.http.HttpAttributes;
import io.micronaut.http.HttpRequest;
import io.micronaut.http.HttpResponse;
import io.micronaut.http.annotation.Controller;
import io.micronaut.http.annotation.Get;
import io.micronaut.http.client.BlockingHttpClient;
import io.micronaut.http.client.HttpClient;
import io.micronaut.http.client.annotation.Client;
import io.micronaut.http.uri.UriBuilder;
import io.micronaut.test.extensions.junit5.annotation.MicronautTest;
import io.micronaut.web.router.MethodBasedRouteMatch;
import io.micronaut.web.router.RouteMatch;
import jakarta.annotation.security.RolesAllowed;
import org.junit.jupiter.api.Test;

import java.net.URI;
import java.util.*;

import static org.junit.jupiter.api.Assertions.*;

@Property(name = "spec.name", value = "MethodRouteStringValuesTest")
@MicronautTest
class MethodRouteStringValuesTest {

@Test
void methodRouteStringValuesMatchesGetValue(@Client("/") HttpClient httpClient) {
BlockingHttpClient client = httpClient.toBlocking();
assertScenario(client, UriBuilder.of("/example").path("user").build(), Collections.singletonList("isAuthenticated()"));
assertScenario(client, UriBuilder.of("/example").path("admin").build(), Arrays.asList("ROLE_ADMIN", "ROLE_X"));
assertScenario(client, UriBuilder.of("/foobarxxx").path("admin").build(), Collections.singletonList("ROLE_X"));
assertScenario(client, UriBuilder.of("/foobarxxx").path("user").build(), Collections.emptyList()); // RolesAllowed is not inherited
}

private void assertScenario(BlockingHttpClient client, URI uri, List<String> expectedList) {
HttpResponse<StringValuesAndGetValues> response = client.exchange(HttpRequest.GET(uri),
StringValuesAndGetValues.class);
StringValuesAndGetValues m = response.getBody().orElse(null);
assertNotNull(m);
assertNotNull(m.getValueString);
assertNotNull(m.stringValue);
assertEquals(expectedList.size(), m.getValueString.size());
assertEquals(expectedList.size(), m.stringValue.size());
for (String expected : expectedList) {
assertTrue(m.getValueString.contains(expected));
assertTrue(m.stringValue.contains(expected));
}
}

@Property(name = "spec.name", value = "MethodRouteStringValuesTest")
@Controller("/example")
@RolesAllowed("isAuthenticated()")
static class ExampleController {

@Get("/admin")
@RolesAllowed({"ROLE_ADMIN", "ROLE_X"})
StringValuesAndGetValues admin(HttpRequest<?> request) {
return stringValuesAndGetValues(request);
}

@Get("/user")
StringValuesAndGetValues user(HttpRequest<?> request) {
return stringValuesAndGetValues(request);
}
}

interface ExampleInterface {
@Get("/admin")
@RolesAllowed("ROLE_ADMIN")
StringValuesAndGetValues admin(HttpRequest<?> request);

@Get("/user")
@RolesAllowed("ROLE_USER")
StringValuesAndGetValues user(HttpRequest<?> request);
}

@Controller("/foobarxxx")
@Property(name = "spec.name", value = "MethodRouteStringValuesTest")
static class ExampleControllerWithInterface implements ExampleInterface {
@RolesAllowed("ROLE_X")
@Override
public StringValuesAndGetValues admin(HttpRequest<?> request) {
return stringValuesAndGetValues(request);
}

@Override
public StringValuesAndGetValues user(HttpRequest<?> request) {
return stringValuesAndGetValues(request);
}
}

private static StringValuesAndGetValues stringValuesAndGetValues(HttpRequest<?> request) {
StringValuesAndGetValues defaultResult = new StringValuesAndGetValues(Collections.emptyList(), Collections.emptyList());
return methodBasedRouteMatch(request).map(methodRoute ->
new StringValuesAndGetValues(methodRoute.getValue(RolesAllowed.class, String[].class).map(Arrays::asList).orElse(Collections.emptyList()),
Arrays.asList(methodRoute.stringValues(RolesAllowed.class))))
.orElse(defaultResult);
}

private static Optional<MethodBasedRouteMatch> methodBasedRouteMatch(HttpRequest<?> request) {
RouteMatch<?> routeMatch = request.getAttribute(HttpAttributes.ROUTE_MATCH, RouteMatch.class).orElse(null);
if (routeMatch == null) {
return Optional.empty();
}
if (routeMatch instanceof MethodBasedRouteMatch methodRoute) {
return Optional.of(methodRoute);
}

return Optional.empty();
}

@JsonInclude
@Introspected
record StringValuesAndGetValues(List<String> getValueString, List<String> stringValue) {
}
}