Skip to content

Scope config lookup to org #19217

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

Merged
merged 1 commit into from
Dec 8, 2023
Merged

Scope config lookup to org #19217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 components/gitpod-db/src/project-db.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { TransactionalDB } from "./typeorm/transactional-db-impl";
export const ProjectDB = Symbol("ProjectDB");
export interface ProjectDB extends TransactionalDB<ProjectDB> {
findProjectById(projectId: string): Promise<Project | undefined>;
findProjectsByCloneUrl(cloneUrl: string): Promise<Project[]>;
findProjectsByCloneUrl(cloneUrl: string, organizationId?: string): Promise<Project[]>;
findProjects(orgID: string): Promise<Project[]>;
findProjectsBySearchTerm(args: FindProjectsBySearchTermArgs): Promise<{ total: number; rows: Project[] }>;
storeProject(project: Project): Promise<Project>;
Expand Down
4 changes: 2 additions & 2 deletions components/gitpod-db/src/typeorm/project-db-impl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,9 @@ export class ProjectDBImpl extends TransactionalDBImpl<ProjectDB> implements Pro
return repo.findOne({ id: projectId, markedDeleted: false });
}

public async findProjectsByCloneUrl(cloneUrl: string): Promise<Project[]> {
public async findProjectsByCloneUrl(cloneUrl: string, organizationId?: string): Promise<Project[]> {
const repo = await this.getRepo();
const conditions: FindConditions<DBProject> = { cloneUrl, markedDeleted: false };
const conditions: FindConditions<DBProject> = { cloneUrl, markedDeleted: false, teamId: organizationId };
Copy link
Member

Choose a reason for hiding this comment

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

How does it work here? If teamId is present but undefined then it doest not count or it will check as null.

Copy link
Contributor

Choose a reason for hiding this comment

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

TypeOrm don't know it's null unless we use something like Null (not sure) or queryBuild with IS NULL,

so condition would be { cloneUrl, markedDeleted: false }

Copy link
Contributor

@mustard-mh mustard-mh Dec 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We may need to worry if organizationId is default value => EmptyString in gPRC, then it will find with { teamId: "" }

Copy link
Contributor

Choose a reason for hiding this comment

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

@filiptronicek

return repo.find(conditions);
}

Expand Down
4 changes: 2 additions & 2 deletions components/server/src/projects/projects-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,8 @@ export class ProjectsService {
return filteredProjects;
}

async findProjectsByCloneUrl(userId: string, cloneUrl: string): Promise<Project[]> {
const projects = await this.projectDB.findProjectsByCloneUrl(cloneUrl);
async findProjectsByCloneUrl(userId: string, cloneUrl: string, organizationId?: string): Promise<Project[]> {
const projects = await this.projectDB.findProjectsByCloneUrl(cloneUrl, organizationId);
const result: Project[] = [];
for (const project of projects) {
if (await this.auth.hasPermissionOnProject(userId, "read_info", project.id)) {
Expand Down
6 changes: 5 additions & 1 deletion components/server/src/workspace/context-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,11 @@ export class ContextService {
if (options?.projectId) {
project = await this.projectsService.getProject(user.id, options.projectId);
} else if (CommitContext.is(context)) {
const projects = await this.projectsService.findProjectsByCloneUrl(user.id, context.repository.cloneUrl);
const projects = await this.projectsService.findProjectsByCloneUrl(
Copy link
Member

Choose a reason for hiding this comment

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

It would be good if we added DB tests for various cases.

user.id,
context.repository.cloneUrl,
options?.organizationId,
);
if (projects.length > 1) {
throw new ApplicationError(ErrorCodes.BAD_REQUEST, "Multiple projects found for clone URL.");
}
Expand Down