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.

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

Bug fix for TEF exclusion not honored #17

Open
wants to merge 4 commits 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
4 changes: 2 additions & 2 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,9 @@
</build>

<properties>
<revision>0.1.5</revision>
<revision>0.2.0</revision>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be a minor version bump, since its a bugfix. Refer to semver for guidance.

Copy link
Author

Choose a reason for hiding this comment

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

This is also JDK17 compliant change.
Hence the bump to major version seemed relevant.

<guava.version>19.0</guava.version>
<guice.version>4.2.3</guice.version>
<guice.version>5.1.0</guice.version>
</properties>

<dependencies>
Expand Down
13 changes: 8 additions & 5 deletions tef-impl/src/main/java/flipkart/tef/execution/FlowBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
import flipkart.tef.capability.AdapterConflictRuntimeException;
import flipkart.tef.exceptions.UnableToResolveDataFromAdapterRuntimeException;
import flipkart.tef.flow.SimpleFlow;
import sun.reflect.generics.reflectiveObjects.ParameterizedTypeImpl;
import java.lang.reflect.ParameterizedType;

import java.lang.reflect.Field;
import java.util.ArrayList;
Expand Down Expand Up @@ -168,6 +168,9 @@ SimpleFlow build() {
for (Class<? extends IBizlogic> e : excludedBizlogics) {
bizlogics.remove(e);
bizlogicDependencyMap.removeAll(e);
if (reverseBizlogicDependencyMap.containsValue(e)){
reverseBizlogicDependencyMap.entries().removeIf(entry -> entry.getValue().equals(e));
}
}

int idx = 0;
Expand Down Expand Up @@ -293,13 +296,13 @@ private void populateDataAdapterMap(Class<? extends IBizlogic> bizlogic) {
@SuppressWarnings("unchecked")
private Class<?> getReturnTypeFromBizlogicUsingSunApi(Class<? extends DataAdapterBizlogic<?>> dataAdapterBizLogic, List<Class<? extends DataAdapterBizlogic<?>>> classHierarchy) {
classHierarchy.add(dataAdapterBizLogic);
if (dataAdapterBizLogic.getGenericSuperclass() instanceof ParameterizedTypeImpl) {
ParameterizedTypeImpl genericSuperClass = (ParameterizedTypeImpl) dataAdapterBizLogic.getGenericSuperclass();
if (dataAdapterBizLogic.getGenericSuperclass() instanceof ParameterizedType) {
ParameterizedType genericSuperClass = (ParameterizedType) dataAdapterBizLogic.getGenericSuperclass();
if (genericSuperClass.getActualTypeArguments()[0] instanceof Class) {
return (Class<?>) genericSuperClass.getActualTypeArguments()[0];
} else if (genericSuperClass.getActualTypeArguments()[0] instanceof ParameterizedTypeImpl) {
} else if (genericSuperClass.getActualTypeArguments()[0] instanceof ParameterizedType) {
// The type itself is parameterized
return ((ParameterizedTypeImpl) genericSuperClass.getActualTypeArguments()[0]).getRawType();
return (Class<?>)((ParameterizedType) genericSuperClass.getActualTypeArguments()[0]).getRawType();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this additional cast to Class<?> needed?

}
} else {
// This could be a case of a data adapter being a subclass of another
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import org.junit.BeforeClass;
import org.junit.Test;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

Expand Down Expand Up @@ -163,6 +164,62 @@ public List<Class<? extends IBizlogic>> exclusions() {

}

@Test
public void testThatExclusionIsHonoredWhenControlDependencyIsExcluded() throws Exception {

CapabilityDefinition capabilityDefinition = new EmptyCapabilityDefinition() {
@Override
public String name() {
return "C1";
}

@Override
public List<? extends CapabilityDefinition> dependentCapabilities() {
return Collections.emptyList();
}

@Override
public List<Class<? extends BasicValidationBizlogic>> validators() {
return ImmutableList.of(BasicValidationBizlogic1.class);
}

@Override
public List<Class<? extends BasicEnrichmentBizlogic>> enrichers() {
return ImmutableList.of(BasicEnrichmentBizlogic1.class);
}

@Override
public List<Class<? extends DataAdapterBizlogic>> adapters() {
return ImmutableList.of(DataAdapterBizlogic1.class);
}

@Override
public List<Class<? extends IBizlogic>> exclusions() {
return ImmutableList.of(DataAdapterBizlogic2.class);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given adapter2 was never added to the flow from any place, this exclusion does not test things properly. I think you should add DataAdapterBizlogic2 in adapters

}

@Override
public List<BizlogicDependency> bizlogicDependencies() {
List<BizlogicDependency> list = new ArrayList<>();
// Adding adapter 2 on adapter 1
list.add(new BizlogicDependency(DataAdapterBizlogic2.class, new Class[]{DataAdapterBizlogic1.class, DataAdapterBizlogic3.class}));
return list;
}
};

FluentCapabilityBuilder manager = new FluentCapabilityBuilder();
SimpleFlow flow = manager.withCapability(capabilityDefinition).dataflow();

assertNotNull(flow);

assertEquals(4, flow.getBizlogics().size());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you also assert on the order of bizlogics?

assertTrue(flow.getBizlogics().contains(BasicValidationBizlogic1.class));
assertTrue(flow.getBizlogics().contains(BasicEnrichmentBizlogic1.class));
assertTrue(flow.getBizlogics().contains(DataAdapterBizlogic1.class));
assertTrue(flow.getBizlogics().contains(DataAdapterBizlogic3.class));

}

class BasicValidationBizlogic1 extends BasicValidationBizlogic {

@Override
Expand Down Expand Up @@ -206,4 +263,20 @@ public Object adapt(TefContext tefContext) {
return null;
}
}

class DataAdapterBizlogic2 extends DataAdapterBizlogic<Object> {

@Override
public Object adapt(TefContext tefContext) {
return null;
}
}

class DataAdapterBizlogic3 extends DataAdapterBizlogic<String> {

@Override
public String adapt(TefContext tefContext) {
return null;
}
}
}