Skip to content

feat: automatically derive the the dependent resource type if not specified #2772

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,6 @@ public class ConfigMapDependentResource

public static final String KEY = "key";

public ConfigMapDependentResource() {
super(ConfigMap.class);
}

@Override
protected ConfigMap desired({{artifactClassId}}CustomResource primary,
Context<{{artifactClassId}}CustomResource> context) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,10 +136,6 @@ Deleted (or set to be garbage collected). The following example shows how to cre
@KubernetesDependent(labelSelector = WebPageManagedDependentsReconciler.SELECTOR)
class DeploymentDependentResource extends CRUDKubernetesDependentResource<Deployment, WebPage> {

public DeploymentDependentResource() {
super(Deployment.class);
}

@Override
protected Deployment desired(WebPage webPage, Context<WebPage> context) {
var deploymentName = deploymentName(webPage);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,27 @@ public static Class<?> getTypeArgumentFromExtendedClassByIndex(Class<?> clazz, i
}
}

public static Class<?> getTypeArgumentFromHierarchyByIndex(
Class<?> clazz, Class<?> expectedImplementedInterface, int index) {
Class<?> c = clazz;
while (!(c.getGenericSuperclass() instanceof ParameterizedType)) {
c = c.getSuperclass();
}
Class<?> actualTypeArgument =
(Class<?>) ((ParameterizedType) c.getGenericSuperclass()).getActualTypeArguments()[index];
if (!expectedImplementedInterface.isAssignableFrom(actualTypeArgument)) {
throw new IllegalArgumentException(
GENERIC_PARAMETER_TYPE_ERROR_PREFIX
+ clazz.getName()
+ "because it doesn't extend a class that is parametrized with the type that"
+ " implements "
+ expectedImplementedInterface.getSimpleName()
+ ". Please provide the resource type in the constructor (e.g.,"
+ " super(Deployment.class).");
}
return actualTypeArgument;
}

public static Class<?> getFirstTypeArgumentFromInterface(
Class<?> clazz, Class<?> expectedImplementedInterface) {
return getTypeArgumentFromInterfaceByIndex(clazz, expectedImplementedInterface, 0);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package io.javaoperatorsdk.operator.processing.dependent;

import java.lang.reflect.ParameterizedType;
import java.util.Optional;

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.javaoperatorsdk.operator.api.config.Utils;
import io.javaoperatorsdk.operator.api.reconciler.Context;
import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext;
import io.javaoperatorsdk.operator.api.reconciler.Ignore;
Expand All @@ -23,13 +25,23 @@ public abstract class AbstractEventSourceHolderDependentResource<
private boolean isCacheFillerEventSource;
protected String eventSourceNameToUse;

@SuppressWarnings("unchecked")
protected AbstractEventSourceHolderDependentResource() {
this(null, null);
}

protected AbstractEventSourceHolderDependentResource(Class<R> resourceType) {
this(resourceType, null);
}

protected AbstractEventSourceHolderDependentResource(Class<R> resourceType, String name) {
super(name);
this.resourceType = resourceType;
if (resourceType == null) {
this.resourceType =
(Class<R>) Utils.getTypeArgumentFromHierarchyByIndex(getClass(), HasMetadata.class, 0);
} else {
this.resourceType = resourceType;
}
}

/**
Expand Down Expand Up @@ -116,4 +128,23 @@ protected void onUpdated(P primary, R updated, R actual, Context<P> context) {
private RecentOperationCacheFiller<R> recentOperationCacheFiller() {
return (RecentOperationCacheFiller<R>) eventSource;
}

@SuppressWarnings("unchecked")
private Class<R> findFirstGenericSuperclass(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should throw a meaningful exception when we cannot find the class, could pls add that, also show it works in a unit test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@csviri, this is impossible because this class is Abstract, so it must be subclassed, and it takes parameters. So we can never run into a subclass without parameters.

But you made me realize another potential issue, so I updated the PR.

Class<? extends AbstractEventSourceHolderDependentResource> clazz) {
Class<? extends AbstractEventSourceHolderDependentResource> c = clazz;
while (!(c.getGenericSuperclass() instanceof ParameterizedType)) {
c = (Class<? extends AbstractEventSourceHolderDependentResource>) c.getSuperclass();
}
Class<R> actualTypeArgument =
(Class<R>) ((ParameterizedType) c.getGenericSuperclass()).getActualTypeArguments()[0];
if (!HasMetadata.class.isAssignableFrom(actualTypeArgument)) {
throw new IllegalStateException(
"Not possible to automatically derive the the resource type for "
+ clazz.getName()
+ ". Please provide the resource type in the constructor (e.g.,"
+ " super(Deployment.class).");
}
return actualTypeArgument;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ public abstract class AbstractExternalDependentResource<

private InformerEventSource<?, P> externalStateEventSource;

protected AbstractExternalDependentResource() {}

@SuppressWarnings("unchecked")
protected AbstractExternalDependentResource(Class<R> resourceType) {
super(resourceType);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ public abstract class AbstractPollingDependentResource<R, P extends HasMetadata>
public static final Duration DEFAULT_POLLING_PERIOD = Duration.ofMillis(5000);
private Duration pollingPeriod;

protected AbstractPollingDependentResource() {}

protected AbstractPollingDependentResource(Class<R> resourceType) {
this(resourceType, DEFAULT_POLLING_PERIOD);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ public abstract class PerResourcePollingDependentResource<R, P extends HasMetada
extends AbstractPollingDependentResource<R, P>
implements PerResourcePollingEventSource.ResourceFetcher<R, P> {

public PerResourcePollingDependentResource() {}

public PerResourcePollingDependentResource(Class<R> resourceType) {
super(resourceType);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ public abstract class CRUDKubernetesDependentResource<R extends HasMetadata, P e
extends KubernetesDependentResource<R, P>
implements Creator<R, P>, Updater<R, P>, GarbageCollected<P> {

public CRUDKubernetesDependentResource() {}

public CRUDKubernetesDependentResource(Class<R> resourceType) {
super(resourceType);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
public class CRUDNoGCKubernetesDependentResource<R extends HasMetadata, P extends HasMetadata>
extends KubernetesDependentResource<R, P> implements Creator<R, P>, Updater<R, P>, Deleter<P> {

public CRUDNoGCKubernetesDependentResource() {}

public CRUDNoGCKubernetesDependentResource(Class<R> resourceType) {
super(resourceType);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ public abstract class KubernetesDependentResource<R extends HasMetadata, P exten
private KubernetesDependentResourceConfig<R> kubernetesDependentResourceConfig;
private volatile Boolean useSSA;

public KubernetesDependentResource() {}

public KubernetesDependentResource(Class<R> resourceType) {
this(resourceType, null);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -359,21 +359,11 @@ public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap>
}

public static class ReadOnlyDependent extends KubernetesDependentResource<ConfigMap, ConfigMap>
implements GarbageCollected<ConfigMap> {

public ReadOnlyDependent() {
super(ConfigMap.class);
}
}
implements GarbageCollected<ConfigMap> {}

@KubernetesDependent(informer = @Informer(namespaces = Constants.WATCH_ALL_NAMESPACES))
public static class WatchAllNSDependent extends KubernetesDependentResource<ConfigMap, ConfigMap>
implements GarbageCollected<ConfigMap> {

public WatchAllNSDependent() {
super(ConfigMap.class);
}
}
implements GarbageCollected<ConfigMap> {}

@Workflow(dependents = @Dependent(type = OverriddenNSDependent.class))
@ControllerConfiguration(
Expand All @@ -394,10 +384,6 @@ public static class OverriddenNSDependent
implements GarbageCollected<ConfigMap> {

private static final String DEP_NS = "dependentNS";

public OverriddenNSDependent() {
super(ConfigMap.class);
}
}

@Workflow(
Expand All @@ -415,12 +401,7 @@ public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap>

private static class NamedDependentResource
extends KubernetesDependentResource<ConfigMap, ConfigMap>
implements GarbageCollected<ConfigMap> {

public NamedDependentResource() {
super(ConfigMap.class);
}
}
implements GarbageCollected<ConfigMap> {}

private static class ExternalDependentResource
implements DependentResource<Object, ConfigMap>,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,5 @@ public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap>
}

public static class TestKubernetesDependentResource
extends KubernetesDependentResource<Deployment, TestCustomResource> {

public TestKubernetesDependentResource() {
super(Deployment.class);
}
}
extends KubernetesDependentResource<Deployment, TestCustomResource> {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -144,20 +144,10 @@ public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap>
}

public static class ConfigMapDep extends KubernetesDependentResource<ConfigMap, ConfigMap>
implements GarbageCollected<ConfigMap> {

public ConfigMapDep() {
super(ConfigMap.class);
}
}
implements GarbageCollected<ConfigMap> {}

public static class ServiceDep extends KubernetesDependentResource<Service, ConfigMap>
implements GarbageCollected<ConfigMap> {

public ServiceDep() {
super(Service.class);
}
}
implements GarbageCollected<ConfigMap> {}

@CustomAnnotation(value = CustomAnnotatedDep.PROVIDED_VALUE)
@Configured(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,10 +186,6 @@ HasMetadata createPrimary(String caseName) {
private static class ServiceAccountDR
extends KubernetesDependentResource<ServiceAccount, HasMetadata> {

public ServiceAccountDR() {
super(ServiceAccount.class);
}

@Override
protected ServiceAccount desired(HasMetadata primary, Context<HasMetadata> context) {
return new ServiceAccountBuilder()
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
package io.javaoperatorsdk.operator.processing.dependent.kubernetes;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.api.model.apps.Deployment;
import io.javaoperatorsdk.operator.sample.simple.TestCustomResource;

import static io.javaoperatorsdk.operator.api.config.Utils.GENERIC_PARAMETER_TYPE_ERROR_PREFIX;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

class KubernetesDependentResourceTest {

@ParameterizedTest
@ValueSource(
classes = {
TestDeploymentDependentResource.class,
ChildTestDeploymentDependentResource.class,
GrandChildTestDeploymentDependentResource.class,
ChildTypeWithValidKubernetesDependentResource.class,
ConstructorOverridedCorrectDeployementDependentResource.class
})
void checkResourceTypeDerivationWithInheritance(Class<?> clazz) throws Exception {
KubernetesDependentResource<?, ?> dependentResource =
(KubernetesDependentResource<?, ?>) clazz.getDeclaredConstructor().newInstance();
assertThat(dependentResource).isInstanceOf(KubernetesDependentResource.class);
assertThat(dependentResource.resourceType()).isEqualTo(Deployment.class);
}

private static class TestDeploymentDependentResource
extends KubernetesDependentResource<Deployment, TestCustomResource> {}

private static class ChildTestDeploymentDependentResource
extends TestDeploymentDependentResource {}

private static class GrandChildTestDeploymentDependentResource
extends ChildTestDeploymentDependentResource {}

private static class ChildTypeWithValidKubernetesDependentResource<P, T>
extends KubernetesDependentResource<Deployment, TestCustomResource> {}

private static class ConstructorOverridedCorrectDeployementDependentResource
extends KubernetesDependentResource<Deployment, TestCustomResource> {
public ConstructorOverridedCorrectDeployementDependentResource() {
super(Deployment.class);
}
}

@Test
void validateInvalidTypeDerivationTypesThrowException() {
assertThatThrownBy(() -> new InvalidChildTestDeploymentDependentResource())
.isInstanceOf(IllegalArgumentException.class)
.hasMessage(
GENERIC_PARAMETER_TYPE_ERROR_PREFIX
+ InvalidChildTestDeploymentDependentResource.class.getName()
+ "because it doesn't extend a class that is parametrized with the type that"
+ " implements "
+ HasMetadata.class.getSimpleName()
+ ". Please provide the resource type in the constructor (e.g.,"
+ " super(Deployment.class).");
assertThatThrownBy(() -> new InvalidGrandChildTestDeploymentDependentResource())
.isInstanceOf(IllegalArgumentException.class)
.hasMessage(
GENERIC_PARAMETER_TYPE_ERROR_PREFIX
+ InvalidGrandChildTestDeploymentDependentResource.class.getName()
+ "because it doesn't extend a class that is parametrized with the type that"
+ " implements "
+ HasMetadata.class.getSimpleName()
+ ". Please provide the resource type in the constructor (e.g.,"
+ " super(Deployment.class).");
}

private static class InvalidChildTestDeploymentDependentResource
extends ChildTypeWithValidKubernetesDependentResource<Object, Object> {}

private static class InvalidGrandChildTestDeploymentDependentResource
extends InvalidChildTestDeploymentDependentResource {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,6 @@ public class UnmodifiablePartConfigMapDependent
public static final String UNMODIFIABLE_INITIAL_DATA_KEY = "initialDataKey";
public static final String ACTUAL_DATA_KEY = "actualDataKey";

public UnmodifiablePartConfigMapDependent() {
super(ConfigMap.class);
}

@Override
protected ConfigMap desired(
UnmodifiableDependentPartCustomResource primary,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -414,12 +414,7 @@ public UpdateControl<ConfigMapReader> reconcile(

@KubernetesDependent(useSSA = BooleanWithUndefined.TRUE)
public static class WithAnnotation
extends CRUDKubernetesDependentResource<ConfigMap, ConfigMapReader> {

public WithAnnotation() {
super(ConfigMap.class);
}
}
extends CRUDKubernetesDependentResource<ConfigMap, ConfigMapReader> {}
}

public static class MissingAnnotationReconciler implements Reconciler<ConfigMap> {
Expand All @@ -443,18 +438,10 @@ public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap>
}

private static class DefaultDependent
extends KubernetesDependentResource<ConfigMapReader, ConfigMap> {
public DefaultDependent() {
super(ConfigMapReader.class);
}
}
extends KubernetesDependentResource<ConfigMapReader, ConfigMap> {}

@KubernetesDependent(useSSA = BooleanWithUndefined.FALSE)
private static class NonSSADependent extends KubernetesDependentResource<Service, ConfigMap> {
public NonSSADependent() {
super(Service.class);
}
}
private static class NonSSADependent extends KubernetesDependentResource<Service, ConfigMap> {}
}

public static class TestRetry implements Retry, AnnotationConfigurable<TestRetryConfiguration> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,6 @@ public class ConfigMapDeleterBulkDependentResource
public static final String ADDITIONAL_DATA_KEY = "additionalData";
public static final String INDEX_DELIMITER = "-";

public ConfigMapDeleterBulkDependentResource() {
super(ConfigMap.class);
}

@Override
public Map<String, ConfigMap> desiredResources(
BulkDependentTestCustomResource primary, Context<BulkDependentTestCustomResource> context) {
Expand Down
Loading
Loading