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

Jump to bottom

fix: Correctly parse newline and multiline strings in dotenv files #192

Merged
merged 3 commits into from
Aug 19, 2024
Merged

fix: Correctly parse newline and multiline strings in dotenv files #192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5901e82
test: Add tests for correctly escaping new lines characters `\n`
FranzGB Jun 11, 2024
c1ea77b
fix: Correctly parse newline and multiline strings in dotenv files
FranzGB Jun 11, 2024
5dff2e9
test: Enhance test cases for more robust `\n`
FranzGB Jun 11, 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
8 changes: 8 additions & 0 deletions spec/Configuration/Dotenv/TextSpec.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
it "returns variables from a file without changing the environment" $ do
lookupEnv "DOTENV" `shouldReturn` Nothing

liftM head (parseFile "spec/fixtures/.dotenv") `shouldReturn`

Check warning on line 22 in spec/Configuration/Dotenv/TextSpec.hs

View workflow job for this annotation

GitHub Actions / GHC 9.8 on macos-13 

In the use of ‘head’

Check warning on line 22 in spec/Configuration/Dotenv/TextSpec.hs

View workflow job for this annotation

GitHub Actions / GHC 9.8 on ubuntu-latest 

In the use of ‘head’
(T.pack "DOTENV", T.pack "true")

lookupEnv "DOTENV" `shouldReturn` Nothing
Expand All @@ -27,3 +27,11 @@
it "recognizes unicode characters" $
liftM (!! 1) (parseFile "spec/fixtures/.dotenv") `shouldReturn`
(T.pack "UNICODE_TEST", T.pack "Manabí")

it "handles newline characters correctly" $ do
liftM (!! 6) (parseFile "spec/fixtures/.dotenv") `shouldReturn`
(T.pack "NEWLINE_TEST", T.pack "Hello\nWorld\nThis is a test\n")

it "handles manual line breaks correctly" $ do
liftM (!! 7) (parseFile "spec/fixtures/.dotenv") `shouldReturn`
(T.pack "MULTILINE_TEST", T.pack "Roses are red\nViolets are blue\nCode is my art\nAnd bugs are my glue\n")
6 changes: 6 additions & 0 deletions spec/fixtures/.dotenv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,9 @@ ENVIRONMENT="$HOME"
PREVIOUS="$DOTENV"
ME="$(whoami)"
BLANK=
NEWLINE_TEST="Hello\nWorld\nThis is a test\n"
MULTILINE_TEST="Roses are red
Violets are blue
Code is my art
And bugs are my glue
"
5 changes: 3 additions & 2 deletions src/Configuration/Dotenv/Parse.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import Text.Megaparsec (Parsec, anySingle,
between, eof, noneOf,
oneOf, sepEndBy, (<?>))
import Text.Megaparsec.Char (char, digitChar, eol,
letterChar, spaceChar)
letterChar, spaceChar, string)
import qualified Text.Megaparsec.Char.Lexer as L

type Parser = Parsec Void String
Expand Down Expand Up @@ -85,8 +85,9 @@ interpolatedValueCommandInterpolation = do
symbol = L.symbol sc

literalValueFragment :: String -> Parser VarFragment
literalValueFragment charsToEscape = VarLiteral <$> some (escapedChar <|> normalChar)
literalValueFragment charsToEscape = VarLiteral <$> some (newlineChar <|> escapedChar <|> normalChar)
where
newlineChar = string "\\n" >> return '\n'
Copy link
Collaborator

Choose a reason for hiding this comment

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

@FranzGB I see normalChar parses any char that doesn't need escape. Do you know why that doesn't include the \n?

Copy link
Contributor Author

@FranzGB FranzGB Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hi Cris! Yes, basically this line escapedChar = (char '\\' *> anySingle) <?> "escaped character" is eating all the \ not treating \n as a single character and leaving just the n as that is a candiate for normalChar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do have another idea on how could we achieve this without adding a new function here (newlineChar <|> escapedChar <|> normalChar) but it will have to require modifying escapedChar function. Maybe we can catch up on this WDYT?

escapedChar = (char '\\' *> anySingle) <?> "escaped character"
normalChar = noneOf charsToEscape <?> "unescaped character"

Expand Down
Loading