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

Derive profile name from display name in ProfileService.CreateProfile #4335

Merged
merged 7 commits into from
Sep 13, 2024
20 changes: 19 additions & 1 deletion internal/profiles/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"encoding/json"
"errors"
"fmt"

// ignore this linter warning - this is pre-existing code, and I do not
// want to change the logging library it uses at this time.
// nolint:depguard
Expand Down Expand Up @@ -129,14 +130,31 @@ func (p *profileService) CreateProfile(
PopulateRuleNames(profile)

displayName := profile.GetDisplayName()

listParams := db.ListProfilesByProjectIDAndLabelParams{
ProjectID: projectID,
}

existingProfiles, err := qtx.ListProfilesByProjectIDAndLabel(ctx, listParams)
if err != nil {
return nil, status.Errorf(codes.Unknown, "failed to get profiles: %s", err)
}

profileMap := MergeDatabaseListIntoProfiles(existingProfiles)

existingProfileNames := make([]string, 0, len(profileMap))

// Derive the profile name from the profile display name
name := DeriveProfileNameFromDisplayName(profile, existingProfileNames)

// if empty use the name
if displayName == "" {
displayName = profile.GetName()
}

params := db.CreateProfileParams{
ProjectID: projectID,
Name: profile.GetName(),
Name: name,
DisplayName: displayName,
Labels: profile.GetLabels(),
Remediate: db.ValidateRemediateType(profile.GetRemediate()),
Expand Down
67 changes: 67 additions & 0 deletions internal/profiles/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import (
"io"
"os"
"path/filepath"
"regexp"
"strings"

"github.com/rs/zerolog/log"
"github.com/sqlc-dev/pqtype"
Expand All @@ -32,6 +34,12 @@ import (
pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
)

var nonAlphanumericRegex = regexp.MustCompile(`[^a-zA-Z0-9\s]`)

var multipleSpacesRegex = regexp.MustCompile(`\s{2,}`)

const profileNameMaxLength = 63

// RuleValidationError is used to report errors from evaluating a rule, including
// attribution of the particular error encountered.
type RuleValidationError struct {
Expand Down Expand Up @@ -299,6 +307,65 @@ func MergeDatabaseGetByNameIntoProfiles(ppl []db.GetProfileByProjectAndNameRow)
return profiles
}

// DeriveProfileNameFromDisplayName generates a unique profile name based on the display name and existing profiles.
func DeriveProfileNameFromDisplayName(
profile *pb.Profile,
existingProfileNames []string,
) (name string) {

displayName := profile.GetDisplayName()
name = profile.GetName()

if displayName != "" && name == "" {
// when a display name is provided, but no profile name
// then the profile name is created and saved based on the profile display name
name = cleanDisplayName(displayName)
}
// when both a display name and a profile name are provided
// then the profile name from the incoming request is used as the profile name

derivedName := name
counter := 1

// check if the current project already has a profile with that name, then add a counter
for strings.Contains(strings.Join(existingProfileNames, " "), derivedName) {
derivedName = fmt.Sprintf("%s-%d", name, counter)
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a few edge cases here where the name will exceed 63 characters once we append the counter.
If the counter is a single digit, then we need to remove 2 character from the name, if it's longer than 61 characters.
As the counter grows, it means we'll have to remove more characters from the name, for example, it we append -123 then we need to remove 4 characters from the name, if it's longer than 61 characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I fixed the logic to handle the edge cases mentioned as well as added unit tests for them. @eleftherias

if len(derivedName) > profileNameMaxLength {
nameLength := profileNameMaxLength - len(fmt.Sprintf("-%d", counter))
derivedName = fmt.Sprintf("%s-%d", name[:nameLength], counter)
}
counter++
}
return derivedName

}

// The profile name should be derived from the profile display name given the following logic
func cleanDisplayName(displayName string) string {

// Trim leading and trailing whitespace
displayName = strings.TrimSpace(displayName)

// Remove non-alphanumeric characters
displayName = nonAlphanumericRegex.ReplaceAllString(displayName, "")

// Replace multiple spaces with a single space
displayName = multipleSpacesRegex.ReplaceAllString(displayName, " ")

// Replace all whitespace with underscores
displayName = strings.ReplaceAll(displayName, " ", "_")

// Convert to lower-case
displayName = strings.ToLower(displayName)

// Trim to a maximum length of 63 characters
if len(displayName) > profileNameMaxLength {
displayName = displayName[:profileNameMaxLength]
}

return displayName
}

func dbProfileToPB(p db.Profile) *pb.Profile {
profileID := p.ID.String()
project := p.ProjectID.String()
Expand Down
152 changes: 152 additions & 0 deletions internal/profiles/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -634,3 +634,155 @@ func TestFilterRulesForType(t *testing.T) {
})
}
}

func TestDeriveProfileNameFromDisplayName(t *testing.T) {
t.Parallel()
tests := []struct {
name string
profile *minderv1.Profile
existingProfileNames []string
expected string
}{
{
name: "A short DisplayName with whitespace",
profile: &minderv1.Profile{
Name: "profile_name",
DisplayName: "",
},
existingProfileNames: []string{},
expected: "profile_name",
},
{
name: "A short DisplayName with whitespace",
profile: &minderv1.Profile{
Name: "",
DisplayName: "My custom profile",
},
existingProfileNames: []string{},
expected: "my_custom_profile",
},
{
name: "A very long DisplayName with whitespaces and more than 63 characters",
profile: &minderv1.Profile{
Name: "",
DisplayName: "A very long profile name that is longer than sixty three characters and will be trimmed",
},
existingProfileNames: []string{},
expected: "a_very_long_profile_name_that_is_longer_than_sixty_three_charac",
},
{
name: "A DisplayName with special characters",
profile: &minderv1.Profile{
Name: "",
DisplayName: "Profile with !#$() characters",
},
existingProfileNames: []string{},
expected: "profile_with_characters",
},
{
name: "A DisplayName with alphanumeric values",
profile: &minderv1.Profile{
Name: "",
DisplayName: "My 1st Profile",
},
existingProfileNames: []string{},
expected: "my_1st_profile",
},
{
name: "A DisplayName with non-alphanumeric characters and leadning & trailing whitespaces",
profile: &minderv1.Profile{
Name: "",
DisplayName: " New, Profile! 123. This is a Test Display Name with Special Characters! ",
},
existingProfileNames: []string{},
expected: "new_profile_123_this_is_a_test_display_name_with_special_charac",
},
{
name: "A DisplayName with Leading and trailing white spaces",
profile: &minderv1.Profile{
Name: "",
DisplayName: " Leading and trailing spaces ",
},
existingProfileNames: []string{},
expected: "leading_and_trailing_spaces",
},
{
name: "A DisplayName with mix of upper and low case",
profile: &minderv1.Profile{
Name: "",
DisplayName: "UPPER CASE to lower case",
},
existingProfileNames: []string{},
expected: "upper_case_to_lower_case",
},
{
name: "Derived profile name does not exist in the current project",
profile: &minderv1.Profile{
Name: "",
DisplayName: "My profile",
},
existingProfileNames: []string{"other_profile", "custom_profile"},
expected: "my_profile",
},
{
name: "Derived profile name that does exist in the current project",
profile: &minderv1.Profile{
Name: "",
DisplayName: "My profile",
},
existingProfileNames: []string{"other_profile", "my_profile"},
expected: "my_profile-1",
},
{
name: "Derived profile name which does exist in the current project, when adding a counter to the name",
profile: &minderv1.Profile{
Name: "",
DisplayName: "My profile",
},
existingProfileNames: []string{"other_profile", "my_profile", "my_profile-1"},
expected: "my_profile-2",
},
{
name: "Derived profile name for the edge case: name exceeds 63 characters with single digit counter",
profile: &minderv1.Profile{
Name: "",
DisplayName: "This is a very long display name that will exceed the limit when counter is added",
},
existingProfileNames: []string{"this_is_a_very_long_display_name_that_will_exceed_the_limit_when"},
expected: "this_is_a_very_long_display_name_that_will_exceed_the_limit_w-1",
},
{
name: "Derived profile name for the edge case: name exceeds 63 characters with double digit counter",
profile: &minderv1.Profile{
Name: "",
DisplayName: "This is a very long display name that will exceed the limit when counter is added",
},
existingProfileNames: []string{
"this_is_a_very_long_display_name_that_will_exceed_the_limit_when",
"this_is_a_very_long_display_name_that_will_exceed_the_limit_w-1",
"this_is_a_very_long_display_name_that_will_exceed_the_limit_w-2",
"this_is_a_very_long_display_name_that_will_exceed_the_limit_w-3",
"this_is_a_very_long_display_name_that_will_exceed_the_limit_w-4",
"this_is_a_very_long_display_name_that_will_exceed_the_limit_w-5",
"this_is_a_very_long_display_name_that_will_exceed_the_limit_w-6",
"this_is_a_very_long_display_name_that_will_exceed_the_limit_w-7",
"this_is_a_very_long_display_name_that_will_exceed_the_limit_w-8",
"this_is_a_very_long_display_name_that_will_exceed_the_limit_w-9",
},
expected: "this_is_a_very_long_display_name_that_will_exceed_the_limit_-10",
},
}

for _, tt := range tests {

tt := tt
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

result := profiles.DeriveProfileNameFromDisplayName(tt.profile, tt.existingProfileNames)
if result != tt.expected {
t.Errorf("DeriveProfileNameFromDisplayName: for profile %+v, expected %s, but got %s", tt.profile, tt.expected, result)
}
})
}
}
Loading