Skip to content
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 @@ -6,6 +6,7 @@
import lombok.NoArgsConstructor;

import java.time.Instant;
import java.util.Set;

/**
* Internal DTO for organization filter options.
Expand All @@ -22,4 +23,5 @@ public class OrganizationFilterOptions {
private String status;
private Instant lastActivityFrom;
private Instant lastActivityTo;
private Set<String> excludeOrganizationIds;
}
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ private OrganizationQueryFilter buildQueryFilter(OrganizationFilterOptions filte
.status(filterOptions.getStatus())
.lastActivityFrom(filterOptions.getLastActivityFrom())
.lastActivityTo(filterOptions.getLastActivityTo())
.excludeOrganizationIds(filterOptions.getExcludeOrganizationIds())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's the idea of this filter?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We need to return a list of customers that are not assigned to a cloud provider to the FE

.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import java.time.Instant;
import java.util.List;
import java.util.Set;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.any;
Expand Down Expand Up @@ -125,6 +126,26 @@ void lastActivityRangeIsForwardedToRepository() {
assertThat(forwarded.getLastActivityTo()).isEqualTo(to);
}

@Test
@DisplayName("excludeOrganizationIds on the FilterOptions must reach the repository — the directory connection picker relies on it to hide customers that already have a connection")
void excludeOrganizationIdsIsForwardedToRepository() {
when(repository.countOrganizations(any())).thenReturn(0L);
when(repository.findOrganizationsWithCursor(any(), any(), anyInt(), any(), any()))
.thenReturn(List.of());

Set<String> excluded = Set.of("org-a", "org-b");
OrganizationFilterOptions options = OrganizationFilterOptions.builder()
.excludeOrganizationIds(excluded)
.build();

service.queryOrganizations(options, page(20), null, lastActivity(SortDirection.DESC));

ArgumentCaptor<OrganizationQueryFilter> captor = ArgumentCaptor.forClass(OrganizationQueryFilter.class);
verify(repository).buildOrganizationQuery(captor.capture(), any());
OrganizationQueryFilter forwarded = captor.getValue();
assertThat(forwarded.getExcludeOrganizationIds()).containsExactlyInAnyOrderElementsOf(excluded);
}

@Test
@DisplayName("legacy _id sort keeps a plain ObjectId cursor")
void plainCursorForLegacyIdSort() {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.openframe.api.datafetcher;

import com.netflix.graphql.dgs.*;
import com.openframe.api.dataloader.OrganizationDataLoader;
import com.openframe.api.dto.CountedGenericConnection;
import com.openframe.api.dto.CountedGenericQueryResult;
import com.openframe.api.dto.GenericEdge;
Expand Down Expand Up @@ -111,7 +112,7 @@ public CompletableFuture<?> resolveTarget(DgsDataFetchingEnvironment dfe) {
ItemAssignment assignment = dfe.getSource();
String targetId = assignment.getTargetId();
return switch (assignment.getTargetType()) {
case ORGANIZATION -> dfe.<String, Organization>getDataLoader("organizationDataLoader").load(targetId);
case ORGANIZATION -> dfe.<String, Organization>getDataLoader(OrganizationDataLoader.NAME).load(targetId);
case DEVICE -> dfe.<String, Machine>getDataLoader("machineDataLoader").load(targetId);
case TICKET -> dfe.<String, Ticket>getDataLoader("ticketDataLoader").load(targetId);
case KNOWLEDGE_ARTICLE -> dfe.<String, KnowledgeBaseItem>getDataLoader("knowledgeBaseItemDataLoader").load(targetId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import com.netflix.graphql.dgs.DgsMutation;
import com.netflix.graphql.dgs.DgsQuery;
import com.netflix.graphql.dgs.InputArgument;
import com.openframe.api.dataloader.OrganizationDataLoader;
import com.openframe.api.dto.CountedGenericConnection;
import com.openframe.api.dto.CountedGenericQueryResult;
import com.openframe.api.dto.GenericEdge;
Expand Down Expand Up @@ -181,7 +182,7 @@ public CompletableFuture<List<InstalledAgent>> installedAgents(DgsDataFetchingEn

@DgsData(parentType = "Machine")
public CompletableFuture<Organization> organization(DgsDataFetchingEnvironment dfe) {
DataLoader<String, Organization> dataLoader = dfe.getDataLoader("organizationDataLoader");
DataLoader<String, Organization> dataLoader = dfe.getDataLoader(OrganizationDataLoader.NAME);
Machine machine = dfe.getSource();
String organizationId = machine.getOrganizationId();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import com.netflix.graphql.dgs.DgsMutation;
import com.netflix.graphql.dgs.DgsQuery;
import com.netflix.graphql.dgs.InputArgument;
import com.openframe.api.dataloader.OrganizationDataLoader;
import com.openframe.api.dto.CountedGenericConnection;
import com.openframe.api.dto.CountedGenericQueryResult;
import com.openframe.api.dto.GenericEdge;
Expand Down Expand Up @@ -228,7 +229,7 @@ public CompletableFuture<Organization> timeEntryOrganization(DgsDataFetchingEnvi
if (entry.getOrganizationId() == null) {
return CompletableFuture.completedFuture(null);
}
DataLoader<String, Organization> loader = dfe.getDataLoader("organizationDataLoader");
DataLoader<String, Organization> loader = dfe.getDataLoader(OrganizationDataLoader.NAME);
return loader.load(entry.getOrganizationId());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@
import lombok.RequiredArgsConstructor;
import org.dataloader.BatchLoader;

import java.util.*;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionStage;
import java.util.stream.Collectors;
Expand All @@ -15,10 +18,12 @@
* DataLoader for batch loading Organization objects by organizationId.
* This prevents N+1 query problems when loading organizations for multiple machines.
*/
@DgsDataLoader(name = "organizationDataLoader")
@DgsDataLoader(name = OrganizationDataLoader.NAME)
@RequiredArgsConstructor
public class OrganizationDataLoader implements BatchLoader<String, Organization> {

public static final String NAME = "organizationDataLoader";

private final OrganizationRepository organizationRepository;

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import lombok.NoArgsConstructor;

import java.time.Instant;
import java.util.Set;

/**
* Filter criteria for organization queries.
Expand All @@ -23,4 +24,5 @@ public class OrganizationQueryFilter {
private String status;
private Instant lastActivityFrom;
private Instant lastActivityTo;
private Set<String> excludeOrganizationIds;
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import java.time.LocalDate;
import java.util.ArrayList;
import java.util.List;
import java.util.Set;
import java.util.regex.Pattern;

/**
Expand All @@ -30,6 +31,7 @@ public class CustomOrganizationRepositoryImpl implements CustomOrganizationRepos
private static final String ID_FIELD = "_id";
private static final String UPDATED_AT_FIELD = "updatedAt";
private static final String CURSOR_SEPARATOR = "_";
private static final String ORGANIZATION_ID_FIELD = "organizationId";

private static final List<String> SORTABLE_FIELDS = List.of(
"_id",
Expand Down Expand Up @@ -100,6 +102,11 @@ public Query buildOrganizationQuery(OrganizationQueryFilter filter, String searc
if (filter.getLastActivityTo() != null) {
criteriaList.add(Criteria.where(UPDATED_AT_FIELD).lte(filter.getLastActivityTo()));
}

Set<String> excludedOrganizationIds = filter.getExcludeOrganizationIds();
if (excludedOrganizationIds != null && !excludedOrganizationIds.isEmpty()) {
criteriaList.add(Criteria.where(ORGANIZATION_ID_FIELD).nin(excludedOrganizationIds));
}
Comment on lines +106 to +109

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🦩 🟠 [warn/recommended] excludeOrganizationIds filters on organizationId while other filters/sort use _id and other fields — verify field exists and is indexed

The new exclude filter is applied via Criteria.where(ORGANIZATION_ID_FIELD).nin(excludedOrganizationIds), i.e. against the document's organizationId field. Elsewhere in this same class the default/status/category/employee filters are applied directly on the Organization document fields (status, category, numberOfEmployees) and the default sort/cursor field is _id. Callers building the exclude set (e.g. from ToolConnection or existing connections) need to make sure they are populating organizationId values (the business identifier) and not Mongo _id values — passing _id strings into this set would silently produce a no-op filter since the field being compared is organizationId, not _id. There is no validation or documentation in the DTO indicating which id type is expected. Consider naming the field more explicitly (e.g. excludeByOrganizationId) or adding a comment on OrganizationFilterOptions.excludeOrganizationIds and OrganizationQueryFilter.excludeOrganizationIds clarifying it expects the business organizationId, not the Mongo document _id, and add an index recommendation since organizationId is now used in a $nin clause alongside other filters that may run frequently on the customers connection-picker path.

Evidence
            Set<String> excludedOrganizationIds = filter.getExcludeOrganizationIds();
            if (excludedOrganizationIds != null && !excludedOrganizationIds.isEmpty()) {
                criteriaList.add(Criteria.where(ORGANIZATION_ID_FIELD).nin(excludedOrganizationIds));
            }
🤖 Prompt for AI agents
In openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/organization/CustomOrganizationRepositoryImpl.java around lines 106-109, address this code-review finding: excludeOrganizationIds filters on organizationId while other filters/sort use _id and other fields — verify field exists and is indexed.
The new exclude filter is applied via Criteria.where(ORGANIZATION_ID_FIELD).nin(excludedOrganizationIds), i.e. against the document's `organizationId` field. Elsewhere in this same class the default/status/category/employee filters are applied directly on the Organization document fields (status, category, numberOfEmployees) and the default sort/cursor field is `_id`. Callers building the exclude set (e.g. from ToolConnection or existing connections) need to make sure they are populating `organizationId` values (the business identifier) and not Mongo `_id` values — passing `_id` strings into this set would silently produce a no-op filter since the field being compared is `organizationId`, not `_id`. There is no validation or documentation in the DTO indicating which id type is expected. Consider naming the field more explicitly (e.g. `excludeByOrganizationId`) or adding a comment on `OrganizationFilterOptions.excludeOrganizationIds` and `OrganizationQueryFilter.excludeOrganizationIds` clarifying it expects the business `organizationId`, not the Mongo document `_id`, and add an index recommendation since `organizationId` is now used in a $nin clause alongside other filters that may run frequently on the customers connection-picker path.
The flagged code:
```
            Set<String> excludedOrganizationIds = filter.getExcludeOrganizationIds();
            if (excludedOrganizationIds != null && !excludedOrganizationIds.isEmpty()) {
                criteriaList.add(Criteria.where(ORGANIZATION_ID_FIELD).nin(excludedOrganizationIds));
            }
```
Make the minimal change that resolves the finding; do not refactor unrelated code.

confidence: 35 — react 👍/👎 to teach the reviewer

} else {
// No filter provided — default to ACTIVE status
criteriaList.add(new Criteria().orOperator(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import java.time.Instant;
import java.util.ArrayList;
import java.util.List;
import java.util.Set;

import static org.assertj.core.api.Assertions.assertThat;

Expand Down Expand Up @@ -192,6 +193,58 @@ void countReturnsFilteredTotal() {
assertThat(repository.countOrganizations(query)).isEqualTo(7L);
}

@Test
@DisplayName("excludeOrganizationIds keeps those organizations out of the page")
void excludeOrganizationIdsFiltersThePage() {
save("a", 100);
save("b", 200);
save("c", 300);

OrganizationQueryFilter filter = OrganizationQueryFilter.builder()
.status(OrganizationStatus.ACTIVE.name())
.excludeOrganizationIds(Set.of("b"))
.build();

Query query = repository.buildOrganizationQuery(filter, null);
List<Organization> result = repository.findOrganizationsWithCursor(query, null, 50, SORT_UPDATED_AT, DESC);

assertThat(result).extracting(Organization::getName).containsExactly("c", "a");
}

@Test
@DisplayName("countOrganizations honours excludeOrganizationIds, so filteredCount matches the page")
void countHonoursExcludeOrganizationIds() {
for (int i = 1; i <= 5; i++) {
save("o" + i, i * 100L);
}

OrganizationQueryFilter filter = OrganizationQueryFilter.builder()
.status(OrganizationStatus.ACTIVE.name())
.excludeOrganizationIds(Set.of("o2", "o4"))
.build();

Query query = repository.buildOrganizationQuery(filter, null);

assertThat(repository.countOrganizations(query)).isEqualTo(3L);
}

@Test
@DisplayName("an empty excludeOrganizationIds set excludes nobody")
void emptyExcludeOrganizationIdsKeepsEveryone() {
save("a", 100);
save("b", 200);

OrganizationQueryFilter filter = OrganizationQueryFilter.builder()
.status(OrganizationStatus.ACTIVE.name())
.excludeOrganizationIds(Set.of())
.build();

Query query = repository.buildOrganizationQuery(filter, null);
List<Organization> result = repository.findOrganizationsWithCursor(query, null, 50, SORT_UPDATED_AT, DESC);

assertThat(result).extracting(Organization::getName).containsExactly("b", "a");
}

@Test
@DisplayName("legacy _id cursor still paginates without overlap")
void legacyIdCursorPaginates() {
Expand Down