From f4fef1bc8193df2cfb07bde2d170bca48d9f5986 Mon Sep 17 00:00:00 2001 From: yk-eukarya <81808708+yk-eukarya@users.noreply.github.com> Date: Tue, 4 Mar 2025 13:27:11 +0100 Subject: [PATCH] fix(server): project alias should be unique system wise (#1427) --- .../internal/adapter/publicapi/controller.go | 2 +- .../internal/infrastructure/memory/project.go | 14 ++++----- .../infrastructure/memory/project_test.go | 30 +++++++++---------- .../internal/infrastructure/mongo/project.go | 27 +++++++++++++---- .../infrastructure/mongo/project_test.go | 30 +++++++++---------- server/internal/usecase/interactor/project.go | 21 ++++--------- server/internal/usecase/repo/project.go | 2 +- 7 files changed, 66 insertions(+), 60 deletions(-) diff --git a/server/internal/adapter/publicapi/controller.go b/server/internal/adapter/publicapi/controller.go index c64e133b53..47c2417fc2 100644 --- a/server/internal/adapter/publicapi/controller.go +++ b/server/internal/adapter/publicapi/controller.go @@ -30,7 +30,7 @@ func NewController(project repo.Project, usecases *interfaces.Container, aur ass } func (c *Controller) checkProject(ctx context.Context, prj string) (*project.Project, error) { - pr, err := c.project.FindByPublicName(ctx, prj) + pr, err := c.project.FindByIDOrAlias(ctx, project.IDOrAlias(prj)) if err != nil { if errors.Is(err, rerror.ErrNotFound) { return nil, rerror.ErrNotFound diff --git a/server/internal/infrastructure/memory/project.go b/server/internal/infrastructure/memory/project.go index 4d92522c40..cc37052f56 100644 --- a/server/internal/infrastructure/memory/project.go +++ b/server/internal/infrastructure/memory/project.go @@ -110,26 +110,26 @@ func (r *Project) FindByIDOrAlias(_ context.Context, q project.IDOrAlias) (*proj return nil, rerror.ErrNotFound } -func (r *Project) FindByPublicName(_ context.Context, name string) (*project.Project, error) { +func (r *Project) IsAliasAvailable(_ context.Context, name string) (bool, error) { if r.err != nil { - return nil, r.err + return false, r.err } if name == "" { - return nil, nil + return false, nil } + // no need to filter by workspace, because alias is unique across all workspaces p := r.data.Find(func(_ id.ProjectID, v *project.Project) bool { - return v.Alias() == name && r.f.CanRead(v.Workspace()) + return v.Alias() == name }) if p != nil { - return p, nil + return false, nil } - return nil, rerror.ErrNotFound + return true, nil } - func (r *Project) FindByPublicAPIToken(ctx context.Context, token string) (*project.Project, error) { if r.err != nil { return nil, r.err diff --git a/server/internal/infrastructure/memory/project_test.go b/server/internal/infrastructure/memory/project_test.go index c99b948a75..71acfe6a76 100644 --- a/server/internal/infrastructure/memory/project_test.go +++ b/server/internal/infrastructure/memory/project_test.go @@ -444,7 +444,7 @@ func TestProjectRepo_FindByIDs(t *testing.T) { } } -func TestProjectRepo_FindByPublicName(t *testing.T) { +func TestProjectRepo_IsAliasAvailable(t *testing.T) { mocknow := time.Now().Truncate(time.Millisecond).UTC() tid1 := accountdomain.NewWorkspaceID() id1 := id.NewProjectID() @@ -468,7 +468,7 @@ func TestProjectRepo_FindByPublicName(t *testing.T) { seeds project.List arg string filter *repo.WorkspaceFilter - want *project.Project + want bool wantErr error mockErr bool }{ @@ -477,8 +477,8 @@ func TestProjectRepo_FindByPublicName(t *testing.T) { seeds: project.List{}, arg: "xyz123", filter: nil, - want: nil, - wantErr: rerror.ErrNotFound, + want: true, + wantErr: nil, }, { name: "Not found", @@ -487,8 +487,8 @@ func TestProjectRepo_FindByPublicName(t *testing.T) { }, arg: "xyz123", filter: nil, - want: nil, - wantErr: rerror.ErrNotFound, + want: true, + wantErr: nil, }, { name: "public Found", @@ -497,16 +497,16 @@ func TestProjectRepo_FindByPublicName(t *testing.T) { }, arg: "xyz123", filter: nil, - want: p1, + want: false, wantErr: nil, }, { - name: "linited Found", + name: "limited Found", seeds: project.List{ p2, }, arg: "xyz321", - want: p2, + want: false, filter: nil, wantErr: nil, }, @@ -519,11 +519,11 @@ func TestProjectRepo_FindByPublicName(t *testing.T) { }, arg: "xyz123", filter: nil, - want: p1, + want: false, wantErr: nil, }, { - name: "Filtered should not Found", + name: "Filtered should Found", seeds: project.List{ p1, project.New().NewID().Workspace(accountdomain.NewWorkspaceID()).MustBuild(), @@ -531,8 +531,8 @@ func TestProjectRepo_FindByPublicName(t *testing.T) { }, arg: "xyz123", filter: &repo.WorkspaceFilter{Readable: []accountdomain.WorkspaceID{accountdomain.NewWorkspaceID()}, Writable: []accountdomain.WorkspaceID{}}, - want: nil, - wantErr: rerror.ErrNotFound, + want: false, + wantErr: nil, }, { name: "Filtered should Found", @@ -543,7 +543,7 @@ func TestProjectRepo_FindByPublicName(t *testing.T) { }, arg: "xyz123", filter: &repo.WorkspaceFilter{Readable: []accountdomain.WorkspaceID{tid1}, Writable: []accountdomain.WorkspaceID{}}, - want: p1, + want: false, wantErr: nil, }, { @@ -573,7 +573,7 @@ func TestProjectRepo_FindByPublicName(t *testing.T) { r = r.Filtered(*tc.filter) } - got, err := r.FindByPublicName(ctx, tc.arg) + got, err := r.IsAliasAvailable(ctx, tc.arg) if tc.wantErr != nil { assert.ErrorIs(t, err, tc.wantErr) return diff --git a/server/internal/infrastructure/mongo/project.go b/server/internal/infrastructure/mongo/project.go index 97b8962377..fa3dc0a114 100644 --- a/server/internal/infrastructure/mongo/project.go +++ b/server/internal/infrastructure/mongo/project.go @@ -17,7 +17,7 @@ import ( var ( // TODO: the `publication.token` should be unique, this should be fixed in the future - projectIndexes = []string{"workspace", "publication.token"} + projectIndexes = []string{"workspace"} projectUniqueIndexes = []string{"id"} ) @@ -31,7 +31,21 @@ func NewProject(client *mongox.Client) repo.Project { } func (r *ProjectRepo) Init() error { - return createIndexes(context.Background(), r.client, projectIndexes, projectUniqueIndexes) + idx := mongox.IndexFromKeys(projectIndexes, false) + idx = append(idx, mongox.IndexFromKeys(projectUniqueIndexes, true)...) + idx = append(idx, mongox.Index{ + Name: "re_publication_token", + Key: bson.D{{Key: "publication.token", Value: 1}}, + Unique: true, + Filter: bson.M{"publication.token": bson.M{"$type": "string"}}, + }) + idx = append(idx, mongox.Index{ + Name: "re_alias", + Key: bson.D{{Key: "alias", Value: 1}}, + Unique: true, + Filter: bson.M{"alias": bson.M{"$type": "string"}}, + }) + return createIndexes2(context.Background(), r.client, idx...) } func (r *ProjectRepo) Filtered(f repo.WorkspaceFilter) repo.Project { @@ -90,13 +104,16 @@ func (r *ProjectRepo) FindByIDOrAlias(ctx context.Context, id project.IDOrAlias) return r.findOne(ctx, filter) } -func (r *ProjectRepo) FindByPublicName(ctx context.Context, name string) (*project.Project, error) { +func (r *ProjectRepo) IsAliasAvailable(ctx context.Context, name string) (bool, error) { if name == "" { - return nil, rerror.ErrNotFound + return false, nil } - return r.findOne(ctx, bson.M{ + + // no need to filter by workspace, because alias is unique across all workspaces + c, err := r.client.Count(ctx, bson.M{ "alias": name, }) + return c == 0 && err == nil, err } func (r *ProjectRepo) FindByPublicAPIToken(ctx context.Context, token string) (*project.Project, error) { diff --git a/server/internal/infrastructure/mongo/project_test.go b/server/internal/infrastructure/mongo/project_test.go index 71c978b16c..061270a180 100644 --- a/server/internal/infrastructure/mongo/project_test.go +++ b/server/internal/infrastructure/mongo/project_test.go @@ -421,7 +421,7 @@ func Test_projectRepo_FindByIDs(t *testing.T) { } } -func Test_projectRepo_FindByPublicName(t *testing.T) { +func Test_projectRepo_IsAliasAvailable(t *testing.T) { now := time.Now().Truncate(time.Millisecond).UTC() tid1 := accountdomain.NewWorkspaceID() id1 := id.NewProjectID() @@ -445,7 +445,7 @@ func Test_projectRepo_FindByPublicName(t *testing.T) { seeds project.List arg string filter *repo.WorkspaceFilter - want *project.Project + want bool wantErr error }{ { @@ -453,8 +453,8 @@ func Test_projectRepo_FindByPublicName(t *testing.T) { seeds: project.List{}, arg: "xyz123", filter: nil, - want: nil, - wantErr: rerror.ErrNotFound, + want: true, + wantErr: nil, }, { name: "Not found", @@ -463,8 +463,8 @@ func Test_projectRepo_FindByPublicName(t *testing.T) { }, arg: "xyz123", filter: nil, - want: nil, - wantErr: rerror.ErrNotFound, + want: true, + wantErr: nil, }, { name: "public Found", @@ -473,16 +473,16 @@ func Test_projectRepo_FindByPublicName(t *testing.T) { }, arg: "xyz123", filter: nil, - want: p1, + want: false, wantErr: nil, }, { - name: "linited Found", + name: "limited Found", seeds: project.List{ p2, }, arg: "xyz321", - want: p2, + want: false, filter: nil, wantErr: nil, }, @@ -495,11 +495,11 @@ func Test_projectRepo_FindByPublicName(t *testing.T) { }, arg: "xyz123", filter: nil, - want: p1, + want: false, wantErr: nil, }, { - name: "Filtered should not Found", + name: "Filtered should Found", seeds: project.List{ p1, project.New().NewID().Workspace(accountdomain.NewWorkspaceID()).MustBuild(), @@ -507,8 +507,8 @@ func Test_projectRepo_FindByPublicName(t *testing.T) { }, arg: "xyz123", filter: &repo.WorkspaceFilter{Readable: []accountdomain.WorkspaceID{accountdomain.NewWorkspaceID()}, Writable: []accountdomain.WorkspaceID{}}, - want: nil, - wantErr: rerror.ErrNotFound, + want: false, + wantErr: nil, }, { name: "Filtered should Found", @@ -519,7 +519,7 @@ func Test_projectRepo_FindByPublicName(t *testing.T) { }, arg: "xyz123", filter: &repo.WorkspaceFilter{Readable: []accountdomain.WorkspaceID{tid1}, Writable: []accountdomain.WorkspaceID{}}, - want: p1, + want: false, wantErr: nil, }, } @@ -544,7 +544,7 @@ func Test_projectRepo_FindByPublicName(t *testing.T) { r = r.Filtered(*tc.filter) } - got, err := r.FindByPublicName(ctx, tc.arg) + got, err := r.IsAliasAvailable(ctx, tc.arg) if tc.wantErr != nil { assert.ErrorIs(t, err, tc.wantErr) return diff --git a/server/internal/usecase/interactor/project.go b/server/internal/usecase/interactor/project.go index 8b7fa633f6..43d0648e25 100644 --- a/server/internal/usecase/interactor/project.go +++ b/server/internal/usecase/interactor/project.go @@ -2,7 +2,6 @@ package interactor import ( "context" - "errors" "time" "github.com/reearth/reearth-cms/server/internal/usecase" @@ -13,7 +12,6 @@ import ( "github.com/reearth/reearth-cms/server/pkg/project" "github.com/reearth/reearthx/account/accountdomain" "github.com/reearth/reearthx/account/accountdomain/workspace" - "github.com/reearth/reearthx/rerror" "github.com/reearth/reearthx/usecasex" ) @@ -54,11 +52,9 @@ func (i *Project) Create(ctx context.Context, p interfaces.CreateProjectParam, o pb = pb.Description(*p.Description) } if p.Alias != nil { - proj2, _ := i.repos.Project.FindByPublicName(ctx, *p.Alias) - if proj2 != nil { + if ok, _ := i.repos.Project.IsAliasAvailable(ctx, *p.Alias); !ok { return nil, interfaces.ErrProjectAliasAlreadyUsed } - pb = pb.Alias(*p.Alias) } if len(p.RequestRoles) > 0 { @@ -95,10 +91,8 @@ func (i *Project) Update(ctx context.Context, p interfaces.UpdateProjectParam, o proj.UpdateDescription(*p.Description) } - if p.Alias != nil { - proj2, _ := i.repos.Project.FindByPublicName(ctx, *p.Alias) - - if proj2 != nil && proj2.ID() != proj.ID() { + if p.Alias != nil && *p.Alias != proj.Alias() { + if ok, _ := i.repos.Project.IsAliasAvailable(ctx, *p.Alias); !ok { return nil, interfaces.ErrProjectAliasAlreadyUsed } @@ -140,12 +134,7 @@ func (i *Project) CheckAlias(ctx context.Context, alias string) (bool, error) { return false, project.ErrInvalidAlias } - prj, err := i.repos.Project.FindByPublicName(ctx, alias) - if prj == nil && err == nil || err != nil && errors.Is(err, rerror.ErrNotFound) { - return true, nil - } - - return false, err + return i.repos.Project.IsAliasAvailable(ctx, alias) }) } @@ -182,7 +171,7 @@ func (i *Project) RegenerateToken(ctx context.Context, pId id.ProjectID, operato return nil, interfaces.ErrOperationDenied } - if p.Publication() == nil || p.Publication().Scope() != project.PublicationScopeLimited { + if p.Publication() == nil || p.Publication().Scope() != project.PublicationScopeLimited { return nil, interfaces.ErrInvalidProject } diff --git a/server/internal/usecase/repo/project.go b/server/internal/usecase/repo/project.go index 46b4025aec..7f0ac5095a 100644 --- a/server/internal/usecase/repo/project.go +++ b/server/internal/usecase/repo/project.go @@ -15,7 +15,7 @@ type Project interface { FindByID(context.Context, id.ProjectID) (*project.Project, error) FindByIDOrAlias(context.Context, project.IDOrAlias) (*project.Project, error) FindByWorkspaces(context.Context, accountdomain.WorkspaceIDList, *usecasex.Pagination) (project.List, *usecasex.PageInfo, error) - FindByPublicName(context.Context, string) (*project.Project, error) + IsAliasAvailable(context.Context, string) (bool, error) CountByWorkspace(context.Context, accountdomain.WorkspaceID) (int, error) FindByPublicAPIToken(context.Context, string) (*project.Project, error) Save(context.Context, *project.Project) error