Improve validation error messages (#33)

* Improve validation error messages * Bump version number
man90es · Sep 11, 2024 · a740c1e · a740c1e
1 parent d9b5ae7
commit a740c1e
Show file tree

Hide file tree

Showing 15 changed files with 132 additions and 96 deletions.
diff --git a/handlers/catchall.go b/handlers/catchall.go
@@ -5,5 +5,5 @@ import (
 )
 
 func catchall(w http.ResponseWriter, r *http.Request) {
-	giveBadRequestResponse(w)
+	giveBadRequestResponse(w, "Requested route is invalid. See documentation "+docsLink)
 }
diff --git a/handlers/getAdventurer.go b/handlers/getAdventurer.go
@@ -13,11 +13,15 @@ import (
 var profilesCache = cache.NewCache[models.Profile]()
 
 func getAdventurer(w http.ResponseWriter, r *http.Request) {
-	profileTarget, profileTargetOk := validators.ValidateProfileTargetQueryParam(r.URL.Query()["profileTarget"])
-	region, regionOk := validators.ValidateRegionQueryParam(r.URL.Query()["region"])
+	profileTarget, profileTargetOk, profileTargetValidationMessage := validators.ValidateProfileTargetQueryParam(r.URL.Query()["profileTarget"])
+	if !profileTargetOk {
+		giveBadRequestResponse(w, profileTargetValidationMessage)
+		return
+	}
 
-	if !profileTargetOk || !regionOk {
-		giveBadRequestResponse(w)
+	region, regionOk, regionValidationMessage := validators.ValidateRegionQueryParam(r.URL.Query()["region"])
+	if !regionOk {
+		giveBadRequestResponse(w, regionValidationMessage)
 		return
 	}
 

diff --git a/handlers/getAdventurerSearch.go b/handlers/getAdventurerSearch.go
@@ -15,15 +15,15 @@ import (
 var profileSearchCache = cache.NewCache[[]models.Profile]()
 
 func getAdventurerSearch(w http.ResponseWriter, r *http.Request) {
-	region, regionOk := validators.ValidateRegionQueryParam(r.URL.Query()["region"])
+	region, regionOk, regionValidationMessage := validators.ValidateRegionQueryParam(r.URL.Query()["region"])
 	if !regionOk {
-		giveBadRequestResponse(w)
+		giveBadRequestResponse(w, regionValidationMessage)
 		return
 	}
 
-	query, queryOk := validators.ValidateAdventurerNameQueryParam(r.URL.Query()["query"], region)
+	query, queryOk, queryValidationMessage := validators.ValidateAdventurerNameQueryParam(r.URL.Query()["query"], region)
 	if !queryOk {
-		giveBadRequestResponse(w)
+		giveBadRequestResponse(w, queryValidationMessage)
 		return
 	}
 

diff --git a/handlers/getGuild.go b/handlers/getGuild.go
@@ -14,11 +14,15 @@ import (
 var guildProfilesCache = cache.NewCache[models.GuildProfile]()
 
 func getGuild(w http.ResponseWriter, r *http.Request) {
-	name, nameOk := validators.ValidateGuildNameQueryParam(r.URL.Query()["guildName"])
-	region, regionOk := validators.ValidateRegionQueryParam(r.URL.Query()["region"])
+	name, nameOk, nameValidationMessage := validators.ValidateGuildNameQueryParam(r.URL.Query()["guildName"])
+	if !nameOk {
+		giveBadRequestResponse(w, nameValidationMessage)
+		return
+	}
 
-	if !nameOk || !regionOk {
-		giveBadRequestResponse(w)
+	region, regionOk, regionValidationMessage := validators.ValidateRegionQueryParam(r.URL.Query()["region"])
+	if !regionOk {
+		giveBadRequestResponse(w, regionValidationMessage)
 		return
 	}
 

diff --git a/handlers/getGuildSearch.go b/handlers/getGuildSearch.go
@@ -15,12 +15,17 @@ import (
 var guildSearchCache = cache.NewCache[[]models.GuildProfile]()
 
 func getGuildSearch(w http.ResponseWriter, r *http.Request) {
-	name, nameOk := validators.ValidateGuildNameQueryParam(r.URL.Query()["query"])
+	name, nameOk, nameValidationMessage := validators.ValidateGuildNameQueryParam(r.URL.Query()["query"])
+	if !nameOk {
+		giveBadRequestResponse(w, nameValidationMessage)
+		return
+	}
+
 	page := validators.ValidatePageQueryParam(r.URL.Query()["page"])
-	region, regionOk := validators.ValidateRegionQueryParam(r.URL.Query()["region"])
 
-	if !nameOk || !regionOk {
-		giveBadRequestResponse(w)
+	region, regionOk, regionValidationMessage := validators.ValidateRegionQueryParam(r.URL.Query()["region"])
+	if !regionOk {
+		giveBadRequestResponse(w, regionValidationMessage)
 		return
 	}
 

diff --git a/handlers/getStatus.go b/handlers/getStatus.go
@@ -10,7 +10,7 @@ import (
 )
 
 var initTime = time.Now()
-var version = "1.9.1"
+var version = "1.9.3"
 
 func getStatus(w http.ResponseWriter, r *http.Request) {
 	json.NewEncoder(w).Encode(map[string]interface{}{

diff --git a/handlers/giveBadRequestResponse.go b/handlers/giveBadRequestResponse.go
@@ -7,10 +7,10 @@ import (
 
 const docsLink = "https://man90es.github.io/BDO-REST-API"
 
-func giveBadRequestResponse(w http.ResponseWriter) {
+func giveBadRequestResponse(w http.ResponseWriter, message string) {
 	w.WriteHeader(http.StatusBadRequest)
 
 	json.NewEncoder(w).Encode(map[string]string{
-		"message": "Route or parameter is invalid. See documentation " + docsLink,
+		"message": message,
 	})
 }
diff --git a/validators/ValidateAdventurerNameQueryParam.go b/validators/ValidateAdventurerNameQueryParam.go
@@ -1,15 +1,16 @@
 package validators
 
 import (
+	"fmt"
 	"strings"
 	"unicode"
 )
 
 // The naming policies in BDO are fucked up
 // This function only checks the length and allowed symbols
-func ValidateAdventurerNameQueryParam(query []string, region string) (name string, ok bool) {
+func ValidateAdventurerNameQueryParam(query []string, region string) (name string, ok bool, errorMessage string) {
 	if 1 > len(query) {
-		return "", false
+		return "", false, "Adventurer name is missing from request"
 	}
 
 	minLength := map[string]int{
@@ -19,7 +20,7 @@ func ValidateAdventurerNameQueryParam(query []string, region string) (name strin
 	}[region]
 
 	if len(query[0]) < minLength || len(query[0]) > 16 {
-		return query[0], false
+		return query[0], false, fmt.Sprintf("Adventurer name should be between %v and 16 symbols long", minLength)
 	}
 
 	// Returns false for allowed characters
@@ -48,5 +49,9 @@ func ValidateAdventurerNameQueryParam(query []string, region string) (name strin
 		return true
 	}
 
-	return query[0], strings.IndexFunc(query[0], f) == -1
+	if i := strings.IndexFunc(query[0], f); i != -1 {
+		return query[0], false, fmt.Sprintf("Adventurer name contains a forbidden symbol at position %v: %q", i+1, query[0][i])
+	}
+
+	return query[0], true, ""
 }
diff --git a/validators/ValidateAdventurerNameQueryParam_test.go b/validators/ValidateAdventurerNameQueryParam_test.go
@@ -4,32 +4,33 @@ import "testing"
 
 func TestValidateAdventurerNameQueryParam(t *testing.T) {
 	tests := []struct {
-		expectedName string
-		expectedOk   bool
-		input        []string
-		region       string
+		expectedName    string
+		expectedOk      bool
+		expectedMessage string
+		input           []string
+		region          string
 	}{
-		{input: []string{"1Number"}, region: "EU", expectedName: "1Number", expectedOk: true}, // Starts with a number
-		{input: []string{"Adventurer_123"}, region: "EU", expectedName: "Adventurer_123", expectedOk: true},
-		{input: []string{"JohnDoe"}, region: "EU", expectedName: "JohnDoe", expectedOk: true},
-		{input: []string{"Name1", "Name2"}, region: "EU", expectedName: "Name1", expectedOk: true},
-		{input: []string{"고대신"}, region: "EU", expectedName: "고대신", expectedOk: true}, // Adventurer name with Korean characters
+		{input: []string{"1Number"}, region: "EU", expectedName: "1Number", expectedOk: true, expectedMessage: ""},
+		{input: []string{"Adventurer_123"}, region: "EU", expectedName: "Adventurer_123", expectedOk: true, expectedMessage: ""},
+		{input: []string{"JohnDoe"}, region: "EU", expectedName: "JohnDoe", expectedOk: true, expectedMessage: ""},
+		{input: []string{"Name1", "Name2"}, region: "EU", expectedName: "Name1", expectedOk: true, expectedMessage: ""},
+		{input: []string{"고대신"}, region: "EU", expectedName: "고대신", expectedOk: true, expectedMessage: ""},
 
-		{input: []string{""}, region: "EU", expectedName: "", expectedOk: false},                                                     // Empty adventurer name
-		{input: []string{"Ad"}, region: "EU", expectedName: "Ad", expectedOk: false},                                                 // Too short
-		{input: []string{"Adventurer With Spaces"}, region: "EU", expectedName: "Adventurer With Spaces", expectedOk: false},         // Contains spaces
-		{input: []string{"AdventurerNameTooLong12345"}, region: "EU", expectedName: "AdventurerNameTooLong12345", expectedOk: false}, // Too long
-		{input: []string{"Name$"}, region: "EU", expectedName: "Name$", expectedOk: false},                                           // Contains an invalid symbol
-		{input: []string{}, region: "EU", expectedName: "", expectedOk: false},
+		{input: []string{""}, region: "EU", expectedName: "", expectedOk: false, expectedMessage: "Adventurer name should be between 3 and 16 symbols long"},
+		{input: []string{"Ad"}, region: "EU", expectedName: "Ad", expectedOk: false, expectedMessage: "Adventurer name should be between 3 and 16 symbols long"},
+		{input: []string{"With Spaces"}, region: "EU", expectedName: "With Spaces", expectedOk: false, expectedMessage: "Adventurer name contains a forbidden symbol at position 5: ' '"},
+		{input: []string{"AdventurerNameTooLong12345"}, region: "EU", expectedName: "AdventurerNameTooLong12345", expectedOk: false, expectedMessage: "Adventurer name should be between 3 and 16 symbols long"},
+		{input: []string{"Name$"}, region: "EU", expectedName: "Name$", expectedOk: false, expectedMessage: "Adventurer name contains a forbidden symbol at position 5: '$'"},
+		{input: []string{}, region: "EU", expectedName: "", expectedOk: false, expectedMessage: "Adventurer name is missing from request"},
 
-		{input: []string{""}, region: "SA", expectedName: "", expectedOk: false},
-		{input: []string{"Ad"}, region: "SA", expectedName: "Ad", expectedOk: true},
+		{input: []string{""}, region: "SA", expectedName: "", expectedOk: false, expectedMessage: "Adventurer name should be between 2 and 16 symbols long"},
+		{input: []string{"Ad"}, region: "SA", expectedName: "Ad", expectedOk: true, expectedMessage: ""},
 	}
 
 	for _, test := range tests {
-		name, ok := ValidateAdventurerNameQueryParam(test.input, test.region)
-		if name != test.expectedName || ok != test.expectedOk {
-			t.Errorf("Input: %v %v, Expected: %v %v, Got: %v %v", test.input, test.region, test.expectedName, test.expectedOk, name, ok)
+		name, ok, message := ValidateAdventurerNameQueryParam(test.input, test.region)
+		if name != test.expectedName || ok != test.expectedOk || message != test.expectedMessage {
+			t.Errorf("Input: %v %v, Expected: %v %v %v, Got: %v %v %v", test.input, test.region, test.expectedName, test.expectedOk, test.expectedMessage, name, ok, message)
 		}
 	}
 }
diff --git a/validators/ValidateGuildNameQueryParam.go b/validators/ValidateGuildNameQueryParam.go
@@ -1,22 +1,23 @@
 package validators
 
 import (
+	"fmt"
 	"strings"
 	"unicode"
 )
 
 // The naming policies in BDO are fucked up
 // This function only checks the length and allowed symbols
 // I also assumed that the allowed symbols are the same as for adventurer names
-func ValidateGuildNameQueryParam(query []string) (guildName string, ok bool) {
+func ValidateGuildNameQueryParam(query []string) (guildName string, ok bool, errorMessage string) {
 	if 1 > len(query) {
-		return "", false
+		return "", false, "Guild name is missing from request"
 	}
 
 	guildName = strings.ToLower(query[0])
 
 	if len(guildName) < 2 {
-		return guildName, false
+		return guildName, false, "Guild name can't be shorter than 2 symbols"
 	}
 
 	// Returns false for allowed characters
@@ -45,5 +46,9 @@ func ValidateGuildNameQueryParam(query []string) (guildName string, ok bool) {
 		return true
 	}
 
-	return guildName, strings.IndexFunc(guildName, f) == -1
+	if i := strings.IndexFunc(guildName, f); i != -1 {
+		return guildName, false, fmt.Sprintf("Guild name contains a forbidden symbol at position %v: %q", i+1, guildName[i])
+	}
+
+	return guildName, true, ""
 }
diff --git a/validators/ValidateGuildNameQueryParam_test.go b/validators/ValidateGuildNameQueryParam_test.go
@@ -4,27 +4,28 @@ import "testing"
 
 func TestValidateGuildNameQueryParam(t *testing.T) {
 	tests := []struct {
-		expectedName string
-		expectedOk   bool
-		input        []string
+		expectedName    string
+		expectedOk      bool
+		expectedMessage string
+		input           []string
 	}{
-		{input: []string{"1NumberGuild"}, expectedName: "1numberguild", expectedOk: true}, // Contains a number
-		{input: []string{"Adventure_Guild"}, expectedName: "adventure_guild", expectedOk: true},
-		{input: []string{"FirstGuild", "SecondGuild"}, expectedName: "firstguild", expectedOk: true},
-		{input: []string{"MyGuild"}, expectedName: "myguild", expectedOk: true},
-		{input: []string{"고대신"}, expectedName: "고대신", expectedOk: true}, // Guild name with Korean characters
+		{input: []string{"1NumberGuild"}, expectedName: "1numberguild", expectedOk: true, expectedMessage: ""}, // Contains a number
+		{input: []string{"Adventure_Guild"}, expectedName: "adventure_guild", expectedOk: true, expectedMessage: ""},
+		{input: []string{"FirstGuild", "SecondGuild"}, expectedName: "firstguild", expectedOk: true, expectedMessage: ""},
+		{input: []string{"MyGuild"}, expectedName: "myguild", expectedOk: true, expectedMessage: ""},
+		{input: []string{"고대신"}, expectedName: "고대신", expectedOk: true, expectedMessage: ""}, // Guild name with Korean characters
 
-		{input: []string{""}, expectedName: "", expectedOk: false},                                       // Empty guild name
-		{input: []string{"A Guild With Spaces"}, expectedName: "a guild with spaces", expectedOk: false}, // Contains spaces
-		{input: []string{"Some$"}, expectedName: "some$", expectedOk: false},                             // Contains an invalid symbol
-		{input: []string{"x"}, expectedName: "x", expectedOk: false},                                     // Too short
-		{input: []string{}, expectedName: "", expectedOk: false},
+		{input: []string{""}, expectedName: "", expectedOk: false, expectedMessage: "Guild name can't be shorter than 2 symbols"},
+		{input: []string{"A Guild With Spaces"}, expectedName: "a guild with spaces", expectedOk: false, expectedMessage: "Guild name contains a forbidden symbol at position 2: ' '"},
+		{input: []string{"Some$"}, expectedName: "some$", expectedOk: false, expectedMessage: "Guild name contains a forbidden symbol at position 5: '$'"},
+		{input: []string{"x"}, expectedName: "x", expectedOk: false, expectedMessage: "Guild name can't be shorter than 2 symbols"},
+		{input: []string{}, expectedName: "", expectedOk: false, expectedMessage: "Guild name is missing from request"},
 	}
 
 	for _, test := range tests {
-		name, ok := ValidateGuildNameQueryParam(test.input)
-		if name != test.expectedName || ok != test.expectedOk {
-			t.Errorf("Input: %v, Expected: %v %v, Got: %v %v", test.input, test.expectedName, test.expectedOk, name, ok)
+		name, ok, message := ValidateGuildNameQueryParam(test.input)
+		if name != test.expectedName || ok != test.expectedOk || message != test.expectedMessage {
+			t.Errorf("Input: %v, Expected: %v %v %v, Got: %v %v %v", test.input, test.expectedName, test.expectedOk, test.expectedMessage, name, ok, message)
 		}
 	}
 }
diff --git a/validators/ValidateProfileTargetQueryParam.go b/validators/ValidateProfileTargetQueryParam.go
@@ -2,10 +2,14 @@ package validators
 
 // Check that the length is at least 150 characters
 // I don't actually know how long it should be, but the length varies
-func ValidateProfileTargetQueryParam(query []string) (profileTarget string, ok bool) {
+func ValidateProfileTargetQueryParam(query []string) (profileTarget string, ok bool, errorMessage string) {
 	if 1 > len(query) {
-		return "", false
+		return "", false, "Profile target is missing from the request"
 	}
 
-	return query[0], len(query[0]) >= 150
+	if ok := len(query[0]) >= 150; ok {
+		return query[0], true, ""
+	}
+
+	return query[0], false, "Profile target has to be at least 150 characters long"
 }
diff --git a/validators/ValidateProfileTargetQueryParam_test.go b/validators/ValidateProfileTargetQueryParam_test.go
@@ -4,30 +4,31 @@ import "testing"
 
 func TestValidateProfileTargetQueryParam(t *testing.T) {
 	tests := []struct {
-		expectedOk bool
-		expectedPT string
-		input      []string
+		expectedOk      bool
+		expectedPT      string
+		expectedMessage string
+		input           []string
 	}{
 		// Valid profile targets with lengths >= 150
-		{input: []string{repeat("A", 150)}, expectedPT: repeat("A", 150), expectedOk: true},
-		{input: []string{repeat("A", 200)}, expectedPT: repeat("A", 200), expectedOk: true},
+		{input: []string{repeat("A", 150)}, expectedPT: repeat("A", 150), expectedOk: true, expectedMessage: ""},
+		{input: []string{repeat("A", 200)}, expectedPT: repeat("A", 200), expectedOk: true, expectedMessage: ""},
 
 		// Invalid profile targets with lengths < 150
-		{input: []string{""}, expectedPT: "", expectedOk: false},
-		{input: []string{"Short"}, expectedPT: "Short", expectedOk: false},
-		{input: []string{repeat("A", 149)}, expectedPT: repeat("A", 149), expectedOk: false},
+		{input: []string{""}, expectedPT: "", expectedOk: false, expectedMessage: "Profile target has to be at least 150 characters long"},
+		{input: []string{"Short"}, expectedPT: "Short", expectedOk: false, expectedMessage: "Profile target has to be at least 150 characters long"},
+		{input: []string{repeat("A", 149)}, expectedPT: repeat("A", 149), expectedOk: false, expectedMessage: "Profile target has to be at least 150 characters long"},
 
 		// Query param not provided
-		{input: []string{}, expectedPT: "", expectedOk: false},
+		{input: []string{}, expectedPT: "", expectedOk: false, expectedMessage: "Profile target is missing from the request"},
 
 		// Several profileTargets provided
-		{input: []string{repeat("A", 150), repeat("B", 150)}, expectedPT: repeat("A", 150), expectedOk: true},
+		{input: []string{repeat("A", 150), repeat("B", 150)}, expectedPT: repeat("A", 150), expectedOk: true, expectedMessage: ""},
 	}
 
 	for _, test := range tests {
-		pT, ok := ValidateProfileTargetQueryParam(test.input)
-		if pT != test.expectedPT || ok != test.expectedOk {
-			t.Errorf("Input: %v, Expected: %v %v, Got: %v %v", test.input, test.expectedPT, test.expectedOk, pT, ok)
+		pT, ok, message := ValidateProfileTargetQueryParam(test.input)
+		if pT != test.expectedPT || ok != test.expectedOk || message != test.expectedMessage {
+			t.Errorf("Input: %v, Expected: %v %v %v, Got: %v %v %v", test.input, test.expectedPT, test.expectedOk, test.expectedMessage, pT, ok, message)
 		}
 	}
 }

diff --git a/validators/ValidateRegionQueryParam.go b/validators/ValidateRegionQueryParam.go
@@ -1,17 +1,22 @@
 package validators
 
 import (
+	"fmt"
 	"slices"
 	"strings"
 )
 
-func ValidateRegionQueryParam(query []string) (region string, ok bool) {
+func ValidateRegionQueryParam(query []string) (region string, ok bool, errorMessage string) {
 	if 1 > len(query) {
-		return "EU", true
+		return "EU", true, ""
 	}
 
 	region = strings.ToUpper(query[0])
 
 	// TODO: Add KR region once the translations are ready
-	return region, slices.Contains([]string{"EU", "NA", "SA"}, region)
+	if !slices.Contains([]string{"EU", "NA", "SA"}, region) {
+		return region, false, fmt.Sprintf("Region %v is not supported", region)
+	}
+
+	return region, true, ""
 }