Code review

code review

Author: aureamunoz

api/revapi.json

@@ -45,6 +45,12 @@
         "code": "java.method.addedToInterface",
         "new": "method io.smallrye.mutiny.Uni<java.lang.Void> io.smallrye.stork.api.ServiceRegistrar<MetadataKeyType extends java.lang.Enum<MetadataKeyType> & io.smallrye.stork.api.MetadataKey>::registerServiceInstance(java.lang.String, java.lang.String, java.util.List<java.lang.String>, io.smallrye.stork.api.Metadata<MetadataKeyType>, java.lang.String, int)",
         "justification": "Added tags parameter to support tagging service instances at registration time."
+      },
+      {
+        "ignore": true,
+        "code": "java.method.addedToInterface",
+        "new": "method io.smallrye.mutiny.Uni<java.lang.Void> io.smallrye.stork.api.ServiceRegistrar<MetadataKeyType extends java.lang.Enum<MetadataKeyType> & io.smallrye.stork.api.MetadataKey>::registerServiceInstance(java.lang.String, java.util.List<java.lang.String>, io.smallrye.stork.api.Metadata<MetadataKeyType>, java.lang.String, int)",
+        "justification": "Added tags-only registration method (without instanceName) as a default method, backward-compatible."
       }
     ]
   }

api/src/main/java/io/smallrye/stork/api/Service.java

@@ -239,6 +239,10 @@ public ServiceRegistrar getServiceRegistrar() {
      * @return a {@link Uni} completing when the registration is done
      */
     public Uni<Void> registerInstance(String ipAddress, int port) {
+        if (serviceRegistrar == null) {
+            return Uni.createFrom()
+                    .failure(new UnsupportedOperationException("This service has no service registrar configured."));
+        }
         return serviceRegistrar.registerServiceInstance(serviceName, ipAddress, port);
     }
 
@@ -250,11 +254,44 @@ public Uni<Void> registerInstance(String ipAddress, int port) {
      * @param port the port of the instance
      * @return a {@link Uni} completing when the registration is done
      */
-    public Uni<Void> registerInstance(String instanceName, String ipAddress, int port) {
+    public Uni<Void> registerNamedInstance(String instanceName, String ipAddress, int port) {
+        if (serviceRegistrar == null) {
+            return Uni.createFrom()
+                    .failure(new UnsupportedOperationException("This service has no service registrar configured."));
+        }
         return serviceRegistrar.registerServiceInstance(serviceName, instanceName, Metadata.empty(), ipAddress, port);
     }
 
-    public Uni<Void> registerInstance(String instanceName, List<String> tags, String ipAddress, int port) {
+    /**
+     * Registers a service instance with tags.
+     *
+     * @param tags the tags to associate with this instance
+     * @param ipAddress the IP address of the instance
+     * @param port the port of the instance
+     * @return a {@link Uni} completing when the registration is done
+     */
+    public Uni<Void> registerInstance(List<String> tags, String ipAddress, int port) {
+        if (serviceRegistrar == null) {
+            return Uni.createFrom()
+                    .failure(new UnsupportedOperationException("This service has no service registrar configured."));
+        }
+        return serviceRegistrar.registerServiceInstance(serviceName, tags, Metadata.empty(), ipAddress, port);
+    }
+
+    /**
+     * Registers a service instance with an explicit instance name and tags.
+     *
+     * @param instanceName the unique identifier for this instance in the registry
+     * @param tags the tags to associate with this instance
+     * @param ipAddress the IP address of the instance
+     * @param port the port of the instance
+     * @return a {@link Uni} completing when the registration is done
+     */
+    public Uni<Void> registerNamedInstance(String instanceName, List<String> tags, String ipAddress, int port) {
+        if (serviceRegistrar == null) {
+            return Uni.createFrom()
+                    .failure(new UnsupportedOperationException("This service has no service registrar configured."));
+        }
         return serviceRegistrar.registerServiceInstance(serviceName, instanceName, tags, Metadata.empty(), ipAddress, port);
     }
 
@@ -265,31 +302,79 @@ public Uni<Void> registerInstance(String instanceName, List<String> tags, String
      * @return a {@link Uni} completing when the registration is done
      */
     public Uni<Void> registerInstance(ServiceRegistrar.RegistrarOptions options) {
+        if (serviceRegistrar == null) {
+            return Uni.createFrom()
+                    .failure(new UnsupportedOperationException("This service has no service registrar configured."));
+        }
         return serviceRegistrar.registerServiceInstance(options);
     }
 
+    /**
+     * @deprecated Use {@link #registerInstance(String, int)} instead. The service name is already
+     *             known by this {@link Service} instance.
+     */
+    @Deprecated(forRemoval = true)
+    public Uni<Void> registerInstance(String serviceName, String ipAddress, int port) {
+        return registerInstance(ipAddress, port);
+    }
+
+    /**
+     * @deprecated Use {@link #registerNamedInstance(String, String, int)} instead. The service name
+     *             is already known by this {@link Service} instance.
+     */
+    @Deprecated(forRemoval = true)
+    public Uni<Void> registerInstance(String serviceName, String instanceName, String ipAddress, int port) {
+        return registerNamedInstance(instanceName, ipAddress, port);
+    }
+
     /**
      * Deregisters all instances of this service.
      *
      * @return a {@link Uni} completing when the deregistration is done
-     * @deprecated Prefer {@link #deregisterServiceInstance(String, int)} or
-     *             {@link #deregisterServiceInstance(String)} to target a specific instance.
+     * @deprecated Prefer {@link #deregisterNamedInstance(String)} or
+     *             {@link #deregisterServiceInstance(String, int)} to target a specific instance.
      */
     @Deprecated(forRemoval = true)
     public Uni<Void> deregisterServiceInstance() {
+        if (serviceRegistrar == null) {
+            return Uni.createFrom()
+                    .failure(new UnsupportedOperationException("This service has no service registrar configured."));
+        }
         return serviceRegistrar.deregisterServiceInstance(serviceName);
     }
 
+    /**
+     * @deprecated Use {@link #deregisterServiceInstance()} instead. The service name is already
+     *             known by this {@link Service} instance.
+     */
+    @Deprecated(forRemoval = true)
+    public Uni<Void> deregisterServiceInstance(String serviceName) {
+        return deregisterServiceInstance();
+    }
+
     /**
      * Deregisters the service instance identified by the given instance name.
      *
      * @param instanceName the registration ID of the instance to deregister
      * @return a {@link Uni} completing when the deregistration is done
      */
-    public Uni<Void> deregisterServiceInstance(String instanceName) {
+    public Uni<Void> deregisterNamedInstance(String instanceName) {
+        if (serviceRegistrar == null) {
+            return Uni.createFrom()
+                    .failure(new UnsupportedOperationException("This service has no service registrar configured."));
+        }
         return serviceRegistrar.deregisterServiceInstance(serviceName, instanceName);
     }
 
+    /**
+     * @deprecated Use {@link #deregisterNamedInstance(String)} instead. The service name is already
+     *             known by this {@link Service} instance.
+     */
+    @Deprecated(forRemoval = true)
+    public Uni<Void> deregisterServiceInstance(String serviceName, String instanceName) {
+        return deregisterNamedInstance(instanceName);
+    }
+
     /**
      * Deregisters the service instance identified by the given IP address and port.
      *
@@ -298,9 +383,22 @@ public Uni<Void> deregisterServiceInstance(String instanceName) {
      * @return a {@link Uni} completing when the deregistration is done
      */
     public Uni<Void> deregisterServiceInstance(String ipAddress, int port) {
+        if (serviceRegistrar == null) {
+            return Uni.createFrom()
+                    .failure(new UnsupportedOperationException("This service has no service registrar configured."));
+        }
         return serviceRegistrar.deregisterServiceInstance(serviceName, ipAddress, port);
     }
 
+    /**
+     * @deprecated Use {@link #deregisterServiceInstance(String, int)} instead. The service name is
+     *             already known by this {@link Service} instance.
+     */
+    @Deprecated(forRemoval = true)
+    public Uni<Void> deregisterServiceInstance(String serviceName, String ipAddress, int port) {
+        return deregisterServiceInstance(ipAddress, port);
+    }
+
     public ObservationCollector getObservations() {
         return observations;
     }

api/src/main/java/io/smallrye/stork/api/ServiceRegistrar.java

@@ -32,20 +32,29 @@ default void checkRegistrarOptionsNotNull(RegistrarOptions options) {
 
     default Uni<Void> registerServiceInstance(String serviceName, Metadata<MetadataKeyType> metadata, String ipAddress,
             int defaultPort) {
-        return registerServiceInstance(serviceName, null, metadata, ipAddress, defaultPort);
+        return registerServiceInstance(serviceName, (String) null, metadata, ipAddress, defaultPort);
+    }
+
+    default Uni<Void> registerServiceInstance(String serviceName, List<String> tags,
+            Metadata<MetadataKeyType> metadata, String ipAddress, int defaultPort) {
+        return registerServiceInstance(serviceName, null, tags, metadata, ipAddress, defaultPort);
     }
 
     Uni<Void> registerServiceInstance(String serviceName, String instanceName, Metadata<MetadataKeyType> metadata,
             String ipAddress, int defaultPort);
 
-    Uni<Void> registerServiceInstance(String serviceName, String instanceName, List<String> tags,
-            Metadata<MetadataKeyType> metadata, String ipAddress, int defaultPort);
+    default Uni<Void> registerServiceInstance(String serviceName, String instanceName, List<String> tags,
+            Metadata<MetadataKeyType> metadata, String ipAddress, int defaultPort) {
+        return registerServiceInstance(serviceName, instanceName, metadata, ipAddress, defaultPort);
+    }
 
     default Uni<Void> registerServiceInstance(RegistrarOptions options) {
         checkRegistrarOptionsNotNull(options);
         checkAddressNotNull(options.ipAddress());
 
-        return registerServiceInstance(options.serviceName(), options.ipAddress(), options.defaultPort());
+        return registerServiceInstance(options.serviceName(), null, options.tags() != null ? options.tags() : List.of(),
+                Metadata.empty(), options.ipAddress(),
+                options.defaultPort());
     }
 
     @Deprecated

api/src/test/java/io/smallrye/stork/api/ServiceTest.java

@@ -0,0 +1,90 @@
+package io.smallrye.stork.api;
+
+import java.util.List;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+import io.smallrye.mutiny.Uni;
+import io.smallrye.stork.api.observability.NoopObservationCollector;
+
+class ServiceTest {
+
+    private static Service newServiceWithoutRegistrar() {
+        return new Service("my-service", "round-robin", "test-discovery", new NoopObservationCollector(),
+                serviceInstances -> {
+                    throw new AssertionError("Load balancer should not be used in this test");
+                },
+                () -> Uni.createFrom().item(List.of()), null, false);
+    }
+
+    @Test
+    void registerInstanceShouldFailWhenRegistrarIsMissing() {
+        Service service = newServiceWithoutRegistrar();
+
+        UnsupportedOperationException exception = Assertions.assertThrows(UnsupportedOperationException.class,
+                () -> service.registerInstance("127.0.0.1", 8080).await().indefinitely());
+
+        Assertions.assertEquals("This service has no service registrar configured.", exception.getMessage());
+    }
+
+    @Test
+    void registerNamedInstanceShouldFailWhenRegistrarIsMissing() {
+        Service service = newServiceWithoutRegistrar();
+
+        Assertions.assertThrows(UnsupportedOperationException.class,
+                () -> service.registerNamedInstance("instance-1", "127.0.0.1", 8080).await().indefinitely());
+    }
+
+    @Test
+    void deregisterNamedInstanceShouldFailWhenRegistrarIsMissing() {
+        Service service = newServiceWithoutRegistrar();
+
+        Assertions.assertThrows(UnsupportedOperationException.class,
+                () -> service.deregisterNamedInstance("instance-1").await().indefinitely());
+    }
+
+    @Test
+    void deregisterByAddressShouldFailWhenRegistrarIsMissing() {
+        Service service = newServiceWithoutRegistrar();
+
+        Assertions.assertThrows(UnsupportedOperationException.class,
+                () -> service.deregisterServiceInstance("127.0.0.1", 8080).await().indefinitely());
+    }
+
+    @Test
+    void registerWithOptionsShouldFailWhenRegistrarIsMissing() {
+        Service service = newServiceWithoutRegistrar();
+
+        ServiceRegistrar.RegistrarOptions options = new ServiceRegistrar.RegistrarOptions(
+                "my-service", "127.0.0.1", 8080, List.of("tag-1"), null);
+
+        Assertions.assertThrows(UnsupportedOperationException.class,
+                () -> service.registerInstance(options).await().indefinitely());
+    }
+
+    @Test
+    void registerWithTagsShouldFailWhenRegistrarIsMissing() {
+        Service service = newServiceWithoutRegistrar();
+
+        Assertions.assertThrows(UnsupportedOperationException.class,
+                () -> service.registerInstance(List.of("v1.0"), "127.0.0.1", 8080).await().indefinitely());
+    }
+
+    @Test
+    void registerNamedInstanceWithTagsShouldFailWhenRegistrarIsMissing() {
+        Service service = newServiceWithoutRegistrar();
+
+        Assertions.assertThrows(UnsupportedOperationException.class,
+                () -> service.registerNamedInstance("instance-1", List.of("v1.0"), "127.0.0.1", 8080).await()
+                        .indefinitely());
+    }
+
+    @Test
+    void deregisterAllShouldFailWhenRegistrarIsMissing() {
+        Service service = newServiceWithoutRegistrar();
+
+        Assertions.assertThrows(UnsupportedOperationException.class,
+                () -> service.deregisterServiceInstance().await().indefinitely());
+    }
+}

docs/docs/service-registration/custom-service-registration.md

@@ -88,6 +88,9 @@ Finally, you can implement your own service deregistration mechanism in `deregis
     The default implementations of the second and third methods fall back to the first, which may deregister **all** instances.
     **Custom registrar implementations should override at least one of the specific-instance methods** to provide precise, instance-level deregistration and avoid unintended side effects.
 
+    Note that the `Service` class wraps these with simpler methods that do not require passing the service name:
+    `deregisterNamedInstance(instanceName)`, `deregisterServiceInstance(ipAddress, port)`, and the deprecated no-arg `deregisterServiceInstance()`.
+
 
 ## Using your service registrar using the programmatic API
 

docs/docs/service-registration/overview.md

@@ -39,13 +39,13 @@ SmallRye Stork now supports deregistration of service instances from a central r
 This feature complements the registration mechanism.
 
 This operation is typically should be triggered when a service instance is shut down or no longer available, helping maintain a consistent and accurate service registry.
-To enable service deregistration, you can invoke the `deregisterServiceInstance` method on the `ServiceRegistrar` implementation programmatically.
-To target a specific instance, prefer one of these overloads:
+To enable service deregistration, you can invoke the deregistration methods on the `Service` instance programmatically.
+To target a specific instance, prefer one of these methods:
 
-- `deregisterServiceInstance(serviceName, instanceName)` — deregisters the instance registered under the given name.
-- `deregisterServiceInstance(serviceName, ipAddress, port)` — deregisters the instance identified by IP and port.
+- `deregisterNamedInstance(instanceName)` — deregisters the instance registered under the given name.
+- `deregisterServiceInstance(ipAddress, port)` — deregisters the instance identified by IP and port.
 
-The single-argument overload `deregisterServiceInstance(serviceName)` deregisters **all** instances of the service.
+The no-argument overload `deregisterServiceInstance()` deregisters **all** instances of the service and is deprecated, prefer one of the specific methods above.
 
 **Note**: When used in standalone mode, Stork does **not** automatically handle service instance registration or deregistration. 
 However, when using the quarkus-stork-registration extension within a Quarkus application, 

docs/snippets/examples/AcmeRegistrarApiUsage.java

@@ -14,7 +14,7 @@ public void example(StorkServiceRegistry stork) {
                 new AcmeLoadBalancerConfiguration().withMyAttribute("my-value"),new AcmeRegistrarConfiguration())
         );
 
-        stork.getService("my-service").registerInstance("my-service", "localhost",
+        stork.getService("my-service").registerNamedInstance("my-instance", "localhost",
                 9000);
     }