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

(breaking) fix: turn on werror for all projects #63

Closed
wants to merge 7 commits into from
Closed
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
24 changes: 5 additions & 19 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ project.ext.externalDependency = [
'h2': 'com.h2database:h2:1.4.196',
'jacksonCore': 'com.fasterxml.jackson.core:jackson-core:2.9.7',
'jacksonDataBind': 'com.fasterxml.jackson.core:jackson-databind:2.9.7',
'javatuples': 'org.javatuples:javatuples:1.2',
'jsonSimple': 'com.googlecode.json-simple:json-simple:1.1.1',
'junitJupiterApi': "org.junit.jupiter:junit-jupiter-api:$junitJupiterVersion",
'junitJupiterParams': "org.junit.jupiter:junit-jupiter-params:$junitJupiterVersion",
Expand All @@ -70,30 +69,17 @@ project.ext.externalDependency = [
apply plugin: 'com.diffplug.spotless'
apply from: "./gradle/release.gradle"

// TODO expand this to all projects and then delete this allow list. This list is letting us fix errors over time rather
// than in one big change.
def wErrorProjects = [
project(':core-models'),
// project(':dao-api'),
// project(':dao-api-impl'),
// project(':restli-resources'),
project(':testing'),
project(':validators')
]

allprojects {
apply plugin: 'idea'
apply plugin: 'eclipse'
apply plugin: 'checkstyle'

gradle.projectsEvaluated {
if (wErrorProjects.contains(project)) {
tasks.withType(JavaCompile) {
options.compilerArgs << "-Xlint:all" << "-Werror" <<
"-Xlint:-deprecation" << // TODO
"-Xlint:-processing" << // TODO we have annotations like @Nonnull that need a processor
"-Xlint:-serial" // I don't think we care about having custom Exception subclasses be serializable...
}
tasks.withType(JavaCompile) {
options.compilerArgs << "-Xlint:all" << "-Werror" <<
"-Xlint:-deprecation" << // TODO
"-Xlint:-processing" << // TODO we have annotations like @Nonnull that need a processor
"-Xlint:-serial" // I don't think we care about having custom Exception subclasses be serializable...
}
}
}
Expand Down
1 change: 0 additions & 1 deletion dao-api/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ apply from: "$rootDir/gradle/java-publishing.gradle"
dependencies {
compile project(':core-models')
compile project(':validators')
compile externalDependency.javatuples
compile externalDependency.reflections
compile externalDependency.commonsLang

Expand Down
50 changes: 32 additions & 18 deletions dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java
Original file line number Diff line number Diff line change
Expand Up @@ -74,15 +74,15 @@ static class AddResult<ASPECT extends RecordTemplate> {

private static final int DEFAULT_MAX_TRANSACTION_RETRY = 3;

protected final BaseMetadataEventProducer _producer;
protected final BaseMetadataEventProducer<?, ASPECT_UNION, URN> _producer;
protected final LocalDAOStorageConfig _storageConfig;

// Maps an aspect class to the corresponding retention policy
private final Map<Class<? extends RecordTemplate>, Retention> _aspectRetentionMap = new HashMap<>();

// Maps as aspect class to a list of post-update hooks
private final Map<Class<? extends RecordTemplate>, List<BiConsumer<Urn, RecordTemplate>>> _aspectPostUpdateHooksMap =
new HashMap<>();
private final Map<Class<? extends RecordTemplate>, List<BiConsumer<URN, ? extends RecordTemplate>>>
_aspectPostUpdateHooksMap = new HashMap<>();

// Maps an aspect class to the corresponding equality tester
private final Map<Class<? extends RecordTemplate>, EqualityTester<? extends RecordTemplate>>
Expand All @@ -109,7 +109,8 @@ static class AddResult<ASPECT extends RecordTemplate> {
* com.linkedin.metadata.aspect
* @param producer {@link BaseMetadataEventProducer} for the metadata event producer
*/
public BaseLocalDAO(@Nonnull Class<ASPECT_UNION> aspectUnionClass, @Nonnull BaseMetadataEventProducer producer) {
public BaseLocalDAO(@Nonnull Class<ASPECT_UNION> aspectUnionClass,
@Nonnull BaseMetadataEventProducer<?, ASPECT_UNION, URN> producer) {
super(aspectUnionClass);
_producer = producer;
_storageConfig = LocalDAOStorageConfig.builder().build();
Expand All @@ -121,7 +122,8 @@ public BaseLocalDAO(@Nonnull Class<ASPECT_UNION> aspectUnionClass, @Nonnull Base
* @param producer {@link BaseMetadataEventProducer} for the metadata event producer
* @param storageConfig {@link LocalDAOStorageConfig} containing storage config of full list of supported aspects
*/
public BaseLocalDAO(@Nonnull BaseMetadataEventProducer producer, @Nonnull LocalDAOStorageConfig storageConfig) {
public BaseLocalDAO(@Nonnull BaseMetadataEventProducer<?, ASPECT_UNION, URN> producer,
@Nonnull LocalDAOStorageConfig storageConfig) {
super(storageConfig.getAspectStorageConfigMap().keySet());
_producer = producer;
_storageConfig = storageConfig;
Expand Down Expand Up @@ -159,20 +161,20 @@ public <ASPECT extends RecordTemplate> Retention getRetention(@Nonnull Class<ASP
* order of invocation when multiple hooks are added for a single aspect. Adding the same hook again will result in
* {@link IllegalArgumentException} thrown. Hooks are invoked in the order they're registered.
*/
public <URN extends Urn, ASPECT extends RecordTemplate> void addPostUpdateHook(@Nonnull Class<ASPECT> aspectClass,
public <ASPECT extends RecordTemplate> void addPostUpdateHook(@Nonnull Class<ASPECT> aspectClass,
@Nonnull BiConsumer<URN, ASPECT> postUpdateHook) {

checkValidAspect(aspectClass);
// TODO: Also validate Urn once we convert all aspect models to PDL with proper annotation

final List<BiConsumer<Urn, RecordTemplate>> hooks =
final List<BiConsumer<URN, ? extends RecordTemplate>> hooks =
_aspectPostUpdateHooksMap.getOrDefault(aspectClass, new LinkedList<>());

if (hooks.contains(postUpdateHook)) {
throw new IllegalArgumentException("Adding an already-registered hook");
}

hooks.add((BiConsumer<Urn, RecordTemplate>) postUpdateHook);
hooks.add(postUpdateHook);
_aspectPostUpdateHooksMap.put(aspectClass, hooks);
}

Expand All @@ -189,10 +191,11 @@ public <ASPECT extends RecordTemplate> void setEqualityTester(@Nonnull Class<ASP
* Gets the {@link EqualityTester} for an aspect, or {@link DefaultEqualityTester} if none is registered.
*/
@Nonnull
@SuppressWarnings("unchecked")
public <ASPECT extends RecordTemplate> EqualityTester<ASPECT> getEqualityTester(@Nonnull Class<ASPECT> aspectClass) {
checkValidAspect(aspectClass);
return (EqualityTester<ASPECT>) _aspectEqualityTesterMap.computeIfAbsent(aspectClass,
key -> new DefaultEqualityTester<ASPECT>());
key -> new DefaultEqualityTester<>());
}

/**
Expand Down Expand Up @@ -228,6 +231,7 @@ public void setAlwaysEmitAspectSpecificAuditEvent(boolean alwaysEmitAspectSpecif
*
* @deprecated Use {@link #enableLocalSecondaryIndex(boolean)} instead
*/
@Deprecated
public void setWriteToLocalSecondaryIndex(boolean writeToLocalSecondaryIndex) {
_enableLocalSecondaryIndex = writeToLocalSecondaryIndex;
}
Expand Down Expand Up @@ -315,7 +319,11 @@ public <ASPECT extends RecordTemplate> ASPECT add(@Nonnull URN urn, @Nonnull Cla
}
// 7. Invoke post-update hooks if there's any
if (_aspectPostUpdateHooksMap.containsKey(aspectClass)) {
_aspectPostUpdateHooksMap.get(aspectClass).forEach(hook -> hook.accept(urn, newValue));
for (BiConsumer<URN, ? extends RecordTemplate> hook : _aspectPostUpdateHooksMap.get(aspectClass)) {
@SuppressWarnings("unchecked")
final BiConsumer<URN, ASPECT> typedHook = ((BiConsumer<URN, ASPECT>) hook);
typedHook.accept(urn, newValue);
}
}

return newValue;
Expand All @@ -334,6 +342,7 @@ public <ASPECT extends RecordTemplate> ASPECT add(@Nonnull URN urn, @Nonnull Cla
* Similar to {@link #add(Urn, Class, Function, AuditStamp)} but takes the new value directly.
*/
@Nonnull
@SuppressWarnings("unchecked")
public <ASPECT extends RecordTemplate> ASPECT add(@Nonnull URN urn, @Nonnull ASPECT newValue,
@Nonnull AuditStamp auditStamp) {
return add(urn, (Class<ASPECT>) newValue.getClass(), ignored -> newValue, auditStamp);
Expand Down Expand Up @@ -378,8 +387,8 @@ protected abstract <ASPECT extends RecordTemplate> long saveLatest(@Nonnull URN
* @param newValue {@link RecordTemplate} of the new value of aspect
* @param version version of the aspect
*/
protected abstract <ASPECT extends RecordTemplate> void updateLocalIndex(@Nonnull URN urn,
@Nullable ASPECT newValue, long version);
protected abstract <ASPECT extends RecordTemplate> void updateLocalIndex(@Nonnull URN urn, @Nullable ASPECT newValue,
long version);

/**
* Returns list of urns from local secondary index that satisfy the given filter conditions.
Expand All @@ -399,8 +408,8 @@ protected abstract <ASPECT extends RecordTemplate> void updateLocalIndex(@Nonnul
*/
@Nonnull
public List<URN> listUrns(@Nonnull Class<URN> urnClazz, @Nullable URN lastUrn, int pageSize) {
final IndexFilter indexFilter = new IndexFilter()
.setCriteria(new IndexCriterionArray(new IndexCriterion().setAspect(urnClazz.getCanonicalName())));
final IndexFilter indexFilter = new IndexFilter().setCriteria(
new IndexCriterionArray(new IndexCriterion().setAspect(urnClazz.getCanonicalName())));
return listUrns(indexFilter, lastUrn, pageSize);
}

Expand Down Expand Up @@ -519,8 +528,10 @@ protected abstract <ASPECT extends RecordTemplate> void applyTimeBasedRetention(
* @return backfilled aspect
* @deprecated Use {@link #backfill(Set, Set)} instead
*/
@Deprecated
@Nonnull
public <ASPECT extends RecordTemplate> Optional<ASPECT> backfill(@Nonnull Class<ASPECT> aspectClass, @Nonnull URN urn) {
public <ASPECT extends RecordTemplate> Optional<ASPECT> backfill(@Nonnull Class<ASPECT> aspectClass,
@Nonnull URN urn) {
return backfill(BackfillMode.BACKFILL_ALL, aspectClass, urn);
}

Expand Down Expand Up @@ -560,7 +571,8 @@ public Map<URN, Map<Class<? extends RecordTemplate>, Optional<? extends RecordTe
private Map<URN, Map<Class<? extends RecordTemplate>, Optional<? extends RecordTemplate>>> backfill(
@Nonnull BackfillMode mode, @Nonnull Set<Class<? extends RecordTemplate>> aspectClasses, @Nonnull Set<URN> urns) {
checkValidAspects(aspectClasses);
final Map<URN, Map<Class<? extends RecordTemplate>, Optional<? extends RecordTemplate>>> urnToAspects = get(aspectClasses, urns);
final Map<URN, Map<Class<? extends RecordTemplate>, Optional<? extends RecordTemplate>>> urnToAspects =
get(aspectClasses, urns);
urnToAspects.forEach((urn, aspects) -> {
aspects.forEach((aspectClass, aspect) -> aspect.ifPresent(value -> backfill(mode, value, urn)));
});
Expand All @@ -584,7 +596,7 @@ public Map<URN, Map<Class<? extends RecordTemplate>, Optional<? extends RecordTe
@Nonnull Class<URN> urnClazz, @Nullable URN lastUrn, int pageSize) {

final List<URN> urnList = listUrns(urnClazz, lastUrn, pageSize);
return backfill(mode, aspectClasses, new HashSet(urnList));
return backfill(mode, aspectClasses, new HashSet<>(urnList));
}

/**
Expand All @@ -595,7 +607,8 @@ public Map<URN, Map<Class<? extends RecordTemplate>, Optional<? extends RecordTe
* @param urn {@link Urn} for the entity
* @param <ASPECT> must be a supported aspect type in {@code ASPECT_UNION}.
*/
private <ASPECT extends RecordTemplate> void backfill(@Nonnull BackfillMode mode, @Nonnull ASPECT aspect, @Nonnull URN urn) {
private <ASPECT extends RecordTemplate> void backfill(@Nonnull BackfillMode mode, @Nonnull ASPECT aspect,
@Nonnull URN urn) {
if (_enableLocalSecondaryIndex && (mode == BackfillMode.SCSI_ONLY || mode == BackfillMode.BACKFILL_ALL)) {
updateLocalIndex(urn, aspect, FIRST_VERSION);
}
Expand Down Expand Up @@ -687,6 +700,7 @@ public abstract <ASPECT extends RecordTemplate> ListResult<ASPECT> list(@Nonnull
* Similar to {@link #getWithExtraInfo(Set)} but only using only one {@link AspectKey}.
*/
@Nonnull
@SuppressWarnings("unchecked")
public <ASPECT extends RecordTemplate> Optional<AspectWithExtraInfo<ASPECT>> getWithExtraInfo(
@Nonnull AspectKey<URN, ASPECT> key) {
if (getWithExtraInfo(Collections.singleton(key)).containsKey(key)) {
Expand Down
44 changes: 32 additions & 12 deletions dao-api/src/main/java/com/linkedin/metadata/dao/BaseQueryDAO.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
import java.util.List;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import org.javatuples.Triplet;
import lombok.AllArgsConstructor;
import lombok.Getter;

import static com.linkedin.metadata.dao.utils.QueryUtils.*;

Expand Down Expand Up @@ -112,28 +113,47 @@ List<RecordTemplate> findEntities(
@Nonnull Class<RELATIONSHIP> relationshipType, @Nonnull RelationshipFilter relationshipFilter, int minHops,
int maxHops, int offset, int count);

/**
* A path to visit.
*/
@Getter
@AllArgsConstructor
public static final class TraversalPath {
/**
* Relationship type on the traverse path.
*
* <p>Must be a type defined in com.linkedin.metadata.relationship.
*/
private final Class<? extends RecordTemplate> relationship;

/**
* The relationship filter to apply in a graph query.
*/
private final RelationshipFilter relationshipFilter;

/**
* Intermediate entity type on the traverse path.
*
* <p>Must be a type defined in com.linkedin.metadata.entity.
*/
private final Class<? extends RecordTemplate> intermediateEntity;
}

/**
* Finds a list of entities based on the given traversing paths.
*
* @param sourceEntityClass the source entity class as the starting point for the query
* @param sourceEntityClass the source entity class as the starting point for the query. Must be a type defined in
* com.linkedin.metadata.entity.
* @param sourceEntityFilter the filter to apply to the source entity when querying
* @param traversePaths specify the traverse paths via a list of (relationship type, relationship filter,
* intermediate entities)
* @param count the maximum number of entities to return. Ignored if set to a non-positive value.
*
* @param <SRC_ENTITY> source ENTITY type. Starting point of the traverse path. Must be a type defined in
* com.linkedin.metadata.entity.
* @param <INTER_ENTITY> intermediate entity type on the traverse path. Must be a type defined in
* com.linkedin.metadata.entity.
* @param <RELATIONSHIP> relationship type on the traverse path. Must be a type defined in
* com.linkedin.metadata.relationship.
* @return a list of entities that match the conditions specified in {@code filter}
*/
@Nonnull
public abstract <SRC_ENTITY extends RecordTemplate, RELATIONSHIP extends RecordTemplate, INTER_ENTITY extends RecordTemplate>
List<RecordTemplate> findEntities(
@Nullable Class<SRC_ENTITY> sourceEntityClass, @Nonnull Filter sourceEntityFilter,
@Nonnull List<Triplet<Class<RELATIONSHIP>, RelationshipFilter, Class<INTER_ENTITY>>> traversePaths, int offset, int count);
public abstract List<RecordTemplate> findEntities(@Nullable Class<? extends RecordTemplate> sourceEntityClass,
@Nonnull Filter sourceEntityFilter, @Nonnull List<TraversalPath> traversePaths, int offset, int count);

/**
* Finds a list of relationships of a specific type based on the given relationship filter and source entity filter.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ public BaseReadDAO(@Nonnull Set<Class<? extends RecordTemplate>> aspects) {
* Similar to {@link #get(Set)} but only using only one {@link AspectKey}.
*/
@Nonnull
@SuppressWarnings("unchecked")
public <ASPECT extends RecordTemplate> Optional<ASPECT> get(@Nonnull AspectKey<URN, ASPECT> key) {
return (Optional<ASPECT>) get(Collections.singleton(key)).get(key);
}
Expand Down Expand Up @@ -116,6 +117,7 @@ public Map<Class<? extends RecordTemplate>, Optional<? extends RecordTemplate>>
* Similar to {@link #get(Set, Set)} but only for one aspect.
*/
@Nonnull
@SuppressWarnings("unchecked")
public <ASPECT extends RecordTemplate> Map<URN, Optional<ASPECT>> get(
@Nonnull Class<ASPECT> aspectClass, @Nonnull Set<URN> urns) {

Expand Down
Original file line number Diff line number Diff line change
@@ -1,23 +1,25 @@
package com.linkedin.metadata.dao.equality;

import com.linkedin.data.template.DataTemplate;
import com.linkedin.data.template.RecordTemplate;
import javax.annotation.Nonnull;


/**
* A {@link EqualityTester} that always returns false.
*/
public class AlwaysFalseEqualityTester<T extends DataTemplate> implements EqualityTester<T> {
public class AlwaysFalseEqualityTester<T extends RecordTemplate> implements EqualityTester<T> {
private static final AlwaysFalseEqualityTester<?> INSTANCE = new AlwaysFalseEqualityTester<>();

/**
* Creates a new instance of {@link AlwaysFalseEqualityTester}.
* Returns the singleton instance of {@link AlwaysFalseEqualityTester}.
*/
public static <CLASS extends DataTemplate> AlwaysFalseEqualityTester<CLASS> newInstance() {
return new AlwaysFalseEqualityTester<>();
@SuppressWarnings("unchecked")
public static <T extends RecordTemplate> AlwaysFalseEqualityTester<T> instance() {
return (AlwaysFalseEqualityTester<T>) INSTANCE;
}

@Override
public boolean equals(@Nonnull T o1, @Nonnull T o2) {
public boolean equals(@Nonnull RecordTemplate o1, @Nonnull RecordTemplate o2) {
return false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,23 @@

import com.linkedin.data.template.DataTemplate;
import com.linkedin.data.template.DataTemplateUtil;
import com.linkedin.data.template.RecordTemplate;
import javax.annotation.Nonnull;


/**
* A {@link EqualityTester} that uses {@link DataTemplateUtil#areEqual(DataTemplate, DataTemplate)} to check for
* semantic equality.
*/
public class DefaultEqualityTester<T extends DataTemplate> implements EqualityTester<T> {
public class DefaultEqualityTester<T extends RecordTemplate> implements EqualityTester<T> {
private static final DefaultEqualityTester<?> INSTANCE = new DefaultEqualityTester<>();

/**
* Creates a new instance of {@link DefaultEqualityTester}.
* Returns the singleton instance of {@link DefaultEqualityTester}.
*/
public static <CLASS extends DataTemplate> DefaultEqualityTester<CLASS> newInstance() {
return new DefaultEqualityTester<>();
@SuppressWarnings("unchecked")
public static <T extends RecordTemplate> DefaultEqualityTester<T> instance() {
return (DefaultEqualityTester<T>) INSTANCE;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
package com.linkedin.metadata.dao.equality;

import com.linkedin.data.template.DataTemplate;
import com.linkedin.data.template.RecordTemplate;
import javax.annotation.Nonnull;


/**
* An interface for testing equality between two objects of the same type.
*/
public interface EqualityTester<T extends DataTemplate> {
public interface EqualityTester<T extends RecordTemplate> {

boolean equals(@Nonnull T o1, @Nonnull T o2);
}
Loading