Skip to content

Conversation

mertcanaltin
Copy link
Member

Dotenv::ParseContent includes some improvements to avoid crash problems when processing a multiline value
#52248

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Mar 29, 2024
Mert Can Altin added 2 commits March 29, 2024 22:43
assert.strictEqual(process.env.MULTI_SINGLE_QUOTED, 'THIS\nIS\nA\nMULTILINE\nSTRING');
assert.strictEqual(process.env.MULTI_BACKTICKED, 'THIS\nIS\nA\n"MULTILINE\'S"\nSTRING');
assert.strictEqual(process.env.MULTI_NOT_VALID_QUOTE, '"');
assert.strictEqual(process.env.MULTI_NOT_VALID_QUOTE, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

does this cover all the added code?

Copy link
Contributor

Choose a reason for hiding this comment

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

The MULTI_NOT_VALID_QUOTE test makes sure that if there's a line starting with a double quote that doesn't close, we still count it as one single value.

MULTI_NOT_VALID_QUOTE="

IMO we should keep this test to make sure we handle these situations right

Copy link
Contributor

Choose a reason for hiding this comment

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

Dotenv interprets this as a single quote " too not an empty value

Copy link
Member Author

Choose a reason for hiding this comment

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

yes this preserves the previous code

Copy link
Member Author

Choose a reason for hiding this comment

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

I must have done this wrong I will update it today

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

Can you add a failing test?

@mertcanaltin mertcanaltin requested a review from anonrig March 30, 2024 07:26
@mertcanaltin
Copy link
Member Author

Can you add a failing test?

I wonder if it's normal to get fail afterwards 🤔

@marco-ippolito
Copy link
Member

Can you add a failing test?

I wonder if it's normal to get fail afterwards 🤔

you can make sure it throws assert.throws

@mertcanaltin
Copy link
Member Author

I wanted to switch to the code in the main branch and test here, but I got such a test error

node git:(dev-52248)  ./node test/parallel/test-dotenv.js --NODE_SKIP_FLAG_CHECK
node:assert:126
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ undefined
- 'basic'
    at Object.<anonymous> (/Users/mert/Desktop/openSource/node/test/parallel/test-dotenv.js:8:8)
    at Module._compile (node:internal/modules/cjs/loader:1421:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1499:10)
    at Module.load (node:internal/modules/cjs/loader:1232:32)
    at Module._load (node:internal/modules/cjs/loader:1048:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:187:14)
    at node:internal/main/run_main_module:28:49 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: undefined,
  expected: 'basic',
  operator: 'strictEqual'
}

Node.js v22.0.0-pre

@mertcanaltin mertcanaltin added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 6, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 6, 2024
@nodejs-github-bot
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants