From adf81bb96af142cc1bb4dbf99b5b441e531c1674 Mon Sep 17 00:00:00 2001 From: Ryan Baxter Date: Fri, 30 Oct 2026 11:04:45 -0400 Subject: [PATCH] Fixes potential NPE in getServices and getAllInstances Fixes #1040 --- .../discovery/ConsulDiscoveryClient.java | 24 ++++++++++------- .../ConsulReactiveDiscoveryClient.java | 14 +++++----- .../ConsulReactiveDiscoveryClientTests.java | 26 ++++++++++++++++--- 3 files changed, 43 insertions(+), 21 deletions(-) diff --git a/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulDiscoveryClient.java b/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulDiscoveryClient.java index e327e87ab..c71e41a1e 100644 --- a/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulDiscoveryClient.java +++ b/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulDiscoveryClient.java @@ -79,18 +79,20 @@ private void addInstancesToList(List instances, String serviceI ResponseEntity> healthServices = client.getHealthServices(serviceId, properties.isQueryPassing(), properties.getAclToken(), tags, queryParams); - for (HealthService service : healthServices.getBody()) { + List body = healthServices.getBody(); + if (body == null) { + throw new IllegalStateException("Consul did not return instances for service " + serviceId + + " (HTTP status " + healthServices.getStatusCode() + ")"); + } + + for (HealthService service : body) { instances.add(new ConsulServiceInstance(service, serviceId)); } } public List getAllInstances() { List instances = new ArrayList<>(); - Map> catalogServices = client - .getCatalogServices(properties.getAclToken(), QueryParams.DEFAULT) - .getBody(); - - for (String serviceId : catalogServices.keySet()) { + for (String serviceId : getServices()) { addInstancesToList(instances, serviceId, null); } return instances; @@ -98,9 +100,13 @@ public List getAllInstances() { @Override public List getServices() { - Map> catalogServices = client - .getCatalogServices(properties.getAclToken(), QueryParams.DEFAULT) - .getBody(); + ResponseEntity>> response = client.getCatalogServices(properties.getAclToken(), + QueryParams.DEFAULT); + Map> catalogServices = response.getBody(); + if (catalogServices == null) { + throw new IllegalStateException( + "Consul did not return a service catalog (HTTP status " + response.getStatusCode() + ")"); + } return new ArrayList<>(catalogServices.keySet()); } diff --git a/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/reactive/ConsulReactiveDiscoveryClient.java b/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/reactive/ConsulReactiveDiscoveryClient.java index 006fd3028..c34dd59fd 100644 --- a/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/reactive/ConsulReactiveDiscoveryClient.java +++ b/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/reactive/ConsulReactiveDiscoveryClient.java @@ -62,10 +62,9 @@ public Flux getInstances(String serviceId) { return Flux.defer(() -> { List instances = blockingClient.getInstances(serviceId); return Flux.fromIterable(instances); - }).onErrorResume(exception -> { - logger.error("Error getting instances from Consul.", exception); - return Flux.empty(); - }).subscribeOn(Schedulers.boundedElastic()); + }) + .doOnError(exception -> logger.error("Error getting instances from Consul.", exception)) + .subscribeOn(Schedulers.boundedElastic()); } @Override @@ -73,10 +72,9 @@ public Flux getServices() { return Flux.defer(() -> { List services = blockingClient.getServices(); return services == null ? Flux.empty() : Flux.fromIterable(services); - }).onErrorResume(exception -> { - logger.error("Error getting services from Consul.", exception); - return Flux.empty(); - }).subscribeOn(Schedulers.boundedElastic()); + }) + .doOnError(exception -> logger.error("Error getting services from Consul.", exception)) + .subscribeOn(Schedulers.boundedElastic()); } @Override diff --git a/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/reactive/ConsulReactiveDiscoveryClientTests.java b/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/reactive/ConsulReactiveDiscoveryClientTests.java index 9b96307f5..bd3f1c085 100644 --- a/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/reactive/ConsulReactiveDiscoveryClientTests.java +++ b/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/reactive/ConsulReactiveDiscoveryClientTests.java @@ -71,14 +71,22 @@ public void verifyDefaults() { } @Test - public void shouldReturnEmptyFluxOfServicesWhenConsulFails() { + public void shouldPropagateErrorFromServicesWhenConsulFails() { Flux services = client.getServices(); when(consulClient.getCatalogServices(any(), any())) .thenThrow(new RuntimeException("Possible runtime exception")); - StepVerifier.create(services).expectNextCount(0).expectComplete().verify(); + StepVerifier.create(services).expectErrorMessage("Possible runtime exception").verify(); verify(consulClient).getCatalogServices(any(), any()); } + @Test + public void shouldPropagateErrorFromServicesWhenConsulReturnsNoBody() { + Flux services = client.getServices(); + when(consulClient.getCatalogServices(any(), any())) + .thenReturn(ResponseEntity.status(500).>>build()); + StepVerifier.create(services).expectError(IllegalStateException.class).verify(); + } + @Test public void shouldReturnFluxOfServices() { Flux services = client.getServices(); @@ -121,17 +129,27 @@ public void shouldReturnEmptyFluxForNonExistingService() { } @Test - public void shouldReturnEmptyFluxWhenConsulFails() { + public void shouldPropagateErrorFromInstancesWhenConsulFails() { configureCommonProperties(); when(consulClient.getHealthServices(eq("existing-service"), any(boolean.class), any(), any(List.class), any(QueryParams.class))) .thenThrow(new RuntimeException("Possible runtime exception")); Flux instances = client.getInstances("existing-service"); - StepVerifier.create(instances).expectNextCount(0).expectComplete().verify(); + StepVerifier.create(instances).expectErrorMessage("Possible runtime exception").verify(); verify(consulClient).getHealthServices(eq("existing-service"), any(boolean.class), any(), any(List.class), any(QueryParams.class)); } + @Test + public void shouldPropagateErrorFromInstancesWhenConsulReturnsNoBody() { + configureCommonProperties(); + when(consulClient.getHealthServices(eq("existing-service"), any(boolean.class), any(), any(List.class), + any(QueryParams.class))) + .thenReturn(ResponseEntity.status(500).>build()); + Flux instances = client.getInstances("existing-service"); + StepVerifier.create(instances).expectError(IllegalStateException.class).verify(); + } + @Test public void shouldReturnFluxOfServiceInstances() { configureCommonProperties();