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

Feature: Add member count for groups in VaultDetail view #321

Closed
wants to merge 3 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
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
import org.cryptomator.hub.entities.Group;
import org.cryptomator.hub.entities.User;

import jakarta.inject.Inject;

abstract sealed class AuthorityDto permits UserDto, GroupDto, MemberDto {

public enum Type {
Expand All @@ -30,10 +32,13 @@ protected AuthorityDto(String id, Type type, String name, String pictureUrl) {
this.pictureUrl = pictureUrl;
}

@Inject
static User.Repository userRepo;
Comment on lines +35 to +36
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid static injection for thread safety.

Using static injection for repositories can cause issues in multi-threaded environments and makes testing more difficult. Consider passing the repository as a parameter to the fromEntity method.

-@Inject
-static User.Repository userRepo;

 static AuthorityDto fromEntity(Authority a) {
+static AuthorityDto fromEntity(Authority a, User.Repository userRepo) {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@Inject
static User.Repository userRepo;
// Removed static injection for thread safety and better separation of concerns
// (Removed the following lines)
// @Inject
// static User.Repository userRepo;
static AuthorityDto fromEntity(Authority a, User.Repository userRepo) {
// method implementation that makes use of the passed userRepo
// ...
}


static AuthorityDto fromEntity(Authority a) {
return switch (a) {
case User u -> UserDto.justPublicInfo(u);
case Group g -> GroupDto.fromEntity(g);
case Group g -> new GroupDto(g.getId(), g.getName(), (int) userRepo.getEffectiveGroupUsers(g.getId()).count());
default -> throw new IllegalStateException("authority is not of type user or group");
};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
import jakarta.ws.rs.QueryParam;
import jakarta.ws.rs.core.MediaType;
import org.cryptomator.hub.entities.Authority;
import org.cryptomator.hub.entities.Group;
import org.cryptomator.hub.entities.User;
import org.eclipse.microprofile.openapi.annotations.Operation;
import org.eclipse.microprofile.openapi.annotations.responses.APIResponse;
import org.jboss.resteasy.reactive.NoCache;
Expand All @@ -21,6 +23,9 @@ public class AuthorityResource {

@Inject
Authority.Repository authorityRepo;

@Inject
User.Repository userRepo;

@GET
@Path("/search")
Expand All @@ -29,7 +34,10 @@ public class AuthorityResource {
@NoCache
@Operation(summary = "search authority by name")
public List<AuthorityDto> search(@QueryParam("query") @NotBlank String query) {
return authorityRepo.byName(query).map(AuthorityDto::fromEntity).toList();
List<Authority> authorities = authorityRepo.byName(query).toList();
return authorities.stream()
.map(this::convertToDto)
.toList();
}

@GET
Expand All @@ -40,7 +48,20 @@ public List<AuthorityDto> search(@QueryParam("query") @NotBlank String query) {
@Operation(summary = "lists all authorities matching the given ids", description = "lists for each id in the list its corresponding authority. Ignores all id's where an authority cannot be found")
@APIResponse(responseCode = "200")
public List<AuthorityDto> getSome(@QueryParam("ids") List<String> authorityIds) {
return authorityRepo.findAllInList(authorityIds).map(AuthorityDto::fromEntity).toList();
return authorityRepo.findAllInList(authorityIds)
.map(this::convertToDto)
.toList();
}

private AuthorityDto convertToDto(Authority a) {
if (a instanceof User u) {
return UserDto.justPublicInfo(u);
} else if (a instanceof Group g) {
int memberCount = (int) userRepo.getEffectiveGroupUsers(g.getId()).count();
return new GroupDto(g.getId(), g.getName(), memberCount);
} else {
throw new IllegalStateException("authority is not of type user or group");
}
}

}
}
19 changes: 13 additions & 6 deletions backend/src/main/java/org/cryptomator/hub/api/GroupDto.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,19 @@

public final class GroupDto extends AuthorityDto {

GroupDto(@JsonProperty("id") String id, @JsonProperty("name") String name) {
super(id, Type.GROUP, name, null);
}
private final int memberCount;

public static GroupDto fromEntity(Group group) {
return new GroupDto(group.getId(), group.getName());
}
GroupDto(@JsonProperty("id") String id, @JsonProperty("name") String name, @JsonProperty("memberCount") int memberCount) {
super(id, Type.GROUP, name, null);
this.memberCount = memberCount;
}

@JsonProperty("memberCount")
public int getMemberCount() {
return memberCount;
}

public static GroupDto fromEntity(Group group, int memberCount) {
return new GroupDto(group.getId(), group.getName(), memberCount);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import jakarta.annotation.security.RolesAllowed;
import jakarta.inject.Inject;
import jakarta.ws.rs.core.Response;
import jakarta.ws.rs.GET;
import jakarta.ws.rs.Path;
import jakarta.ws.rs.PathParam;
Expand All @@ -13,6 +14,8 @@
import org.eclipse.microprofile.openapi.annotations.Operation;

import java.util.List;
import java.util.HashMap;
import java.util.Map;

@Path("/groups")
public class GroupsResource {
Expand All @@ -28,7 +31,8 @@ public class GroupsResource {
@Produces(MediaType.APPLICATION_JSON)
@Operation(summary = "list all groups")
public List<GroupDto> getAll() {
return groupRepo.findAll().stream().map(GroupDto::fromEntity).toList();
List<Group> groups = groupRepo.findAll().list();
return groups.stream().map(group -> GroupDto.fromEntity(group, groupRepo.countMembers(group.getId()))).toList();
Comment on lines +34 to +35
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Optimize group listing to avoid N+1 query.

The current implementation makes a separate query for each group's member count. Consider fetching all member counts in a single query.

 public List<GroupDto> getAll() {
-    List<Group> groups = groupRepo.findAll().list();
-    return groups.stream().map(group -> GroupDto.fromEntity(group, groupRepo.countMembers(group.getId()))).toList();
+    var groups = groupRepo.findAll().list();
+    var memberCounts = getEntityManager()
+        .createQuery("SELECT g.id, COUNT(m) FROM Group g LEFT JOIN g.members m GROUP BY g.id", Object[].class)
+        .getResultList()
+        .stream()
+        .collect(Collectors.toMap(row -> (String) row[0], row -> ((Long) row[1]).intValue()));
+    return groups.stream()
+        .map(group -> GroupDto.fromEntity(group, memberCounts.getOrDefault(group.getId(), 0)))
+        .toList();
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
List<Group> groups = groupRepo.findAll().list();
return groups.stream().map(group -> GroupDto.fromEntity(group, groupRepo.countMembers(group.getId()))).toList();
public List<GroupDto> getAll() {
- List<Group> groups = groupRepo.findAll().list();
- return groups.stream().map(group -> GroupDto.fromEntity(group, groupRepo.countMembers(group.getId()))).toList();
+ var groups = groupRepo.findAll().list();
+ var memberCounts = getEntityManager()
+ .createQuery("SELECT g.id, COUNT(m) FROM Group g LEFT JOIN g.members m GROUP BY g.id", Object[].class)
+ .getResultList()
+ .stream()
+ .collect(Collectors.toMap(row -> (String) row[0], row -> ((Long) row[1]).intValue()));
+ return groups.stream()
+ .map(group -> GroupDto.fromEntity(group, memberCounts.getOrDefault(group.getId(), 0)))
+ .toList();
}

}

@GET
Expand Down
22 changes: 15 additions & 7 deletions backend/src/main/java/org/cryptomator/hub/entities/Group.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,8 @@

import io.quarkus.hibernate.orm.panache.PanacheRepositoryBase;
import jakarta.enterprise.context.ApplicationScoped;
import jakarta.persistence.CascadeType;
import jakarta.persistence.DiscriminatorValue;
import jakarta.persistence.Entity;
import jakarta.persistence.JoinColumn;
import jakarta.persistence.JoinTable;
import jakarta.persistence.ManyToMany;
import jakarta.persistence.Table;
import jakarta.inject.Inject;
import jakarta.persistence.*;

import java.util.HashSet;
import java.util.Set;
Expand All @@ -25,6 +20,9 @@ public class Group extends Authority {
)
private Set<Authority> members = new HashSet<>();

@Inject
transient Repository groupRepo;

public Set<Authority> getMembers() {
return members;
}
Expand All @@ -33,7 +31,17 @@ public void setMembers(Set<Authority> members) {
this.members = members;
}

public int getMemberCount() {
return groupRepo.countMembers(this.getId());
}

@ApplicationScoped
public static class Repository implements PanacheRepositoryBase<Group, String> {
public int countMembers(String groupId) {
return getEntityManager()
.createQuery("SELECT SIZE(g.members) FROM Group g WHERE g.id = :groupId", Integer.class)
.setParameter("groupId", groupId)
.getSingleResult();
}
}
}
12 changes: 11 additions & 1 deletion frontend/src/common/backend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,15 @@ export type GroupDto = {
id: string;
name: string;
pictureUrl?: string;
memberCount: number;
}

class GroupService {
public async getMemberCount(groupId: string): Promise<number> {
return axiosAuth.get<UserDto[]>(`/groups/${groupId}/effective-members`)
.then(response => response.data.length)
.catch(() => 0);
}
}

export type AuthorityDto = UserDto | GroupDto;
Expand Down Expand Up @@ -385,7 +394,8 @@ const services = {
billing: new BillingService(),
version: new VersionService(),
license: new LicenseService(),
settings: new SettingsService()
settings: new SettingsService(),
groups: new GroupService()
};

export default services;
Expand Down
23 changes: 20 additions & 3 deletions frontend/src/components/SearchInputGroup.vue
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,29 @@
<img v-else :src="selectedItem.pictureUrl ?? ''" alt="" class="w-5 h-5 rounded-full" >
</div>

<ComboboxInput v-if="selectedItem == null" v-focus class="w-full h-10 rounded-l-md border border-gray-300 bg-white py-2 px-10 shadow-xs focus:border-primary focus:outline-hidden focus:ring-1 focus:ring-primary sm:text-sm disabled:bg-primary-l2" placeholder="John Doe" @change="query = $event.target.value"/>
<input v-else v-model="selectedItem.name" class="w-full h-10 rounded-l-md border border-gray-300 bg-primary-l2 py-2 px-10 shadow-xs focus:border-primary focus:outline-hidden focus:ring-1 focus:ring-primary sm:text-sm" readonly />
</div>
<ComboboxInput
v-if="selectedItem == null"
v-focus
class="w-full h-10 rounded-l-md border border-gray-300 bg-white py-2 pl-10 pr-3 shadow-xs focus:border-primary focus:outline-hidden focus:ring-1 focus:ring-primary sm:text-sm disabled:bg-primary-l2"
placeholder="John Doe"
@change="query = $event.target.value"
/>

<div v-else class="w-full h-10 rounded-l-md border border-gray-300 bg-primary-l2 py-2 pl-10 pr-3 flex items-center justify-between shadow-xs sm:text-sm">
<span class="truncate">{{ selectedItem.name }}</span>
<span v-if="selectedItem.type === 'GROUP' && 'memberCount' in selectedItem" class="text-gray-500 text-xs italic mr-6">
{{ selectedItem.memberCount }} {{ t('vaultDetails.sharedWith.members') }}
</span>
</div>
</div>
<ComboboxOptions v-if="selectedItem == null && filteredItems.length > 0" class="absolute z-10 mt-1 max-h-56 w-full overflow-auto rounded-md bg-white py-1 text-base shadow-lg ring-1 ring-black/5 focus:outline-hidden sm:text-sm">
<ComboboxOption v-for="item in filteredItems" :key="item.id" :value="item" class="relative cursor-default select-none py-2 pl-3 pr-9 ui-not-active:text-gray-900 ui-active:bg-primary ui-active:text-white">
<div class="flex items-center">
<img :src="item.pictureUrl ?? ''" alt="" class="h-6 w-6 shrink-0 rounded-full" >
<span class="ml-3 truncate">{{ item.name }}</span>
<span v-if="'memberCount' in item && item.memberCount !== undefined" class="ml-auto text-xs text-gray-500 italic">
{{ item.memberCount }} {{ t('vaultDetails.sharedWith.members') }}
</span>
</div>
</ComboboxOption>
</ComboboxOptions>
Expand All @@ -38,13 +52,16 @@ import { Combobox, ComboboxInput, ComboboxOption, ComboboxOptions } from '@headl
import { UsersIcon, XCircleIcon } from '@heroicons/vue/24/solid';
import { computed, nextTick, ref, shallowRef, watch } from 'vue';
import { debounce } from '../common/util';
import { useI18n } from 'vue-i18n';

export type Item = {
id: string;
name: string;
pictureUrl?: string;
}

const { t } = useI18n({ useScope: 'global' });

const props = defineProps<{
actionTitle: string
onSearch: (query: string) => Promise<T[]>
Expand Down
58 changes: 52 additions & 6 deletions frontend/src/components/VaultDetails.vue
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,13 @@
<div class="flex justify-between items-center">
<div class="flex items-center whitespace-nowrap w-full" :title="member.name">
<img :src="member.pictureUrl" alt="" class="w-8 h-8 rounded-full" />
<p class="w-full ml-4 text-sm font-medium text-gray-900 truncate">{{ member.name }}</p>
<div class="w-full flex justify-between items-center">
<p class="ml-4 text-sm font-medium text-gray-900 truncate">{{ member.name }}</p>
<span v-if="member.type === 'GROUP' && groupMemberCounts.get(member.id) !== undefined"
class="text-xs italic text-gray-500 text-right">
{{ groupMemberCounts.get(member.id) }} {{ t('vaultDetails.sharedWith.members') }}
</span>
</div>
<TrustDetails v-if="member.type === 'USER'" :trusted-user="member" :trusts="trusts" @trust-changed="refreshTrusts()"/>
<div v-if="member.role == 'OWNER'" class="ml-3 inline-flex items-center rounded-md bg-gray-50 px-2 py-1 text-xs font-medium text-gray-600 ring-1 ring-inset ring-gray-500/10">{{ t('vaultDetails.sharedWith.badge.owner') }}</div>
<Menu v-if="member.id != me?.id" as="div" class="relative ml-2 inline-block shrink-0 text-left">
Expand Down Expand Up @@ -298,10 +304,24 @@ async function fetchData() {

async function fetchOwnerData() {
try {
(await backend.vaults.getMembers(props.vaultId)).forEach(member => members.value.set(member.id, member));
const fetchedMembers = await backend.vaults.getMembers(props.vaultId);
for (const member of fetchedMembers) {
if (member.type === "GROUP") {
try {
const count = await backend.groups.getMemberCount(member.id);
members.value.set(member.id, { ...member, memberCount: count });
} catch (error) {
members.value.set(member.id, { ...member, memberCount: 0 });
}
} else {
members.value.set(member.id, member);
}
}
Comment on lines +307 to +319
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider using Promise.allSettled for parallel member count fetching.

The current implementation fetches member counts sequentially, which could be optimized.

-    const fetchedMembers = await backend.vaults.getMembers(props.vaultId);
-    for (const member of fetchedMembers) {
-      if (member.type === "GROUP") {
-        try {
-          const count = await backend.groups.getMemberCount(member.id);
-          members.value.set(member.id, { ...member, memberCount: count });
-        } catch (error) {
-          members.value.set(member.id, { ...member, memberCount: 0 });
-        }
-      } else {
-        members.value.set(member.id, member);
-      }
-    }
+    const fetchedMembers = await backend.vaults.getMembers(props.vaultId);
+    const memberPromises = fetchedMembers.map(async member => {
+      if (member.type === "GROUP") {
+        const count = await backend.groups.getMemberCount(member.id)
+          .catch(() => 0);
+        return { ...member, memberCount: count };
+      }
+      return member;
+    });
+    const results = await Promise.allSettled(memberPromises);
+    results.forEach((result, index) => {
+      if (result.status === 'fulfilled') {
+        members.value.set(result.value.id, result.value);
+      }
+    });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const fetchedMembers = await backend.vaults.getMembers(props.vaultId);
for (const member of fetchedMembers) {
if (member.type === "GROUP") {
try {
const count = await backend.groups.getMemberCount(member.id);
members.value.set(member.id, { ...member, memberCount: count });
} catch (error) {
members.value.set(member.id, { ...member, memberCount: 0 });
}
} else {
members.value.set(member.id, member);
}
}
const fetchedMembers = await backend.vaults.getMembers(props.vaultId);
const memberPromises = fetchedMembers.map(async member => {
if (member.type === "GROUP") {
const count = await backend.groups.getMemberCount(member.id)
.catch(() => 0);
return { ...member, memberCount: count };
}
return member;
});
const results = await Promise.allSettled(memberPromises);
results.forEach((result, index) => {
if (result.status === 'fulfilled') {
members.value.set(result.value.id, result.value);
}
});


await refreshTrusts();
usersRequiringAccessGrant.value = await backend.vaults.getUsersRequiringAccessGrant(props.vaultId);
vaultRecoveryRequired.value = false;

const vaultKeyJwe = await backend.vaults.accessToken(props.vaultId, true);
vaultKeys.value = await loadVaultKeys(vaultKeyJwe);
} catch (error) {
Expand All @@ -317,6 +337,16 @@ async function fetchOwnerData() {
}
}

const groupMemberCounts = computed(() => {
const counts = new Map<string, number>();
members.value.forEach((member) => {
if (member.type === 'GROUP' && 'memberCount' in member) {
counts.set(member.id, member.memberCount);
}
});
return counts;
});

async function loadVaultKeys(vaultKeyJwe: string): Promise<VaultKeys> {
const userKeys = await userdata.decryptUserKeysWithBrowser();
return VaultKeys.decryptWithUserKey(vaultKeyJwe, userKeys.ecdhKeyPair.privateKey);
Expand Down Expand Up @@ -465,10 +495,26 @@ function refreshVault(updatedVault: VaultDto) {
emit('vaultUpdated', updatedVault);
}

async function searchAuthority(query: string): Promise<AuthorityDto[]> {
return (await backend.authorities.search(query))
.filter(authority => !members.value.has(authority.id))
.sort((a, b) => a.name.localeCompare(b.name));
const searchResults = ref<Array<AuthorityDto & { memberCount?: number }>>([]);

async function searchAuthority(query: string): Promise<(AuthorityDto & { memberCount?: number })[]> {
const results = await backend.authorities.search(query);
const filtered = results.filter(authority => !members.value.has(authority.id));

const enhanced = await Promise.all(
filtered.map(async authority => {
if (authority.type === "GROUP") {
try {
const count = await backend.groups.getMemberCount(authority.id);
return { ...authority, memberCount: count };
} catch (error) {
return { ...authority, memberCount: 0 };
}
}
return authority;
})
);
return enhanced.sort((a, b) => a.name.localeCompare(b.name));
}

async function updateMemberRole(member: MemberDto, role: VaultRole) {
Expand Down
1 change: 1 addition & 0 deletions frontend/src/i18n/en-US.json
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@
"vaultDetails.sharedWith.title": "Shared with",
"vaultDetails.sharedWith.badge.owner": "Owner",
"vaultDetails.sharedWith.grantOwnership": "Grant Ownership",
"vaultDetails.sharedWith.members": "Members",
"vaultDetails.sharedWith.revokeOwnership": "Revoke Ownership",
"vaultDetails.actions.title": "Actions",
"vaultDetails.actions.updatePermissions": "Update Permissions",
Expand Down