Skip to content
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.

Sign up for GitHub

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

feat(discovery): add generated ID to ServiceRefs #1056

Closed
wants to merge 4 commits into from
Closed

feat(discovery): add generated ID to ServiceRefs #1056

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6ac3274
feat(discovery): add generated ID to ServiceRefs
andrewazores Aug 22, 2022
4f017c7
correct test to expect UUID in responses
andrewazores Aug 23, 2022
74856b2
fix unrelated types bug
andrewazores Aug 23, 2022
54b2ca3
serialize IDs in AbstractNode embedded ServiceRef
andrewazores Sep 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/main/java/io/cryostat/discovery/AbstractNodeTypeAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
import java.util.UUID;

import io.cryostat.core.log.Logger;
import io.cryostat.platform.ServiceRef;
Expand Down Expand Up @@ -128,6 +129,7 @@ public AbstractNode read(JsonReader reader) throws IOException {
break;
case "target":
reader.beginObject();
UUID id = null;
URI connectUrl = null;
String alias = null;
Map<String, String> targetLabels = new HashMap<>();
Expand All @@ -136,6 +138,9 @@ public AbstractNode read(JsonReader reader) throws IOException {
while (reader.hasNext()) {
String targetTokenName = reader.nextName();
switch (targetTokenName) {
case "id":
id = UUID.fromString(reader.nextString());
break;
case "connectUrl":
try {
connectUrl = new URI(reader.nextString());
Expand Down Expand Up @@ -190,7 +195,7 @@ public AbstractNode read(JsonReader reader) throws IOException {
}
}
reader.endObject();
target = new ServiceRef(connectUrl, alias);
target = new ServiceRef(id, connectUrl, alias);
target.setLabels(targetLabels);
target.setPlatformAnnotations(platformAnnotations);
target.setCryostatAnnotations(cryostatAnnotations);
Expand Down Expand Up @@ -242,6 +247,9 @@ public void write(JsonWriter writer, AbstractNode node) throws IOException {

writer.name("target").beginObject();

if (sr.getId().isPresent()) {
writer.name("id").value(sr.getId().get().toString());
}
writer.name("connectUrl").value(sr.getServiceUri().toString());
writer.name("alias").value(sr.getAlias().orElse(sr.getServiceUri().toString()));
writeMap(writer, "labels", sr.getLabels());
Expand Down
21 changes: 21 additions & 0 deletions src/main/java/io/cryostat/discovery/DiscoveryStorage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,11 +176,13 @@ public Optional<PluginInfo> getById(UUID id) {
}

public Optional<PluginInfo> getByRealm(String realm) {
Objects.requireNonNull(realm, "realm");
return dao.getByRealm(realm);
}

public UUID register(String realm, URI callback) throws RegistrationException {
// FIXME this method should return a Future and be performed async
Objects.requireNonNull(realm, "realm");
try {
CompletableFuture<Boolean> cf = new CompletableFuture<>();
ping(HttpMethod.GET, callback).onComplete(ar -> cf.complete(ar.succeeded()));
Expand All @@ -200,6 +202,8 @@ public UUID register(String realm, URI callback) throws RegistrationException {
}

public Set<? extends AbstractNode> update(UUID id, Set<? extends AbstractNode> children) {
Objects.requireNonNull(id, "id");
Objects.requireNonNull(children, "children");
PluginInfo plugin = dao.get(id).orElseThrow(() -> new NotFoundException(id));
logger.trace("Discovery Update {} ({}): {}", id, plugin.getRealm(), children);
EnvironmentNode original = gson.fromJson(plugin.getSubtree(), EnvironmentNode.class);
Expand All @@ -226,6 +230,7 @@ public Set<? extends AbstractNode> update(UUID id, Set<? extends AbstractNode> c
}

public PluginInfo deregister(UUID id) {
Objects.requireNonNull(id, "id");
PluginInfo plugin = dao.get(id).orElseThrow(() -> new NotFoundException(id));
dao.delete(id);
findLeavesFrom(gson.fromJson(plugin.getSubtree(), EnvironmentNode.class)).stream()
Expand All @@ -245,6 +250,7 @@ public EnvironmentNode getDiscoveryTree() {
}

public Optional<EnvironmentNode> getSubtree(String realm) {
Objects.requireNonNull(realm, "realm");
return getByRealm(realm)
.map(PluginInfo::getSubtree)
.map(s -> gson.fromJson(s, EnvironmentNode.class));
Expand All @@ -254,12 +260,27 @@ private Set<TargetNode> getLeafNodes() {
return findLeavesFrom(getDiscoveryTree());
}

public Optional<ServiceRef> getServiceById(UUID id) {
Objects.requireNonNull(id, "id");
for (ServiceRef sr : listDiscoverableServices()) {
if (sr.getId().isEmpty()) {
logger.warn("ServiceRef {} has a null ID!");
continue;
}
if (sr.getId().get().equals((id))) {
return Optional.of(sr);
}
}
return Optional.empty();
}

@Override
public List<ServiceRef> listDiscoverableServices() {
return getLeafNodes().stream().map(TargetNode::getTarget).toList();
}

public List<ServiceRef> listDiscoverableServices(String realm) {
Objects.requireNonNull(realm, "realm");
return getSubtree(realm)
.map(this::findLeavesFrom)
.map(leaves -> leaves.stream().map(TargetNode::getTarget).toList())
Expand Down
6 changes: 3 additions & 3 deletions src/main/java/io/cryostat/discovery/PluginInfoDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,13 @@ class PluginInfoDao extends AbstractDao<UUID, PluginInfo> {
this.gson = gson;
}

public PluginInfo save(String realm, URI callback, EnvironmentNode subtree) {
PluginInfo save(String realm, URI callback, EnvironmentNode subtree) {
Objects.requireNonNull(realm);
Objects.requireNonNull(subtree);
return super.save(new PluginInfo(realm, callback, gson.toJson(subtree)));
}

public Optional<PluginInfo> getByRealm(String realm) {
Optional<PluginInfo> getByRealm(String realm) {
Objects.requireNonNull(realm);

CriteriaBuilder cb = entityManager.getCriteriaBuilder();
Expand All @@ -94,7 +94,7 @@ public Optional<PluginInfo> getByRealm(String realm) {
}
}

public PluginInfo update(UUID id, Set<? extends AbstractNode> children) {
PluginInfo update(UUID id, Set<? extends AbstractNode> children) {
Objects.requireNonNull(id);
Objects.requireNonNull(children);
EntityTransaction transaction = entityManager.getTransaction();
Expand Down
21 changes: 20 additions & 1 deletion src/main/java/io/cryostat/platform/ServiceRef.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.UUID;

import com.google.gson.annotations.SerializedName;
import org.apache.commons.lang3.builder.EqualsBuilder;
Expand All @@ -52,25 +53,41 @@

public class ServiceRef {

private UUID id;
private final @SerializedName("connectUrl") URI serviceUri;
private final String alias;
private final Map<String, String> labels;
private final Annotations annotations;

public ServiceRef(URI uri, String alias) {
// TODO we generate these IDs manually in application code, and these get stored in the
// Discovery database where ServiceRefs are embedded within TargetNodes in the realms' subtrees.
// Should we create a table just for ServiceRefs, allow the database to generate the ID, and
// have a relationship between the Discovery plugin table and the ServiceRef table? It's a tree
// relationship, not a simple relational one-one or many-many etc.
public ServiceRef(UUID id, URI uri, String alias) {
this.id = id != null ? id : UUID.randomUUID();
this.serviceUri = Objects.requireNonNull(uri);
this.alias = alias;
this.labels = new HashMap<>();
this.annotations = new Annotations();
}

public ServiceRef(URI uri, String alias) {
this(UUID.randomUUID(), uri, alias);
}

public ServiceRef(ServiceRef sr) {
this.id = sr.id;
this.serviceUri = sr.serviceUri;
this.alias = sr.alias;
this.labels = new HashMap<String, String>(sr.labels);
this.annotations = new Annotations(sr.annotations);
}

public Optional<UUID> getId() {
return Optional.ofNullable(id);
}

public URI getServiceUri() {
return serviceUri;
}
Expand Down Expand Up @@ -127,6 +144,7 @@ public boolean equals(Object other) {
return false;
}
ServiceRef sr = (ServiceRef) other;
// id is intentionally ignored
return new EqualsBuilder()
.append(serviceUri, sr.serviceUri)
.append(alias, sr.alias)
Expand All @@ -137,6 +155,7 @@ public boolean equals(Object other) {

@Override
public int hashCode() {
// id is intentionally ignored
return new HashCodeBuilder()
.append(serviceUri)
.append(alias)
Expand Down
7 changes: 2 additions & 5 deletions src/test/java/io/cryostat/discovery/DiscoveryStorageTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,6 @@ class Updating {

@Test
void throwsIfUuidNull() {
Mockito.when(dao.get(Mockito.isNull())).thenThrow(NullPointerException.class);
Assertions.assertThrows(
NullPointerException.class, () -> storage.update(null, Set.of()));
}
Expand All @@ -401,9 +400,8 @@ void throwsIfInvalidIdGiven() {

@Test
void throwsIfChildrenNull() {
UUID id = UUID.randomUUID();
Mockito.when(dao.get(id)).thenReturn(Optional.of(new PluginInfo()));
Assertions.assertThrows(NullPointerException.class, () -> storage.update(id, null));
Assertions.assertThrows(
NullPointerException.class, () -> storage.update(UUID.randomUUID(), null));
}

@Test
Expand Down Expand Up @@ -456,7 +454,6 @@ class Deregistration {

@Test
void throwsIfUuidNull() {
Mockito.when(dao.get(Mockito.isNull())).thenThrow(NullPointerException.class);
Assertions.assertThrows(NullPointerException.class, () -> storage.deregister(null));
}

Expand Down
140 changes: 100 additions & 40 deletions src/test/java/itest/CustomTargetsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
import io.vertx.core.json.JsonArray;
import io.vertx.core.json.JsonObject;
import itest.bases.StandardSelfTest;
import org.hamcrest.Matcher;
import org.hamcrest.MatcherAssert;
import org.hamcrest.Matchers;
import org.junit.jupiter.api.MethodOrderer.OrderAnnotation;
Expand Down Expand Up @@ -133,45 +134,69 @@ void targetShouldAppearInListing() throws ExecutionException, InterruptedExcepti
MatcherAssert.assertThat(body, Matchers.notNullValue());
MatcherAssert.assertThat(body.size(), Matchers.equalTo(2));

JsonObject selfJdp =
new JsonObject(
Map.of(
"alias",
"io.cryostat.Cryostat",
"connectUrl",
"service:jmx:rmi:///jndi/rmi://cryostat-itests:9091/jmxrmi",
"labels",
Map.of(),
"annotations",
Map.of(
"cryostat",
Map.of(
"REALM",
"JDP",
"HOST",
"cryostat-itests",
"PORT",
"9091",
"JAVA_MAIN",
"io.cryostat.Cryostat"),
"platform",
Map.of())));
JsonObject selfCustom =
new JsonObject(
Map.of(
"alias",
"self",
"connectUrl",
"localhost:0",
"labels",
Map.of(),
"annotations",
Map.of(
"cryostat",
Map.of("REALM", "Custom Targets"),
"platform",
Map.of())));
MatcherAssert.assertThat(body, Matchers.containsInAnyOrder(selfJdp, selfCustom));
Matcher selfJdpMatcher =
Matchers.allOf(
(Matcher)
Matchers.hasEntry(
Matchers.equalTo("id"),
Matchers.not(Matchers.blankOrNullString())),
(Matcher)
Matchers.hasEntry(
Matchers.equalTo("alias"),
Matchers.equalTo("io.cryostat.Cryostat")),
(Matcher)
Matchers.hasEntry(
Matchers.equalTo("connectUrl"),
Matchers.equalTo(
"service:jmx:rmi:///jndi/rmi://cryostat-itests:9091/jmxrmi")),
(Matcher)
Matchers.hasEntry(
Matchers.equalTo("labels"), Matchers.equalTo(Map.of())),
(Matcher)
Matchers.hasEntry(
Matchers.equalTo("annotations"),
Matchers.equalTo(
Map.of(
"cryostat",
Map.of(
"REALM",
"JDP",
"HOST",
"cryostat-itests",
"PORT",
"9091",
"JAVA_MAIN",
"io.cryostat.Cryostat"),
"platform", Map.of()))));

Matcher selfCustomMatcher =
Matchers.allOf(
(Matcher)
Matchers.hasEntry(
Matchers.equalTo("id"),
Matchers.not(Matchers.blankOrNullString())),
(Matcher)
Matchers.hasEntry(
Matchers.equalTo("alias"), Matchers.equalTo("self")),
(Matcher)
Matchers.hasEntry(
Matchers.equalTo("connectUrl"),
Matchers.equalTo("localhost:0")),
(Matcher)
Matchers.hasEntry(
Matchers.equalTo("labels"), Matchers.equalTo(Map.of())),
(Matcher)
Matchers.hasEntry(
Matchers.equalTo("annotations"),
Matchers.equalTo(
Map.of(
"cryostat",
Map.of("REALM", "Custom Targets"),
"platform",
Map.of()))));

MatcherAssert.assertThat(
body.getList(), Matchers.containsInAnyOrder(selfJdpMatcher, selfCustomMatcher));
}

@Test
Expand Down Expand Up @@ -260,6 +285,41 @@ void targetShouldNoLongerAppearInListing() throws ExecutionException, Interrupte
"io.cryostat.Cryostat"),
"platform",
Map.of())));
MatcherAssert.assertThat(body, Matchers.contains(selfJdp));

Matcher selfJdpMatcher =
Matchers.allOf(
(Matcher)
Matchers.hasEntry(
Matchers.equalTo("id"),
Matchers.not(Matchers.blankOrNullString())),
(Matcher)
Matchers.hasEntry(
Matchers.equalTo("alias"),
Matchers.equalTo("io.cryostat.Cryostat")),
(Matcher)
Matchers.hasEntry(
Matchers.equalTo("connectUrl"),
Matchers.equalTo(
"service:jmx:rmi:///jndi/rmi://cryostat-itests:9091/jmxrmi")),
(Matcher)
Matchers.hasEntry(
Matchers.equalTo("labels"), Matchers.equalTo(Map.of())),
(Matcher)
Matchers.hasEntry(
Matchers.equalTo("annotations"),
Matchers.equalTo(
Map.of(
"cryostat",
Map.of(
"REALM",
"JDP",
"HOST",
"cryostat-itests",
"PORT",
"9091",
"JAVA_MAIN",
"io.cryostat.Cryostat"),
"platform", Map.of()))));
MatcherAssert.assertThat(body.getList(), Matchers.containsInAnyOrder(selfJdpMatcher));
}
}