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

Jump to bottom

Util to generateId, remove extra userEmail args #999

Merged
merged 8 commits into from
Sep 13, 2024
Merged

Util to generateId, remove extra userEmail args #999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6e7d3a6
Use ServiceUtils.newArtifactId(), enhanced Switch
dexamundsen Sep 12, 2024
bad9695
Fill lastModifiedBy in builder
dexamundsen Sep 12, 2024
9b91e70
Switch upgrade
dexamundsen Sep 12, 2024
db752e2
test fix
dexamundsen Sep 12, 2024
af2aa19
Condense collect.tolist()
dexamundsen Sep 12, 2024
e01f2f1
Revert "Condense collect.tolist()"
dexamundsen Sep 12, 2024
0a93c8e
Merge branch 'main' into dexamundsen/artifacts
dexamundsen Sep 13, 2024
bb91f31
Merge branch 'main' into dexamundsen/artifacts
dexamundsen Sep 13, 2024
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
2 changes: 1 addition & 1 deletion .github/workflows/regression-test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ jobs:
env:
TEST_PROJECT_SA_KEY: ${{ secrets.TEST_PROJECT_SA_KEY }}
- name: Gradle Run Regression Tests Only
run: ./gradlew service:regressionTests -PregressionTestUnderlays=cmssynpuf,aouSR2019q4r4 --info --scan
run: ./gradlew service:regressionTests -PregressionTestUnderlays=cmssynpuf,aouSR2019q4r4 --scan
dexamundsen marked this conversation as resolved.
Show resolved Hide resolved
env:
DBMS: postgresql
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
Expand Down
2 changes: 1 addition & 1 deletion .github/workflowsForDownstreamRepo/regression-test-downstream-repo.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ jobs:
mkdir -p rendered/
echo "$GHA_SA_KEY" > rendered/gha_sa_key.json
export GOOGLE_APPLICATION_CREDENTIALS=$PWD/rendered/gha_sa_key.json
./tanagra/gradlew -p tanagra service:regressionTests -PregressionTestUnderlays="${UNDERLAYS:-$DEFAULT_UNDERLAYS}" --info --scan
./tanagra/gradlew -p tanagra service:regressionTests -PregressionTestUnderlays="${UNDERLAYS:-$DEFAULT_UNDERLAYS}" --scan
dexamundsen marked this conversation as resolved.
Show resolved Hide resolved
env:
DBMS: postgresql
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
Expand Down
13 changes: 5 additions & 8 deletions indexer/src/main/java/bio/terra/tanagra/indexing/JobSequencer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,14 +497,11 @@ public enum JobExecutor {

public JobRunner getRunner(
List<SequencedJobSet> jobSets, boolean isDryRun, IndexingJob.RunType runType) {
switch (this) {
case SERIAL:
return new SerialRunner(jobSets, isDryRun, runType);
case PARALLEL:
return new ParallelRunner(jobSets, isDryRun, runType);
default:
throw new IllegalArgumentException("Unknown JobExecution enum type: " + this);
}
return switch (this) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

enhanced switch from java 17

Copy link
Collaborator

Choose a reason for hiding this comment

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

In general, it's not a good idea to make this sort of sweeping cleanup in a PR that is doing other, unrelated things. It makes this change harder to review because I don't know if any differences I come across are intended or not, and can cause cherry-picks, rollbacks, etc. to be more difficult. Don't bother pulling it out of this PR but something to keep in mind in the future.

Copy link
Contributor Author

@dexamundsen dexamundsen Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sure -- put these changes together since all of them are cleanup / optimizations. the entire PR is low priority and we could revert the entire thing. hence put them together. dao changes were left separate because that was not a cleanup, but refactored (for later use in next pr), same with snyk ones

case SERIAL -> new SerialRunner(jobSets, isDryRun, runType);
case PARALLEL -> new ParallelRunner(jobSets, isDryRun, runType);
default -> throw new IllegalArgumentException("Unknown JobExecution enum type: " + this);
};
}
}
}
80 changes: 38 additions & 42 deletions service/src/main/java/bio/terra/tanagra/app/controller/CohortsApiController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ public ResponseEntity<ApiCohort> createCohort(String studyId, ApiCohortCreateInf
Cohort.builder()
.displayName(body.getDisplayName())
.description(body.getDescription())
.underlay(body.getUnderlayName()),
SpringAuthentication.getCurrentUser().getEmail());
.underlay(body.getUnderlayName())
.createdBy(SpringAuthentication.getCurrentUser().getEmail()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Useremail is only used for this field in nested functions. Add it to builder and remove the param

Copy link
Collaborator

Choose a reason for hiding this comment

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

The user email is not just used for the cohort created by field, it's also used for the activity log.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, the activity log used cohort.getCreatedBy(), which might also be more descriptive imo

return ResponseEntity.ok(ToApiUtils.toApiObject(createdCohort));
}

Expand Down Expand Up @@ -195,36 +195,36 @@ public ResponseEntity<ApiInstanceCountList> queryCohortCounts(

private static CohortRevision.CriteriaGroupSection fromApiObject(ApiCriteriaGroupSection apiObj) {
BooleanAndOrFilter.LogicalOperator operator;
JoinOperator joinOperator;
switch (apiObj.getOperator()) {
case OR:
operator = BooleanAndOrFilter.LogicalOperator.OR;
joinOperator = null;
break;
case AND:
operator = BooleanAndOrFilter.LogicalOperator.AND;
joinOperator = null;
break;
case DURING_SAME_ENCOUNTER:
operator = BooleanAndOrFilter.LogicalOperator.OR;
joinOperator = JoinOperator.DURING_SAME_ENCOUNTER;
break;
case WITHIN_NUM_DAYS:
operator = BooleanAndOrFilter.LogicalOperator.OR;
joinOperator = JoinOperator.WITHIN_NUM_DAYS;
break;
case NUM_DAYS_BEFORE:
operator = BooleanAndOrFilter.LogicalOperator.OR;
joinOperator = JoinOperator.NUM_DAYS_BEFORE;
break;
case NUM_DAYS_AFTER:
operator = BooleanAndOrFilter.LogicalOperator.OR;
joinOperator = JoinOperator.NUM_DAYS_AFTER;
break;
default:
throw new IllegalArgumentException(
"Unknown criteria group section operator: " + apiObj.getOperator());
}
JoinOperator joinOperator =
dexamundsen marked this conversation as resolved.
Show resolved Hide resolved
switch (apiObj.getOperator()) {
case OR -> {
operator = BooleanAndOrFilter.LogicalOperator.OR;
yield null;
}
case AND -> {
operator = BooleanAndOrFilter.LogicalOperator.AND;
yield null;
}
case DURING_SAME_ENCOUNTER -> {
operator = BooleanAndOrFilter.LogicalOperator.OR;
yield JoinOperator.DURING_SAME_ENCOUNTER;
}
case WITHIN_NUM_DAYS -> {
operator = BooleanAndOrFilter.LogicalOperator.OR;
yield JoinOperator.WITHIN_NUM_DAYS;
}
case NUM_DAYS_BEFORE -> {
operator = BooleanAndOrFilter.LogicalOperator.OR;
yield JoinOperator.NUM_DAYS_BEFORE;
}
case NUM_DAYS_AFTER -> {
operator = BooleanAndOrFilter.LogicalOperator.OR;
yield JoinOperator.NUM_DAYS_AFTER;
}
default ->
throw new IllegalArgumentException(
"Unknown criteria group section operator: " + apiObj.getOperator());
};

return CohortRevision.CriteriaGroupSection.builder()
.id(apiObj.getId())
Expand Down Expand Up @@ -258,15 +258,11 @@ private static CohortRevision.CriteriaGroup fromApiObject(ApiCriteriaGroup apiOb
}

private static ReducingOperator fromApiObject(ApiReducingOperator apiObj) {
switch (apiObj) {
case ANY:
return null;
case FIRST_MENTION_OF:
return ReducingOperator.FIRST_MENTION_OF;
case LAST_MENTION_OF:
return ReducingOperator.LAST_MENTION_OF;
default:
throw new IllegalArgumentException("Unknown reducing operator: " + apiObj);
}
return switch (apiObj) {
case ANY -> null;
case FIRST_MENTION_OF -> ReducingOperator.FIRST_MENTION_OF;
case LAST_MENTION_OF -> ReducingOperator.LAST_MENTION_OF;
default -> throw new IllegalArgumentException("Unknown reducing operator: " + apiObj);
};
}
}
5 changes: 2 additions & 3 deletions service/src/main/java/bio/terra/tanagra/app/controller/ReviewsApiController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,11 +147,10 @@ public ResponseEntity<ApiReviewList> listReviews(
offset,
limit);
ApiReviewList apiReviews = new ApiReviewList();
Cohort cohort = cohortService.getCohort(studyId, cohortId);
reviewService
.listReviews(authorizedReviewIds, offset, limit)
.forEach(
review ->
apiReviews.add(toApiObject(review, cohortService.getCohort(studyId, cohortId))));
.forEach(review -> apiReviews.add(toApiObject(review, cohort)));
return ResponseEntity.ok(apiReviews);
}

Expand Down
19 changes: 7 additions & 12 deletions service/src/main/java/bio/terra/tanagra/app/controller/objmapping/FromApiUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,18 +481,13 @@ private static ValueDisplayField buildRelationshipField(
}

public static Literal fromApiObject(ApiLiteral apiLiteral) {
switch (apiLiteral.getDataType()) {
case INT64:
return Literal.forInt64(apiLiteral.getValueUnion().getInt64Val());
case STRING:
return Literal.forString(apiLiteral.getValueUnion().getStringVal());
case BOOLEAN:
return Literal.forBoolean(apiLiteral.getValueUnion().isBoolVal());
case DATE:
return Literal.forDate(apiLiteral.getValueUnion().getDateVal());
default:
throw new SystemException("Unknown API data type: " + apiLiteral.getDataType());
}
return switch (apiLiteral.getDataType()) {
case INT64 -> Literal.forInt64(apiLiteral.getValueUnion().getInt64Val());
case STRING -> Literal.forString(apiLiteral.getValueUnion().getStringVal());
case BOOLEAN -> Literal.forBoolean(apiLiteral.getValueUnion().isBoolVal());
case DATE -> Literal.forDate(apiLiteral.getValueUnion().getDateVal());
default -> throw new SystemException("Unknown API data type: " + apiLiteral.getDataType());
};
}

public static BinaryOperator fromApiObject(ApiBinaryOperator apiOperator) {
Expand Down
48 changes: 23 additions & 25 deletions service/src/main/java/bio/terra/tanagra/app/controller/objmapping/ToApiUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,29 +67,28 @@ public static ApiLiteral toApiObject(Literal literal) {

ApiLiteral apiLiteral =
new ApiLiteral().dataType(ApiDataType.fromValue(literal.getDataType().name()));
switch (literal.getDataType()) {
case INT64:
return apiLiteral.valueUnion(new ApiLiteralValueUnion().int64Val(literal.getInt64Val()));
case STRING:
return apiLiteral.valueUnion(new ApiLiteralValueUnion().stringVal(literal.getStringVal()));
case BOOLEAN:
return apiLiteral.valueUnion(new ApiLiteralValueUnion().boolVal(literal.getBooleanVal()));
case DATE:
return apiLiteral.valueUnion(
new ApiLiteralValueUnion()
.dateVal(literal.getDateVal() == null ? null : literal.getDateVal().toString()));
case TIMESTAMP:
return apiLiteral.valueUnion(
new ApiLiteralValueUnion()
.timestampVal(
literal.getTimestampVal() == null
? null
: literal.getTimestampVal().toString()));
case DOUBLE:
return apiLiteral.valueUnion(new ApiLiteralValueUnion().doubleVal(literal.getDoubleVal()));
default:
throw new SystemException("Unknown data type: " + literal.getDataType());
}
return switch (literal.getDataType()) {
case INT64 ->
apiLiteral.valueUnion(new ApiLiteralValueUnion().int64Val(literal.getInt64Val()));
case STRING ->
apiLiteral.valueUnion(new ApiLiteralValueUnion().stringVal(literal.getStringVal()));
case BOOLEAN ->
apiLiteral.valueUnion(new ApiLiteralValueUnion().boolVal(literal.getBooleanVal()));
case DATE ->
apiLiteral.valueUnion(
new ApiLiteralValueUnion()
.dateVal(literal.getDateVal() == null ? null : literal.getDateVal().toString()));
case TIMESTAMP ->
apiLiteral.valueUnion(
new ApiLiteralValueUnion()
.timestampVal(
literal.getTimestampVal() == null
? null
: literal.getTimestampVal().toString()));
case DOUBLE ->
apiLiteral.valueUnion(new ApiLiteralValueUnion().doubleVal(literal.getDoubleVal()));
default -> throw new SystemException("Unknown data type: " + literal.getDataType());
};
}

public static ApiCohort toApiObject(Cohort cohort) {
Expand Down Expand Up @@ -215,8 +214,7 @@ private static ApiInstance toApiObject(ListInstance listInstance) {
hierarchyFieldSets,
((HierarchyIsMemberField) field).getHierarchy().getName())
.setIsMember(value.getValue().getBooleanVal());
} else if (field instanceof RelatedEntityIdCountField) {
RelatedEntityIdCountField countField = (RelatedEntityIdCountField) field;
} else if (field instanceof RelatedEntityIdCountField countField) {
relationshipFieldSets.add(
new ApiInstanceRelationshipFields()
.relatedEntity(countField.getCountedEntity().getName())
Expand Down
3 changes: 0 additions & 3 deletions service/src/main/java/bio/terra/tanagra/db/CohortDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,10 +327,7 @@ public String createNextRevision(
.setIsMostRecent(true)
.version(cohort.getMostRecentRevision().getVersion() + 1)
.createdBy(userEmail)
.lastModifiedBy(userEmail)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

lastmodifiedby is set in build() during creation if its is null

.id(null) // Builder will generate a new id.
.created(null)
.lastModified(null)
dexamundsen marked this conversation as resolved.
Show resolved Hide resolved
.recordsCount(null) // Only store the records count for frozen revisions.
.build();
createRevision(cohortId, nextRevision);
Expand Down
11 changes: 11 additions & 0 deletions service/src/main/java/bio/terra/tanagra/service/ServiceUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package bio.terra.tanagra.service;

import org.apache.commons.lang3.RandomStringUtils;

public class ServiceUtils {
dexamundsen marked this conversation as resolved.
Show resolved Hide resolved
private ServiceUtils() {}

public static String newArtifactId() {
return RandomStringUtils.randomAlphanumeric(10);
}
}
11 changes: 3 additions & 8 deletions service/src/main/java/bio/terra/tanagra/service/UnderlayService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.stream.Collectors;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Component;

Expand Down Expand Up @@ -65,23 +64,19 @@ public UnderlayService(

public List<Underlay> listUnderlays(ResourceCollection authorizedIds) {
if (authorizedIds.isAllResources()) {
return underlayCache.values().stream()
.map(CachedUnderlay::getUnderlay)
.collect(Collectors.toUnmodifiableList());
dexamundsen marked this conversation as resolved.
Show resolved Hide resolved
return underlayCache.values().stream().map(CachedUnderlay::getUnderlay).toList();
} else {
// If the incoming list is empty, the caller does not have permission to see any
// underlays, so we return an empty list.
if (authorizedIds.isEmpty()) {
return Collections.emptyList();
}
List<String> authorizedNames =
authorizedIds.getResources().stream()
.map(ResourceId::getUnderlay)
.collect(Collectors.toList());
authorizedIds.getResources().stream().map(ResourceId::getUnderlay).toList();
return underlayCache.values().stream()
.map(CachedUnderlay::getUnderlay)
.filter(underlay -> authorizedNames.contains(underlay.getName()))
.collect(Collectors.toUnmodifiableList());
dexamundsen marked this conversation as resolved.
Show resolved Hide resolved
.toList();
}
}

Expand Down
Loading