Browse Source

Improve org error handling (#2117)

* Improve ErrOrgNotExist type
Return new error type
Use good error check
Use new method to check error
Update tests

* Fix unchanged method name report
Antoine GIRARD 2 years ago
parent
commit
30787e48f2
7 changed files with 26 additions and 12 deletions
  1. 16 0
      models/error.go
  2. 2 4
      models/org.go
  3. 1 1
      models/org_team.go
  4. 2 2
      models/org_test.go
  5. 3 3
      routers/api/v1/api.go
  6. 1 1
      routers/api/v1/repo/fork.go
  7. 1 1
      routers/api/v1/repo/repo.go

+ 16 - 0
models/error.go

@@ -448,6 +448,22 @@ func (err ErrAccessTokenEmpty) Error() string {
448 448
 // \_______  /__|  \___  (____  /___|  /__/_____ \(____  /__| |__|\____/|___|  /
449 449
 //         \/     /_____/     \/     \/         \/     \/                    \/
450 450
 
451
+// ErrOrgNotExist represents a "OrgNotExist" kind of error.
452
+type ErrOrgNotExist struct {
453
+	ID   int64
454
+	Name string
455
+}
456
+
457
+// IsErrOrgNotExist checks if an error is a ErrOrgNotExist.
458
+func IsErrOrgNotExist(err error) bool {
459
+	_, ok := err.(ErrOrgNotExist)
460
+	return ok
461
+}
462
+
463
+func (err ErrOrgNotExist) Error() string {
464
+	return fmt.Sprintf("org does not exist [id: %d, name: %s]", err.ID, err.Name)
465
+}
466
+
451 467
 // ErrLastOrgOwner represents a "LastOrgOwner" kind of error.
452 468
 type ErrLastOrgOwner struct {
453 469
 	UID int64

+ 2 - 4
models/org.go

@@ -16,8 +16,6 @@ import (
16 16
 )
17 17
 
18 18
 var (
19
-	// ErrOrgNotExist organization does not exist
20
-	ErrOrgNotExist = errors.New("Organization does not exist")
21 19
 	// ErrTeamNotExist team does not exist
22 20
 	ErrTeamNotExist = errors.New("Team does not exist")
23 21
 )
@@ -180,7 +178,7 @@ func CreateOrganization(org, owner *User) (err error) {
180 178
 // GetOrgByName returns organization by given name.
181 179
 func GetOrgByName(name string) (*User, error) {
182 180
 	if len(name) == 0 {
183
-		return nil, ErrOrgNotExist
181
+		return nil, ErrOrgNotExist{0, name}
184 182
 	}
185 183
 	u := &User{
186 184
 		LowerName: strings.ToLower(name),
@@ -190,7 +188,7 @@ func GetOrgByName(name string) (*User, error) {
190 188
 	if err != nil {
191 189
 		return nil, err
192 190
 	} else if !has {
193
-		return nil, ErrOrgNotExist
191
+		return nil, ErrOrgNotExist{0, name}
194 192
 	}
195 193
 	return u, nil
196 194
 }

+ 1 - 1
models/org_team.go

@@ -230,7 +230,7 @@ func NewTeam(t *Team) (err error) {
230 230
 	if err != nil {
231 231
 		return err
232 232
 	} else if !has {
233
-		return ErrOrgNotExist
233
+		return ErrOrgNotExist{t.OrgID, ""}
234 234
 	}
235 235
 
236 236
 	t.LowerName = strings.ToLower(t.Name)

+ 2 - 2
models/org_test.go

@@ -222,10 +222,10 @@ func TestGetOrgByName(t *testing.T) {
222 222
 	assert.Equal(t, "user3", org.Name)
223 223
 
224 224
 	org, err = GetOrgByName("user2") // user2 is an individual
225
-	assert.Equal(t, ErrOrgNotExist, err)
225
+	assert.True(t, IsErrOrgNotExist(err))
226 226
 
227 227
 	org, err = GetOrgByName("") // corner case
228
-	assert.Equal(t, ErrOrgNotExist, err)
228
+	assert.True(t, IsErrOrgNotExist(err))
229 229
 }
230 230
 
231 231
 func TestCountOrganizations(t *testing.T) {

+ 3 - 3
routers/api/v1/api.go

@@ -208,12 +208,12 @@ func orgAssignment(args ...bool) macaron.Handler {
208 208
 
209 209
 		var err error
210 210
 		if assignOrg {
211
-			ctx.Org.Organization, err = models.GetUserByName(ctx.Params(":orgname"))
211
+			ctx.Org.Organization, err = models.GetOrgByName(ctx.Params(":orgname"))
212 212
 			if err != nil {
213
-				if models.IsErrUserNotExist(err) {
213
+				if models.IsErrOrgNotExist(err) {
214 214
 					ctx.Status(404)
215 215
 				} else {
216
-					ctx.Error(500, "GetUserByName", err)
216
+					ctx.Error(500, "GetOrgByName", err)
217 217
 				}
218 218
 				return
219 219
 			}

+ 1 - 1
routers/api/v1/repo/fork.go

@@ -59,7 +59,7 @@ func CreateFork(ctx *context.APIContext, form api.CreateForkOption) {
59 59
 	} else {
60 60
 		org, err := models.GetOrgByName(*form.Organization)
61 61
 		if err != nil {
62
-			if err == models.ErrOrgNotExist {
62
+			if models.IsErrOrgNotExist(err) {
63 63
 				ctx.Error(422, "", err)
64 64
 			} else {
65 65
 				ctx.Error(500, "GetOrgByName", err)

+ 1 - 1
routers/api/v1/repo/repo.go

@@ -156,7 +156,7 @@ func CreateOrgRepo(ctx *context.APIContext, opt api.CreateRepoOption) {
156 156
 
157 157
 	org, err := models.GetOrgByName(ctx.Params(":org"))
158 158
 	if err != nil {
159
-		if models.IsErrUserNotExist(err) {
159
+		if models.IsErrOrgNotExist(err) {
160 160
 			ctx.Error(422, "", err)
161 161
 		} else {
162 162
 			ctx.Error(500, "GetOrgByName", err)