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
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
@@ -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 {
@@ -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");
};
}
Original file line number Diff line number Diff line change
@@ -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;
@@ -21,6 +23,9 @@ public class AuthorityResource {

@Inject
Authority.Repository authorityRepo;

@Inject
User.Repository userRepo;

@GET
@Path("/search")
@@ -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
@@ -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
@@ -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
@@ -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;
@@ -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 {
@@ -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
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
@@ -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;
@@ -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;
}
@@ -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
@@ -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;
@@ -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;
23 changes: 20 additions & 3 deletions frontend/src/components/SearchInputGroup.vue
Original file line number Diff line number Diff line change
@@ -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>
@@ -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[]>
58 changes: 52 additions & 6 deletions frontend/src/components/VaultDetails.vue
Original file line number Diff line number Diff line change
@@ -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">
@@ -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) {
@@ -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);
@@ -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) {
1 change: 1 addition & 0 deletions frontend/src/i18n/en-US.json
Original file line number Diff line number Diff line change
@@ -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",