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.

Sign up for GitHub

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

Jump to bottom

M3 #441

Merged
merged 15 commits into from
May 12, 2023
Merged

M3 #441

merged 15 commits into from
May 12, 2023

Conversation

sdelamo
Copy link
Contributor

@sdelamo sdelamo commented May 11, 2023

No description provided.

@sdelamo
Copy link
Contributor Author

sdelamo commented May 11, 2023

@wetted there is a breaking change. Please investigate what has changed in core and fix it in servlet.

/Users/sdelamo/github/micronaut-projects/micronaut-servlet/servlet-core/src/main/java/io/micronaut/servlet/http/ServletBodyBinder.java:70: error: getAnnotationType() in ServletBodyBinder cannot override getAnnotationType() in DefaultBodyAnnotationBinder
   public Class<Body> getAnnotationType() {
                      ^
 overridden method is final

@sdelamo sdelamo added the type: dependency-upgrade Upgrade a dependency label May 11, 2023
@graemerocher
Copy link
Contributor

Seems like this method can just be deleted https://github.com/micronaut-projects/micronaut-servlet/blob/master/servlet-core/src/main/java/io/micronaut/servlet/http/ServletBodyBinder.java#L70-L72

@wetted
Copy link
Contributor

wetted commented May 11, 2023 •
edited
Loading

Seems like this method can just be deleted https://github.com/micronaut-projects/micronaut-servlet/blob/master/servlet-core/src/main/java/io/micronaut/servlet/http/ServletBodyBinder.java#L70-L72

Yeah, it can. The bigger problems are that DefaultBodyAnnotationBinder.bind(ArgumentConversionContext<T> context, HttpRequest<?> source) in core is now final and the following override it (now a compile error).

@wetted wetted marked this pull request as ready for review May 11, 2023 20:34
sdelamo
sdelamo commented May 12, 2023
View reviewed changes
...r-undertow/src/test/groovy/io/micronaut/servlet/undertow/UndertowParameterBindingSpec.groovy
@@ -91,7 +91,7 @@ class UndertowParameterBindingSpec extends Specification {
expect:
response.status() == HttpStatus.BAD_REQUEST
response.body().contains('Failed to convert argument')
response.body().contains('Expected one integer, but got array of multiple value')
response.body().contains('Expected one string, but got array of multiple value')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this change expected?

Copy link
Member

Choose a reason for hiding this comment

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

yes the old message was wrong

@sdelamo sdelamo requested review from yawkat and dstepanov May 12, 2023 10:12
yawkat
yawkat reviewed May 12, 2023
View reviewed changes
servlet-core/src/main/java/io/micronaut/servlet/http/ServletBodyBinder.java
@@ -166,7 +171,7 @@ public List<ConversionError> getConversionErrors() {
}

}
return super.bind(context, source);
return defaultBodyAnnotationBinder.bind(context, source);
Copy link
Member

Choose a reason for hiding this comment

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

would be nice to eventually replace this method entirely

@sonarcloud
Copy link

sonarcloud bot commented May 12, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

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

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@wetted wetted requested a review from yawkat May 12, 2023 15:27
@sdelamo sdelamo merged commit c24461d into master May 12, 2023
@sdelamo sdelamo deleted the m3 branch May 12, 2023 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wetted wetted wetted left review comments

@dstepanov dstepanov dstepanov approved these changes

@yawkat yawkat Awaiting requested review from yawkat

Assignees

@wetted wetted

Labels
type: dependency-upgrade Upgrade a dependency
Projects
No open projects
4.0.0 Release
Status: Done
4.0.0-M3 Release
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sdelamo @graemerocher @wetted @dstepanov @yawkat