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.

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

bugfix: fix an int parsing bug in godotenv.Marshal #235

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

bugfix: fix an int parsing bug in godotenv.Marshal #235

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8e06a02
fix an int parsing bug in godotenv.Marshal
t3mp14r3 Aug 30, 2024
de05e2d
got rid of regex, added a check for negative numbers, added a test
t3mp14r3 Sep 2, 2024
2f570db
fix '-' character trimming and expand the test
t3mp14r3 Sep 2, 2024
2099107
replace unicode.IsDigit for number checking
t3mp14r3 Sep 2, 2024
bfb9a56
add the comment to isInt, run go fmt
t3mp14r3 Sep 2, 2024
f7b3a6b
add really big numbers isInt tests
t3mp14r3 Sep 3, 2024
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
22 changes: 19 additions & 3 deletions godotenv.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"os"
"os/exec"
"sort"
"strconv"
"strings"
)

Expand Down Expand Up @@ -159,13 +158,30 @@ func Write(envMap map[string]string, filename string) error {
return file.Sync()
}

func isInt(s string) bool {
Copy link

Choose a reason for hiding this comment

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

Please add a comment about the function purpose, people may be surprised about the fact you didn't use strings.Atoi

  • the fact it supports big numbers with more than X digits
  • the fact a number starting with + is not a valid number (and more likely to be a phone)

Copy link
Author

Choose a reason for hiding this comment

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

Added the comment, though it doesn't seem necessary (other internal functions don't have them), but let's let it be.

s = strings.TrimPrefix(s, "-")

if len(s) == 0 {
return false
}

for _, r := range s {
if '0' <= r && r <= '9' {
continue
}
return false
}

return true
}
Copy link

Choose a reason for hiding this comment

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

Please format the file with gofmt, there is something strange with the code now

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, something weird was going on, fixed that


// Marshal outputs the given environment as a dotenv-formatted environment file.
// Each line is in the format: KEY="VALUE" where VALUE is backslash-escaped.
func Marshal(envMap map[string]string) (string, error) {
lines := make([]string, 0, len(envMap))
for k, v := range envMap {
if d, err := strconv.Atoi(v); err == nil {
lines = append(lines, fmt.Sprintf(`%s=%d`, k, d))
if isInt(v) {
lines = append(lines, fmt.Sprintf(`%s=%s`, k, v))
} else {
lines = append(lines, fmt.Sprintf(`%s="%s"`, k, doubleQuoteEscape(v)))
}
Expand Down
22 changes: 22 additions & 0 deletions godotenv_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,28 @@ func TestComments(t *testing.T) {
loadEnvAndCompareValues(t, Load, envFileName, expectedValues, noopPresets)
}

func TestIsInt(t *testing.T) {
checkAndCompare := func(s string, expected bool) {
if isInt(s) != expected {
t.Fail()
}
}

// invalid values
checkAndCompare("", false)
checkAndCompare("+123", false)
checkAndCompare("+12a3", false)
checkAndCompare("12a3", false)
checkAndCompare("abc", false)
checkAndCompare("12 3", false)
checkAndCompare("-", false)
checkAndCompare(" ", false)

// valid values
checkAndCompare("-123", true)
checkAndCompare("123", true)
}

func TestWrite(t *testing.T) {
writeAndCompare := func(env string, expected string) {
envMap, _ := Unmarshal(env)
Expand Down
Loading