Skip to content

Commit e57e3cc

Browse files
author
Kovacs Istvan
committed
[rachio] Address Copilot review findings
Signed-off-by: Kovacs Istvan <kovacs.istvan77@google.com>
1 parent 9241c93 commit e57e3cc

8 files changed

Lines changed: 103 additions & 6 deletions

File tree

‎bundles/org.openhab.binding.rachio/src/main/java/org/openhab/binding/rachio/internal/api/RachioApiException.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ public RachioApiException(String message, RachioApiResult result) {
4747
}
4848

4949
public RachioApiException(String message, RachioApiResult result, Throwable throwable) {
50-
super(message);
50+
super(message, throwable);
5151
apiResult = result;
5252
e = throwable;
5353
}

‎bundles/org.openhab.binding.rachio/src/main/java/org/openhab/binding/rachio/internal/api/webhook/RachioWebhookResourceType.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import java.util.Locale;
1818

1919
import org.eclipse.jdt.annotation.NonNullByDefault;
20+
import org.eclipse.jdt.annotation.Nullable;
2021

2122
/**
2223
* Resource categories supported by the modern Rachio webhook API.
@@ -60,8 +61,8 @@ public boolean isKnown() {
6061
return this != UNKNOWN;
6162
}
6263

63-
public static RachioWebhookResourceType fromApiValue(String value) {
64-
if (value.isBlank()) {
64+
public static RachioWebhookResourceType fromApiValue(@Nullable String value) {
65+
if (value == null || value.isBlank()) {
6566
return UNKNOWN;
6667
}
6768
String normalizedValue = value.trim().replace('-', '_').toUpperCase(Locale.ROOT);

‎bundles/org.openhab.binding.rachio/src/main/java/org/openhab/binding/rachio/internal/handler/RachioWebhookDispatcher.java‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import java.util.Set;
1717

1818
import org.eclipse.jdt.annotation.NonNullByDefault;
19+
import org.eclipse.jdt.annotation.Nullable;
1920
import org.openhab.binding.rachio.internal.api.json.RachioEventGsonDTO;
2021
import org.openhab.binding.rachio.internal.api.webhook.RachioWebhookResourceType;
2122
import org.slf4j.Logger;
@@ -63,10 +64,14 @@ private RachioWebhookResourceType resolveResourceType(RachioEventGsonDTO event)
6364
if (resourceType.isKnown()) {
6465
return resourceType;
6566
}
66-
if (!event.deviceId.isBlank() || "DEVICE".equals(event.category) || "ZONE".equals(event.category)
67+
if (!isBlank(event.deviceId) || "DEVICE".equals(event.category) || "ZONE".equals(event.category)
6768
|| "SCHEDULE".equals(event.category)) {
6869
return RachioWebhookResourceType.IRRIGATION_CONTROLLER;
6970
}
7071
return RachioWebhookResourceType.UNKNOWN;
7172
}
73+
74+
private static boolean isBlank(@Nullable String value) {
75+
return value == null || value.isBlank();
76+
}
7277
}

‎bundles/org.openhab.binding.rachio/src/main/resources/OH-INF/thing/cloud.xml‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,8 @@
1717
<parameter name="apikey" type="text">
1818
<label>Rachio API Key</label>
1919
<description>Visit the Rachio Web App account settings to create an API key.</description>
20-
<required>false</required>
20+
<context>password</context>
21+
<required>true</required>
2122
</parameter>
2223
<parameter name="pollingInterval" type="integer" min="30" step="30">
2324
<label>Polling/Refresh Interval</label>

‎bundles/org.openhab.binding.rachio/src/test/java/org/openhab/binding/rachio/internal/RachioConfigurationTest.java‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -226,8 +226,25 @@ void cloudThingXmlExposesRequiredConfigParameters() throws IOException, URISynta
226226
}
227227
}
228228

229+
@Test
230+
void apiKeyConfigParameterIsRequiredAndMasked() throws IOException, URISyntaxException {
231+
String apiKeyParameter = extractParameterBlock(readResource("/OH-INF/thing/cloud.xml"), PARAM_APIKEY);
232+
233+
assertThat(apiKeyParameter, containsString("<context>password</context>"));
234+
assertThat(apiKeyParameter, containsString("<required>true</required>"));
235+
}
236+
229237
private String readResource(String resourcePath) throws IOException, URISyntaxException {
230238
return Files.readString(Path.of(Objects.requireNonNull(getClass().getResource(resourcePath)).toURI()),
231239
StandardCharsets.UTF_8);
232240
}
241+
242+
private static String extractParameterBlock(String xml, String parameterName) {
243+
String openingTag = "<parameter name=\"" + parameterName + "\"";
244+
int start = xml.indexOf(openingTag);
245+
int end = xml.indexOf("</parameter>", start);
246+
assertThat("Missing parameter " + parameterName, start >= 0, is(true));
247+
assertThat("Missing parameter end tag for " + parameterName, end >= 0, is(true));
248+
return xml.substring(start, end);
249+
}
233250
}

‎bundles/org.openhab.binding.rachio/src/test/java/org/openhab/binding/rachio/internal/api/RachioApiExceptionTest.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import static org.hamcrest.MatcherAssert.assertThat;
1616
import static org.hamcrest.Matchers.containsString;
1717
import static org.hamcrest.Matchers.not;
18+
import static org.junit.jupiter.api.Assertions.assertSame;
1819

1920
import java.net.MalformedURLException;
2021
import java.net.UnknownHostException;
@@ -88,4 +89,13 @@ void apiResultPayloadIsSummarizedWithoutSensitiveContent() {
8889
assertThat(message, not(containsString("body-secret")));
8990
assertThat(message, not(containsString("\"devices\"")));
9091
}
92+
93+
@Test
94+
void apiResultConstructorPreservesCause() {
95+
Throwable cause = new IllegalStateException("boom");
96+
97+
RachioApiException exception = new RachioApiException("Request failed", new RachioApiResult(), cause);
98+
99+
assertSame(cause, exception.getCause());
100+
}
91101
}

‎bundles/org.openhab.binding.rachio/src/test/java/org/openhab/binding/rachio/internal/handler/RachioWebhookDispatcherTest.java‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,20 +19,25 @@
1919
import static org.openhab.binding.rachio.internal.RachioBindingConstants.EVENT_VALVE_RUN_START;
2020

2121
import java.util.List;
22+
import java.util.Objects;
2223

2324
import org.eclipse.jdt.annotation.NonNullByDefault;
2425
import org.junit.jupiter.api.Test;
2526
import org.openhab.binding.rachio.internal.api.json.RachioEventGsonDTO;
2627
import org.openhab.binding.rachio.internal.api.json.RachioEventGsonDTO.RachioWebhookPayload;
2728
import org.openhab.binding.rachio.internal.api.webhook.RachioWebhookResourceType;
2829

30+
import com.google.gson.Gson;
31+
2932
/**
3033
* Tests generic webhook dispatcher routing.
3134
*
3235
* @author openHAB Contributors - Initial contribution
3336
*/
3437
@NonNullByDefault
3538
class RachioWebhookDispatcherTest {
39+
private static final Gson GSON = new Gson();
40+
3641
@Test
3742
void irrigationEventRoutesThroughResourceAwareDispatcher() {
3843
RecordingHandler irrigationHandler = new RecordingHandler(RachioWebhookResourceType.IRRIGATION_CONTROLLER);
@@ -100,6 +105,64 @@ void unknownResourceTypeDoesNotCrashDispatcher() {
100105
assertThat(irrigationHandler.handled, is(false));
101106
}
102107

108+
@Test
109+
void explicitNullWebhookFieldsDoNotCrashDispatcher() {
110+
RecordingHandler irrigationHandler = new RecordingHandler(RachioWebhookResourceType.IRRIGATION_CONTROLLER);
111+
RachioWebhookDispatcher dispatcher = new RachioWebhookDispatcher(List.of(irrigationHandler));
112+
RachioEventGsonDTO event = parseWebhookEvent("""
113+
{
114+
"resourceType": null,
115+
"deviceId": null,
116+
"category": null,
117+
"eventType": "FUTURE_EVENT",
118+
"resourceId": null
119+
}
120+
""");
121+
122+
assertThat(dispatcher.dispatch(event), is(false));
123+
assertThat(irrigationHandler.handled, is(false));
124+
}
125+
126+
@Test
127+
void nullResourceTypeWithDeviceIdRoutesToIrrigationHandler() {
128+
RecordingHandler irrigationHandler = new RecordingHandler(RachioWebhookResourceType.IRRIGATION_CONTROLLER);
129+
RachioWebhookDispatcher dispatcher = new RachioWebhookDispatcher(List.of(irrigationHandler));
130+
RachioEventGsonDTO event = parseWebhookEvent("""
131+
{
132+
"resourceType": null,
133+
"deviceId": "device-id"
134+
}
135+
""");
136+
137+
assertThat(dispatcher.dispatch(event), is(true));
138+
assertThat(irrigationHandler.handled, is(true));
139+
}
140+
141+
@Test
142+
void nullResourceTypeWithIrrigationCategoryRoutesToIrrigationHandler() {
143+
RecordingHandler irrigationHandler = new RecordingHandler(RachioWebhookResourceType.IRRIGATION_CONTROLLER);
144+
RachioWebhookDispatcher dispatcher = new RachioWebhookDispatcher(List.of(irrigationHandler));
145+
RachioEventGsonDTO event = parseWebhookEvent("""
146+
{
147+
"resourceType": null,
148+
"deviceId": "",
149+
"category": "ZONE"
150+
}
151+
""");
152+
153+
assertThat(dispatcher.dispatch(event), is(true));
154+
assertThat(irrigationHandler.handled, is(true));
155+
}
156+
157+
@Test
158+
void nullApiValueReturnsUnknownResourceType() {
159+
assertThat(RachioWebhookResourceType.fromApiValue(null), is(RachioWebhookResourceType.UNKNOWN));
160+
}
161+
162+
private static RachioEventGsonDTO parseWebhookEvent(String json) {
163+
return Objects.requireNonNull(GSON.fromJson(json, RachioEventGsonDTO.class));
164+
}
165+
103166
private static class RecordingHandler implements RachioWebhookEventHandler {
104167
private final RachioWebhookResourceType resourceType;
105168
private boolean handled = false;

‎bundles/pom.xml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -691,7 +691,7 @@
691691
<plugin>
692692
<groupId>org.apache.maven.plugins</groupId>
693693
<artifactId>maven-dependency-plugin</artifactId>
694-
<version>3.10.0</version>
694+
<version>3.11.0</version>
695695
<executions>
696696
<execution>
697697
<id>embed-dependencies</id>

0 commit comments

Comments
 (0)