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

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
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
25 changes: 22 additions & 3 deletions godotenv.go
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,33 @@ func Write(envMap map[string]string, filename string) error {
return file.Sync()
}

// isInt checks if the string may be serialized as a number value, leading
// "-" symbol is allowed for negative numbers, leading "+" sign is not. The
// length of the value is not limited.
func isInt(s string) bool {

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
}

// 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
38 changes: 30 additions & 8 deletions godotenv_test.go
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)

Choose a reason for hiding this comment

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

Please add a test about a very long number that would break max int64

It's part of your code change

Copy link
Author

Choose a reason for hiding this comment

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

The update is here

}

func TestWrite(t *testing.T) {
writeAndCompare := func(env string, expected string) {
envMap, _ := Unmarshal(env)
Expand Down Expand Up @@ -582,42 +604,42 @@ func TestWhitespace(t *testing.T) {
}{
"Leading whitespace": {
input: " A=a\n",
key: "A",
key: "A",
value: "a",
},
"Leading tab": {
input: "\tA=a\n",
key: "A",
key: "A",
value: "a",
},
"Leading mixed whitespace": {
input: " \t \t\n\t \t A=a\n",
key: "A",
key: "A",
value: "a",
},
"Leading whitespace before export": {
input: " \t\t export A=a\n",
key: "A",
key: "A",
value: "a",
},
"Trailing whitespace": {
input: "A=a \t \t\n",
key: "A",
key: "A",
value: "a",
},
"Trailing whitespace with export": {
input: "export A=a\t \t \n",
key: "A",
key: "A",
value: "a",
},
"No EOL": {
input: "A=a",
key: "A",
key: "A",
value: "a",
},
"Trailing whitespace with no EOL": {
input: "A=a ",
key: "A",
key: "A",
value: "a",
},
}
Expand Down
Loading