Skip to content

Commit

Permalink
[OPIK-709] fix NPE in find projects (#984)
Browse files Browse the repository at this point in the history 
* OPIK-709 test reproducing the issue

* OPIK-709 test reproducing the issue green

* OPIK-709 naming

* OPIK-709 fix tests
  • Loading branch information
@idoberko2
idoberko2 authored Jan 7, 2025
1 parent 8c01dcd commit f059d21
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 18 deletions.
6 changes: 6 additions & 0 deletions apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import lombok.NonNull;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.collections4.CollectionUtils;
import org.jdbi.v3.core.statement.UnableToExecuteStatementException;
import ru.vyarus.guicey.jdbi3.tx.TransactionTemplate;

Expand Down Expand Up @@ -354,6 +355,11 @@ private Page<Project> findWithLastTraceSorting(int page, int size, @NonNull Proj
List<UUID> sorted = sortByLastTrace(allProjectIdsLastUpdated, projectLastUpdatedTraceAtMap, sortingField);
List<UUID> finalIds = PaginationUtils.paginate(page, size, sorted);

if (CollectionUtils.isEmpty(finalIds)) {
// pagination might return an empty list
return ProjectPage.empty(page);
}

// get all project properties for the final list of ids
Map<UUID, Project> projectsById = template.inTransaction(READ_ONLY, handle -> {
ProjectDAO repository = handle.attach(ProjectDAO.class);
Expand Down
59 changes: 41 additions & 18 deletions ...opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ProjectsResourceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -857,21 +857,25 @@ void getProjects__whenSortingProjectsByLastTrace__thenReturnProjectSorted(Direct

mockTargetWorkspace(apiKey, workspaceName, workspaceId);

List<Project> projects = PodamFactoryUtils.manufacturePojoList(factory, Project.class);
List<Project> projects = createProjectsWithLastTrace(apiKey, workspaceName);

projects = projects.stream().map(project -> {
UUID projectId = createProject(project, apiKey, workspaceName);
List<UUID> traceIds = IntStream.range(0, 5)
.mapToObj(i -> createCreateTrace(project.name(), apiKey, workspaceName))
.toList();
requestAndAssertLastTraceSorting(workspaceName, apiKey, projects, request, expected, 1, projects.size());
}

Trace trace = getTrace(traceIds.getLast(), apiKey, workspaceName);
return project.toBuilder()
.id(projectId)
.lastUpdatedTraceAt(trace.lastUpdatedAt()).build();
}).toList();
@Test
@DisplayName("when fetching all project with last trace sorting and out of range pagination, then return empty list")
void getProjects__whenSortingProjectsByLastTraceWithPagination__thenReturnEmptyList() {
final int OUT_OF_RANGE_PAGE = 3;
String workspaceName = UUID.randomUUID().toString();
String apiKey = UUID.randomUUID().toString();
String workspaceId = UUID.randomUUID().toString();

mockTargetWorkspace(apiKey, workspaceName, workspaceId);

requestAndAssertLastTraceSorting(workspaceName, apiKey, projects, request, expected);
List<Project> projects = createProjectsWithLastTrace(apiKey, workspaceName);

requestAndAssertLastTraceSorting(workspaceName, apiKey, List.of(), Direction.DESC, Direction.DESC,
OUT_OF_RANGE_PAGE, projects.size());
}

@ParameterizedTest
Expand Down Expand Up @@ -906,9 +910,10 @@ void getProjects__whenSortingProjectsByLastTraceAndNoTraceExists__thenReturnProj
return project.toBuilder().id(projectId).build();
}).toList();

List<Project> allProjects = Stream.concat(withTraceProjects.stream(), noTraceProjects.stream()).toList();

requestAndAssertLastTraceSorting(
workspaceName, apiKey, Stream.concat(withTraceProjects.stream(), noTraceProjects.stream()).toList(),
request, expected);
workspaceName, apiKey, allProjects, request, expected, 1, allProjects.size());
}

public static Stream<Arguments> sortDirectionProvider() {
Expand All @@ -918,6 +923,22 @@ public static Stream<Arguments> sortDirectionProvider() {
Arguments.of(Named.of("descending", Direction.DESC), Direction.DESC));
}

private List<Project> createProjectsWithLastTrace(String apiKey, String workspaceName) {
List<Project> projects = PodamFactoryUtils.manufacturePojoList(factory, Project.class);

return projects.stream().map(project -> {
UUID projectId = createProject(project, apiKey, workspaceName);
List<UUID> traceIds = IntStream.range(0, 5)
.mapToObj(i -> createCreateTrace(project.name(), apiKey, workspaceName))
.toList();

Trace trace = getTrace(traceIds.getLast(), apiKey, workspaceName);
return project.toBuilder()
.id(projectId)
.lastUpdatedTraceAt(trace.lastUpdatedAt()).build();
}).toList();
}

@ParameterizedTest
@MethodSource
@DisplayName("sort by non-sortable field should return an error")
Expand Down Expand Up @@ -1594,14 +1615,15 @@ private void assertProject(Project project, String apiKey, String workspaceName)
}

private void requestAndAssertLastTraceSorting(String workspaceName, String apiKey, List<Project> allProjects,
Direction request, Direction expected) {
Direction request, Direction expected, int page, int size) {
var sorting = List.of(SortingField.builder()
.field(SortableFields.LAST_UPDATED_TRACE_AT)
.direction(request)
.build());

var actualResponse = client.target(URL_TEMPLATE.formatted(baseURI))
.queryParam("size", allProjects.size())
.queryParam("size", size)
.queryParam("page", page)
.queryParam("sorting", URLEncoder.encode(JsonUtils.writeValueAsString(sorting),
StandardCharsets.UTF_8))
.request()
Expand All @@ -1614,7 +1636,7 @@ private void requestAndAssertLastTraceSorting(String workspaceName, String apiKe
assertThat(actualResponse.getStatusInfo().getStatusCode()).isEqualTo(200);
assertThat(actualEntity.size()).isEqualTo(allProjects.size());
assertThat(actualEntity.total()).isEqualTo(allProjects.size());
assertThat(actualEntity.page()).isEqualTo(1);
assertThat(actualEntity.page()).isEqualTo(page);

if (expected == Direction.DESC) {
allProjects = allProjects.reversed();
Expand Down Expand Up @@ -2087,7 +2109,8 @@ void findFeedbackScoreNames(boolean userProjectId) {
// Create unexpected feedback scores
String unexpectedProjectName = UUID.randomUUID().toString();

UUID unexpectedProjectId = projectResourceClient.createProject(unexpectedProjectName, apiKey, workspaceName);
UUID unexpectedProjectId = projectResourceClient.createProject(unexpectedProjectName, apiKey,
workspaceName);
Project unexpectedProject = projectResourceClient.getProject(unexpectedProjectId, apiKey, workspaceName);

traceResourceClient.createMultiValueScores(otherNames, unexpectedProject,
Expand Down

0 comments on commit f059d21

Please sign in to comment.