Skip to content

Commit 96f7917

Browse files
malczuuumalczuuu
committed
Fix minor issues with ProblemBuilder
- add null-checks of builder methods that convert String to URI - update ProblemBuilderImpl#status(ProblemStatus), so it updates automatically title only if it was null or was implicitly assigned with ProblemStatus
1 parent cacbd51 commit 96f7917

File tree

5 files changed

+484
-3
lines changed

5 files changed

+484
-3
lines changed

‎build.gradle.kts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import com.diffplug.spotless.LineEnding
22

33
plugins {
44
id("java-library")
5+
id("jacoco")
56
id("maven-publish")
67
id("signing")
78
id("com.diffplug.spotless").version("8.0.0")
@@ -145,3 +146,9 @@ tasks.withType<Javadoc>().configureEach {
145146
tasks.withType<Test>().configureEach {
146147
useJUnitPlatform()
147148
}
149+
tasks.test {
150+
finalizedBy(tasks.jacocoTestReport) // report is always generated after tests run
151+
}
152+
tasks.jacocoTestReport {
153+
dependsOn(tasks.test) // tests are required to run before generating the report
154+
}

‎src/main/java/io/github/malczuuu/problem4j/core/ProblemBuilderImpl.java‎

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,29 @@
1111
final class ProblemBuilderImpl implements ProblemBuilder {
1212

1313
private URI type;
14+
1415
private String title;
16+
1517
private int status = 0;
18+
1619
private String detail;
20+
1721
private URI instance;
22+
1823
private final Map<String, Object> extensions = new LinkedHashMap<>();
1924

25+
/**
26+
* Snapshot of the {@link ProblemStatus} used to set the status code. This is used to determine if
27+
* the title should be updated when the status is set with {@link #status(ProblemStatus)}.
28+
*/
29+
private ProblemStatus statusSnapshot;
30+
31+
/**
32+
* Flag indicating if the title was manually set by the user. This helps to determine if the title
33+
* should be updated when the status is set with {@link #status(ProblemStatus)}.
34+
*/
35+
private boolean statusTitleOverwrittenManually;
36+
2037
ProblemBuilderImpl() {}
2138

2239
@Override
@@ -27,32 +44,46 @@ public ProblemBuilder type(URI type) {
2744

2845
@Override
2946
public ProblemBuilder type(String type) {
30-
return type(URI.create(type));
47+
return type != null ? type(URI.create(type)) : this;
3148
}
3249

3350
@Override
3451
public ProblemBuilder title(String title) {
3552
this.title = title;
53+
this.statusTitleOverwrittenManually = true;
3654
return this;
3755
}
3856

3957
@Override
4058
public ProblemBuilder status(int status) {
4159
this.status = status;
60+
this.statusSnapshot = null;
4261
return this;
4362
}
4463

4564
@Override
4665
public ProblemBuilder status(ProblemStatus status) {
4766
if (status != null) {
48-
if (title == null) {
67+
if (isShouldUpdateTitle()) {
4968
title = status.getTitle();
5069
}
70+
5171
this.status = status.getStatus();
72+
this.statusSnapshot = status;
5273
}
5374
return this;
5475
}
5576

77+
private boolean isShouldUpdateTitle() {
78+
if (title == null) {
79+
return true;
80+
}
81+
if (statusTitleOverwrittenManually) {
82+
return false;
83+
}
84+
return statusSnapshot != null && Objects.equals(statusSnapshot.getTitle(), title);
85+
}
86+
5687
@Override
5788
public ProblemBuilder detail(String detail) {
5889
this.detail = detail;
@@ -67,7 +98,7 @@ public ProblemBuilder instance(URI instance) {
6798

6899
@Override
69100
public ProblemBuilder instance(String instance) {
70-
return instance(URI.create(instance));
101+
return instance != null ? instance(URI.create(instance)) : this;
71102
}
72103

73104
@Override

‎src/test/java/io/github/malczuuu/problem4j/core/JsonEscapeTests.java‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,4 +25,39 @@ void shouldEscapeSpecialCharactersWithSlash(String given, String expected, Strin
2525
.withFailMessage(name + " (\"" + expected + "\") failed with \"" + result + "\"")
2626
.isEqualTo(expected);
2727
}
28+
29+
@ParameterizedTest
30+
@CsvSource({
31+
"'\\u0000','control character NUL'",
32+
"'\\u001F','control character US'",
33+
"'\\u007F','delete character'",
34+
"'\\u0080','extended ASCII - padding test'",
35+
"'\\u009F','extended ASCII end'",
36+
"'\\u2000','Unicode space'",
37+
"'\\u20FF','Unicode dingbat'",
38+
})
39+
void shouldEscapeWithHexRepresentation(String expected, String name) {
40+
char given = (char) Integer.parseInt(expected.substring(2), 16);
41+
String result = JsonEscape.escape(String.valueOf(given));
42+
43+
assertThat(result)
44+
.withFailMessage(name + " (\"" + expected + "\") failed with \"" + result + "\"")
45+
.isEqualTo(expected);
46+
}
47+
48+
@ParameterizedTest
49+
@CsvSource({
50+
"'\u0020','regular space'",
51+
"'A','regular letter'",
52+
"'1','digit'",
53+
"'က','unicode outside hex range (Myanmar)'",
54+
"'ぁ','unicode outside hex range (Hiragana)'",
55+
})
56+
void shouldNotEscapeRegularCharacters(String given, String name) {
57+
String result = JsonEscape.escape(given);
58+
59+
assertThat(result)
60+
.withFailMessage(name + " (\"" + given + "\") was escaped to \"" + result + "\"")
61+
.isEqualTo(given);
62+
}
2863
}

‎src/test/java/io/github/malczuuu/problem4j/core/ProblemBuilderImplTests.java‎

Lines changed: 188 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,16 @@
22

33
import static org.assertj.core.api.Assertions.assertThat;
44

5+
import java.net.URI;
56
import java.util.Arrays;
67
import java.util.Collection;
8+
import java.util.Collections;
79
import java.util.HashMap;
810
import java.util.Map;
911
import org.junit.jupiter.api.Test;
12+
import org.junit.jupiter.params.ParameterizedTest;
13+
import org.junit.jupiter.params.provider.NullSource;
14+
import org.junit.jupiter.params.provider.ValueSource;
1015

1116
class ProblemBuilderImplTests {
1217

@@ -18,6 +23,48 @@ void givenNullProblemStatus_shouldNotSetTitleOrStatus() {
1823
assertThat(problem.getTitle()).isNull();
1924
}
2025

26+
@Test
27+
void givenProblemStatusWithNoTitle_shouldSetBothStatusAndTitle() {
28+
Problem problem = Problem.builder().status(ProblemStatus.BAD_REQUEST).build();
29+
30+
assertThat(problem.getStatus()).isEqualTo(400);
31+
assertThat(problem.getTitle()).isEqualTo("Bad Request");
32+
}
33+
34+
@Test
35+
void givenProblemStatusWithCustomTitle_shouldNotOverrideTitle() {
36+
Problem problem =
37+
Problem.builder().title("Custom Title").status(ProblemStatus.BAD_REQUEST).build();
38+
39+
assertThat(problem.getStatus()).isEqualTo(400);
40+
assertThat(problem.getTitle()).isEqualTo("Custom Title");
41+
}
42+
43+
@Test
44+
void givenSecondProblemStatusWithMatchingTitle_shouldUpdateTitle() {
45+
Problem problem =
46+
Problem.builder().status(ProblemStatus.BAD_REQUEST).status(ProblemStatus.NOT_FOUND).build();
47+
48+
assertThat(problem.getStatus()).isEqualTo(404);
49+
assertThat(problem.getTitle()).isEqualTo("Not Found");
50+
}
51+
52+
@Test
53+
void givenNumericStatusAfterProblemStatus_shouldNotAffectTitle() {
54+
Problem problem = Problem.builder().status(ProblemStatus.BAD_REQUEST).status(404).build();
55+
56+
assertThat(problem.getStatus()).isEqualTo(404);
57+
assertThat(problem.getTitle()).isEqualTo("Bad Request");
58+
}
59+
60+
@Test
61+
void givenProblemStatusAfterNumericStatus_shouldSetBothStatusAndTitle() {
62+
Problem problem = Problem.builder().status(404).status(ProblemStatus.BAD_REQUEST).build();
63+
64+
assertThat(problem.getStatus()).isEqualTo(400);
65+
assertThat(problem.getTitle()).isEqualTo("Bad Request");
66+
}
67+
2168
@Test
2269
void givenNullNameExtension_shouldIgnoreIt() {
2370
Problem problem = Problem.builder().extension(null, "value").build();
@@ -78,4 +125,145 @@ void givenCollectionWithNullElement_shouldIgnoreNullElement() {
78125

79126
assertThat(problem.getExtensions()).containsExactlyInAnyOrder("x", "y");
80127
}
128+
129+
@Test
130+
void givenNullUriType_shouldIgnoreIt() {
131+
Problem problem = Problem.builder().type((URI) null).build();
132+
133+
assertThat(problem.getType()).isEqualTo(Problem.BLANK_TYPE);
134+
}
135+
136+
@Test
137+
void givenNullStringType_shouldIgnoreIt() {
138+
Problem problem = Problem.builder().type((String) null).build();
139+
140+
assertThat(problem.getType()).isEqualTo(Problem.BLANK_TYPE);
141+
}
142+
143+
@ParameterizedTest
144+
@ValueSource(strings = "/instances/valid")
145+
@NullSource
146+
void givenNullUriInstance_shouldIgnoreIt(URI instance) {
147+
Problem problem = Problem.builder().instance(instance).instance((String) null).build();
148+
149+
assertThat(problem.getInstance()).isEqualTo(instance);
150+
}
151+
152+
@ParameterizedTest
153+
@ValueSource(strings = "/instances/valid")
154+
@NullSource
155+
void givenNullStringInstance_shouldIgnoreIt(URI instance) {
156+
Problem problem = Problem.builder().instance(instance).instance((String) null).build();
157+
158+
assertThat(problem.getInstance()).isEqualTo(instance);
159+
}
160+
161+
@Test
162+
void givenNullMap_whenExtension_thenIgnoresIt() {
163+
Problem problem = Problem.builder().extension((Map<String, Object>) null).build();
164+
165+
assertThat(problem.getExtensions()).isEmpty();
166+
}
167+
168+
@Test
169+
void givenEmptyMap_whenExtension_thenIgnoresIt() {
170+
Problem problem = Problem.builder().extension(Collections.emptyMap()).build();
171+
172+
assertThat(problem.getExtensions()).isEmpty();
173+
}
174+
175+
@Test
176+
void givenMapWithExistingExtensions_whenNullMap_thenKeepsExisting() {
177+
Problem problem =
178+
Problem.builder()
179+
.extension("existing", "value")
180+
.extension((Map<String, Object>) null)
181+
.build();
182+
183+
assertThat(problem.getExtensions()).containsExactly("existing");
184+
assertThat(problem.getExtensionValue("existing")).isEqualTo("value");
185+
}
186+
187+
@Test
188+
void givenMapWithExistingExtensions_whenEmptyMap_thenKeepsExisting() {
189+
Problem problem =
190+
Problem.builder().extension("existing", "value").extension(Collections.emptyMap()).build();
191+
192+
assertThat(problem.getExtensions()).containsExactly("existing");
193+
assertThat(problem.getExtensionValue("existing")).isEqualTo("value");
194+
}
195+
196+
@Test
197+
void givenMapWithNullKey_whenExtension_thenIgnoresNullKey() {
198+
Map<String, Object> map = new HashMap<>();
199+
map.put(null, "value");
200+
map.put("valid", "value2");
201+
202+
Problem problem = Problem.builder().extension(map).build();
203+
204+
assertThat(problem.getExtensions()).containsExactly("valid");
205+
assertThat(problem.getExtensionValue("valid")).isEqualTo("value2");
206+
}
207+
208+
@Test
209+
void givenMapWithNullValue_whenExtension_thenAcceptsNullValue() {
210+
Map<String, Object> map = new HashMap<>();
211+
map.put("key", null);
212+
213+
Problem problem = Problem.builder().extension(map).build();
214+
215+
assertThat(problem.getExtensions()).containsExactly("key");
216+
assertThat(problem.getExtensionValue("key")).isNull();
217+
}
218+
219+
@Test
220+
void givenMapWithMixedValues_whenExtension_thenAcceptsAllValidEntries() {
221+
Map<String, Object> map = new HashMap<>();
222+
map.put(null, "ignored");
223+
map.put("nullValue", null);
224+
map.put("valid", "value");
225+
map.put("number", 42);
226+
227+
Problem problem = Problem.builder().extension(map).build();
228+
229+
assertThat(problem.getExtensions()).containsExactlyInAnyOrder("nullValue", "valid", "number");
230+
assertThat(problem.getExtensionValue("nullValue")).isNull();
231+
assertThat(problem.getExtensionValue("valid")).isEqualTo("value");
232+
assertThat(problem.getExtensionValue("number")).isEqualTo(42);
233+
}
234+
235+
@Test
236+
void givenManuallySetTitleMatchingStatus_whenNewStatus_thenKeepTitle() {
237+
Problem problem =
238+
Problem.builder()
239+
.status(ProblemStatus.BAD_REQUEST)
240+
.title("Bad Request")
241+
.status(ProblemStatus.NOT_FOUND)
242+
.build();
243+
244+
assertThat(problem.getStatus()).isEqualTo(404);
245+
assertThat(problem.getTitle()).isEqualTo("Bad Request");
246+
}
247+
248+
@Test
249+
void givenStatusTitleNotManuallySet_whenNewStatus_thenUpdateTitle() {
250+
Problem problem =
251+
Problem.builder().status(ProblemStatus.BAD_REQUEST).status(ProblemStatus.NOT_FOUND).build();
252+
253+
assertThat(problem.getStatus()).isEqualTo(404);
254+
assertThat(problem.getTitle()).isEqualTo("Not Found");
255+
}
256+
257+
@Test
258+
void givenManuallySetTitleDifferentFromStatus_whenNewStatus_thenKeepTitle() {
259+
Problem problem =
260+
Problem.builder()
261+
.status(ProblemStatus.BAD_REQUEST)
262+
.title("Custom Error")
263+
.status(ProblemStatus.NOT_FOUND)
264+
.build();
265+
266+
assertThat(problem.getStatus()).isEqualTo(404);
267+
assertThat(problem.getTitle()).isEqualTo("Custom Error");
268+
}
81269
}

0 commit comments

Comments
 (0)