src: don't abort when package.json is a directory #18270

bnoordhuis · 2018-01-20T14:17:01Z

No overhead alternative to #18261.

CI: https://ci.nodejs.org/job/node-test-commit/15553/

bmeck · 2018-01-20T14:37:15Z

src/node_file.cc

@@ -474,14 +481,12 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo<Value>& args) {
    numchars = uv_fs_read(loop, &read_req, fd, &buf, 1, offset, nullptr);
    uv_fs_req_cleanup(&read_req);

-    CHECK_GE(numchars, 0);
+    if (numchars < 0)
+      return;


please return the empty string so that it gets put into packageMainCache

That doesn't sound right. '' is for a package.json without a main property, see the bottom of this function.

I'm look at

node/lib/module.js

Line 133 in e1c29f2

return packageMainCache[requestPath] = undefined;

which eagerly returns with a falsey value and sets an entry.

node/lib/module.js

Line 148 in e1c29f2

if (!pkg) return false;

then bails out. this also looks like it might have a bug re-reading it.

Yes, but that's for when we found (and read) a package.json. That's not the case here, it's closer to an ENOENT or EPERM condition.

edit: to be clear, that's the if (json === undefined) return false a few lines up.

I disagree, we found a package.json it was a directory though.

Exactly, not a file we can parse.

The intent of this change is around package.json/ directories. This mimics a package.json that was found but has no relevant "main" entry : https://github.com/bnoordhuis/io.js/blob/ec93af3bd79c89088f832f72d788225abb106910/src/node_file.cc#L496-L497 . We can discern the "main" we want to represent for it without the need to parse, and returning "" causes the packageMainCache to correctly store the falsey value to bail the module system.

I'm kind of done arguing but I'll address your last point: by your logic errors such as EPERM should also be interpreted as meaning "package.json without a main entry" - but we don't do that and rightly so.

EPERM is different, you can EPERM a directory. We can inspect this entity on the file system.

bmeck · 2018-01-20T14:47:53Z

merging to my PR. would like to see more cooperation towards mentorship in future.

bnoordhuis · 2018-01-27T12:29:06Z

Heads up, I plan on landing this later today (without @bmeck's suggestion) so I can move forward with work that depends on it.

CI: https://ci.nodejs.org/job/node-test-commit/15733/

bmeck · 2018-01-27T13:24:41Z

@bnoordhuis I disagree on landing it, but would like to know if your block required the difference in our opinions.

bmeck

explanation of landing difference

addaleax

LGTM as this is.

Given that I’d expect directories called package.json to almost never show up in the real world, I’m okay with tending towards semantical correctness and not entering the entry into the cache.

(I’d also be okay with the other approach.)

bmeck · 2018-01-27T13:32:51Z

@addaleax falsey values put into the cache don't even work today, hence my need of rereview of the "parent" PR. I still think semantics are closer to a file with empty content than a permissions error since we can inspect the entity on disk.

addaleax · 2018-01-27T13:36:13Z

@bmeck I disagree with your last statement.

Also, is there any practical difference?

bmeck · 2018-01-27T13:39:53Z

@addaleax

I disagree with your last statement.

Then we are at an impasse. I feel rather strongly one way, and others feel another way.

Also, is there any practical difference?

If it gets put into the cache or not, so C++ calls back into the function / stats. Likelyhood of this situation occurring is low but seems relevant still. Of note we also return "" for packages without main fields at all rather than treat it as an error.

bnoordhuis · 2018-01-27T13:41:47Z

would like to know if your block required the difference in our opinions

I don't understand the question. What block?

bmeck · 2018-01-27T13:42:10Z

@bnoordhuis

so I can move forward with work that depends on it.

bnoordhuis · 2018-01-27T13:44:07Z

Still not sure I'm following... the work I'm referring to is unrelated to the module system - I want to factor out file reading in C++.

bmeck · 2018-01-27T13:44:53Z

So it can move forward without this PR?

bnoordhuis · 2018-01-27T13:45:24Z

No, because I want to repurpose code that this PR touches.

bmeck · 2018-01-27T13:53:21Z

@bnoordhuis

If it is not dependent on the module system, but needs some code this PR touches, Would your refactor be any different, due to making the InternalModuleReadJSON return "" as I requested above?

As an alternative I'd also be fine removing "" from being returned on missing "main" in package.json, since it would never be hit by the cache check anyway without a fix.

The consistency between the two is my real issue with not returning "" so as long as it is one way or another.

bmeck · 2018-01-27T14:53:54Z

I guess both PRs can go to tsc-review if we are stuck as well.

guybedford

This seems good to me, the function is returning the package.json contents, so an empty return corresponding to no package.json file seems sensible in terms of the function's operation.

That said, the lack of a package.json can be cached just like the package.json itself, which doesn't currently seem to be the case. Having this scenario register into the cache may be a worthwhile followup PR especially with the ESM loader package.json checks which we should be getting to share this cache as well.

bmeck · 2018-01-27T16:45:10Z

Cache of all package.json situations seems fine to me, but my concern is we don't have a clear definition with what it means to be an error. We have:

some error behavior that doesn't get cached for things like ENOENT and EPERM
some defaulting behavior with the missing "main" string in the file contents in which we attempt to cache as a permanent "error"?
- it doesn't work since falsey values are not hits in the cache
- this behavior gets weird, things like {"name": "main"} avoid this check.
we don't cache JSON.parse errors, and we even throw on them
we don't actually check if the JSON itself has a "main" property using getOwnPropertyDescriptor or anything, but we do cache the result.
- you can have odd situations here as well with things like 0 as the package.json contents.

I'm trying to figure out how to make this more internally consistent:

1 doesn't cache errors, but doesn't treat it as having a value.
2 caches the error with a default value.
3 doesn't cache errors, but does throw.
4 just seems like a bug

I would like to choose one of 1, 2, 3, or caching the error and throwing as a consistent behavior with a personal preference on always caching the results. I think this preference is from working with ESM to a small extent that by spec caches errors permanently when importing other modules.

bnoordhuis · 2018-01-29T13:41:02Z

@bmeck Can you retract your red X? It's fine if you want to discuss semantics but please open a new issue. It shouldn't hold up a bug fix.

bmeck · 2018-01-29T14:03:24Z

@bnoordhuis we have a disagreement, so no. Per stopping fixes, you also keep a red X on my PR. Your choice to open a new PR and not contribute to the original makes this rather hard as well. Maybe keeping things in one place would have been better, but now we have 2 PRs that neither of us likes apparently. One that does acknowledge both contributors involved and solved the review comments. You can remove your X on mine or take one of the 2 suggestions above we can land either PR.

In the future, I highly recommend a different approach to superceding PRs be looked into as your behavior of open a PR that only you commit to ontop of mine has happened a couple times and this scenario seems to be common that you give me a red X and tell me that I am blocking progress or have even insulted me in the past.

bnoordhuis · 2018-01-29T15:27:15Z

Well, okay then. @nodejs/tsc Please weigh in.

targos · 2018-01-29T15:44:52Z

I think we should treat this the same as ENOENT here and make all of this more consistent in a subsequent PR.

src: don't abort when package.json is a directory

ec93af3

Fixes: nodejs#8307

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. labels Jan 20, 2018

bnoordhuis mentioned this pull request Jan 20, 2018

module: ignore package.json/ directories #18261

Closed

3 tasks

richardlau approved these changes Jan 20, 2018

View reviewed changes

bmeck reviewed Jan 20, 2018

View reviewed changes

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

bmeck suggested changes Jan 27, 2018

View reviewed changes

addaleax approved these changes Jan 27, 2018

View reviewed changes

bmeck requested a review from guybedford January 27, 2018 13:34

guybedford approved these changes Jan 27, 2018

View reviewed changes

ryan-ally mentioned this pull request Apr 7, 2024

[Snyk] Security upgrade eslint from 6.8.0 to 9.0.0 ryan-ally/node#105

Open

This was referenced Apr 7, 2024

[Snyk] Security upgrade eslint from 7.32.0 to 9.0.0 aliscco/alisco-node#1361

Open

[Snyk] Security upgrade eslint from 5.16.0 to 9.0.0 aliscco/alisco-node#1362

Open

[Snyk] Security upgrade eslint from 5.16.0 to 9.0.0 aliscco/alisco-node#1363

Open

leonardoadame mentioned this pull request Apr 7, 2024

[Snyk] Security upgrade eslint from 4.19.1 to 9.0.0 leonardoadame/node#759

Open

This was referenced Apr 7, 2024

[Snyk] Security upgrade eslint from 4.19.1 to 9.0.0 aliscco/alisco-node#1364

Open

[Snyk] Security upgrade eslint from 4.19.1 to 9.0.0 aliscco/alisco-node#1365

Open

[Snyk] Security upgrade eslint from 4.19.1 to 9.0.0 aliscco/alisco-node#1366

Open

baby636 mentioned this pull request Apr 7, 2024

[Snyk] Security upgrade eslint from 7.32.0 to 9.0.0 baby636/node#145

Open

ryan-ally mentioned this pull request Apr 7, 2024

[Snyk] Security upgrade eslint from 6.8.0 to 9.0.0 ryan-ally/node#106

Open

aliscco mentioned this pull request Apr 7, 2024

[Snyk] Security upgrade eslint from 3.19.0 to 9.0.0 aliscco/alisco-node#1372

Open

leonardoadame mentioned this pull request Apr 7, 2024

[Snyk] Security upgrade eslint from 3.19.0 to 9.0.0 leonardoadame/node#770

Open

This was referenced Apr 8, 2024

[Snyk] Security upgrade eslint from 4.19.1 to 9.0.0 aliscco/alisco-node#1375

Open

[Snyk] Security upgrade eslint from 7.32.0 to 9.0.0 aliscco/alisco-node#1376

Open

[Snyk] Security upgrade eslint from 5.16.0 to 9.0.0 aliscco/alisco-node#1377

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: don't abort when package.json is a directory #18270

src: don't abort when package.json is a directory #18270

bnoordhuis commented Jan 20, 2018 •

edited

Loading

bmeck Jan 20, 2018

bnoordhuis Jan 20, 2018

bmeck Jan 20, 2018

bnoordhuis Jan 20, 2018 •

edited

Loading

bmeck Jan 20, 2018

bnoordhuis Jan 20, 2018

bmeck Jan 20, 2018

bnoordhuis Jan 20, 2018

bmeck Jan 20, 2018 •

edited

Loading

bmeck commented Jan 20, 2018

bnoordhuis commented Jan 27, 2018

bmeck commented Jan 27, 2018

bmeck left a comment

addaleax left a comment

bmeck commented Jan 27, 2018

addaleax commented Jan 27, 2018

bmeck commented Jan 27, 2018 •

edited

Loading

bnoordhuis commented Jan 27, 2018

bmeck commented Jan 27, 2018

bnoordhuis commented Jan 27, 2018

bmeck commented Jan 27, 2018

bnoordhuis commented Jan 27, 2018

bmeck commented Jan 27, 2018 •

edited

Loading

bmeck commented Jan 27, 2018

guybedford left a comment

bmeck commented Jan 27, 2018 •

edited

Loading

bnoordhuis commented Jan 29, 2018

bmeck commented Jan 29, 2018

bnoordhuis commented Jan 29, 2018

targos commented Jan 29, 2018

src: don't abort when package.json is a directory #18270

src: don't abort when package.json is a directory #18270

Conversation

bnoordhuis commented Jan 20, 2018 • edited Loading

bmeck Jan 20, 2018

Choose a reason for hiding this comment

bnoordhuis Jan 20, 2018

Choose a reason for hiding this comment

bmeck Jan 20, 2018

Choose a reason for hiding this comment

bnoordhuis Jan 20, 2018 • edited Loading

Choose a reason for hiding this comment

bmeck Jan 20, 2018

Choose a reason for hiding this comment

bnoordhuis Jan 20, 2018

Choose a reason for hiding this comment

bmeck Jan 20, 2018

Choose a reason for hiding this comment

bnoordhuis Jan 20, 2018

Choose a reason for hiding this comment

bmeck Jan 20, 2018 • edited Loading

Choose a reason for hiding this comment

bmeck commented Jan 20, 2018

bnoordhuis commented Jan 27, 2018

bmeck commented Jan 27, 2018

bmeck left a comment

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

bmeck commented Jan 27, 2018

addaleax commented Jan 27, 2018

bmeck commented Jan 27, 2018 • edited Loading

bnoordhuis commented Jan 27, 2018

bmeck commented Jan 27, 2018

bnoordhuis commented Jan 27, 2018

bmeck commented Jan 27, 2018

bnoordhuis commented Jan 27, 2018

bmeck commented Jan 27, 2018 • edited Loading

bmeck commented Jan 27, 2018

guybedford left a comment

Choose a reason for hiding this comment

bmeck commented Jan 27, 2018 • edited Loading

bnoordhuis commented Jan 29, 2018

bmeck commented Jan 29, 2018

bnoordhuis commented Jan 29, 2018

targos commented Jan 29, 2018

bnoordhuis commented Jan 20, 2018 •

edited

Loading

bnoordhuis Jan 20, 2018 •

edited

Loading

bmeck Jan 20, 2018 •

edited

Loading

bmeck commented Jan 27, 2018 •

edited

Loading

bmeck commented Jan 27, 2018 •

edited

Loading

bmeck commented Jan 27, 2018 •

edited

Loading