-
Notifications
You must be signed in to change notification settings - Fork 992
Support type-use @Nullable annotation in annotated service
#6457
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
Changes from all commits
469a613
8e9285d
5515e0f
eed6b4f
d9809aa
ca1e429
abfe6ac
8d850d9
c016834
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,6 +68,11 @@ public String defaultValue(@Param @Default("unspecified") String value) { | |
| public String optional(@Param Optional<String> value) { | ||
| return value.orElse("unspecified"); | ||
| } | ||
|
|
||
| @Get("/type_use_nullable") | ||
| public String typeUseNullable(@Param @org.jspecify.annotations.Nullable String value) { | ||
| return nullable(value); | ||
| } | ||
| }); | ||
|
|
||
| sb.annotatedService("/headers", new Object() { | ||
|
|
@@ -98,20 +103,29 @@ public String defaultValue(@Header @Default("unspecified") String value) { | |
| public String optional(@Header Optional<String> value) { | ||
| return value.orElse("unspecified"); | ||
| } | ||
|
|
||
| @Get("/type_use_nullable") | ||
| public String typeUseNullable(@Header @org.jspecify.annotations.Nullable String value) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you also add a test with this?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We may use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added test for nullable response Also I tried to test with @Post("/foo")
@ProducesJson
public Map<String, Object> foo(@RequestObject Map<String, Object> map) {
return map;
}
@Post("/bar")
@ProducesJson
public Map<String, Object> bar(@RequestObject @Nullable Map<String, Object> map) {
if (map == null) {
return Map.of("this is", "null");
}
return map;
}
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think it shouldn't throw an exception if it's annotated with the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree the behavior sounds reasonable - I'm not sure whether this is enforceable for all cases though. I have no objection if for Jackson's case where a literal Having said this, I'm fine with going ahead with merging this PR and handling this separately as they seem to be separate issues
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree. We don't have to deal with this in this PR. Let me merge this. 😉 |
||
| return nullable(value); | ||
| } | ||
| }); | ||
| } | ||
| }; | ||
|
|
||
| @ParameterizedTest | ||
| @CsvSource({ "/nullable", "/jsr305_nullable", "/other_nullable", "/default", "/optional" }) | ||
| @CsvSource({ | ||
| "/nullable", "/jsr305_nullable", "/other_nullable", "/default", "/optional", "type_use_nullable" | ||
| }) | ||
| void params(String path) { | ||
| final BlockingWebClient client = BlockingWebClient.of(server.httpUri().resolve("/params")); | ||
| assertThat(client.get(path + "?value=foo").contentUtf8()).isEqualTo("foo"); | ||
| assertThat(client.get(path).contentUtf8()).isEqualTo("unspecified"); | ||
| } | ||
|
|
||
| @ParameterizedTest | ||
| @CsvSource({ "/nullable", "/jsr305_nullable", "/other_nullable", "/default", "/optional" }) | ||
| @CsvSource({ | ||
| "/nullable", "/jsr305_nullable", "/other_nullable", "/default", "/optional", "/type_use_nullable" | ||
| }) | ||
| void headers(String path) { | ||
| final BlockingWebClient client = BlockingWebClient.of(server.httpUri().resolve("/headers")); | ||
| assertThat(client.execute(RequestHeaders.of(HttpMethod.GET, path, "value", "foo")) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| /* | ||
| * Copyright 2025 LY Corporation | ||
| * | ||
| * LY Corporation licenses this file to you under the Apache License, | ||
| * version 2.0 (the "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at: | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||
| * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the | ||
| * License for the specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| package com.linecorp.armeria.internal.server.annotation; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| import org.junit.jupiter.api.extension.RegisterExtension; | ||
| import org.junit.jupiter.params.ParameterizedTest; | ||
| import org.junit.jupiter.params.provider.CsvSource; | ||
|
|
||
| import com.linecorp.armeria.client.BlockingWebClient; | ||
| import com.linecorp.armeria.server.ServerBuilder; | ||
| import com.linecorp.armeria.server.annotation.Get; | ||
| import com.linecorp.armeria.testing.junit5.server.ServerExtension; | ||
|
|
||
| class AnnotatedServiceNullableResponseTest { | ||
|
|
||
| @RegisterExtension | ||
| static final ServerExtension server = new ServerExtension() { | ||
| @Override | ||
| protected void configure(ServerBuilder sb) throws Exception { | ||
| sb.annotatedService("/response", new Object() { | ||
| @SuppressWarnings("checkstyle:LegacyNullableAnnotation") | ||
| @Get("/jsr305_nullable") | ||
| @javax.annotation.Nullable | ||
| public String jsr305Nullable() { | ||
| return null; | ||
| } | ||
|
|
||
| @Get("/type_use_nullable") | ||
| public @org.jspecify.annotations.Nullable String typeUseNullable() { | ||
| return null; | ||
| } | ||
| }); | ||
| } | ||
| }; | ||
|
|
||
| @ParameterizedTest | ||
| @CsvSource({ | ||
| "/jsr305_nullable", "/type_use_nullable" | ||
| }) | ||
| void response(String path) { | ||
| final BlockingWebClient client = BlockingWebClient.of(server.httpUri().resolve("/response")); | ||
| assertThat(client.get(path).contentUtf8()).isEqualTo(""); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add examples so that readers can distinguish them easily? e.g.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I added comment
ca1e429(#6457)