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

Conversation

sdelamo
Copy link
Contributor

@sdelamo sdelamo commented Jul 10, 2023

No description provided.

@sdelamo sdelamo added the type: bug Something isn't working label Jul 10, 2023
@sonarcloud
Copy link

sonarcloud bot commented Jul 10, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

}

@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

@graemerocher
Copy link
Contributor

It is too late to make a change this big IMO

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

3 participants