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

fs: (+/-)Infinity and NaN invalid unixtimestamp #11919

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
10 participants
@lucamaraschi
Contributor

lucamaraschi commented Mar 18, 2017

Infinity and NaN are currently considered valid input when generating a
unix time stamp but are defaulted arbitrarly to Date.now()/1000. This
PR removes this behaviour and throw an exception like all the other
invalid input types.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows [commit guidelines][]
Affected core subsystem(s)

fs

Show outdated Hide outdated doc/api/fs.md
- If the value is `NaN` or `Infinity`, the value will get converted to
`Date.now() / 1000`.
- If the value is `NaN` or `Infinity` (either positive or negative), an Error is
going to be thrown.

This comment has been minimized.

@mscdex

mscdex Mar 18, 2017

Contributor

Minor nit: I think 'an Error will be thrown' sounds better

@mscdex

mscdex Mar 18, 2017

Contributor

Minor nit: I think 'an Error will be thrown' sounds better

This comment has been minimized.

@cjihrig

cjihrig Mar 18, 2017

Contributor

Agreed. Might be better to list NaN, Infinity, or -Infinity too.

@cjihrig

cjihrig Mar 18, 2017

Contributor

Agreed. Might be better to list NaN, Infinity, or -Infinity too.

This comment has been minimized.

@lucamaraschi

lucamaraschi Mar 18, 2017

Contributor

Thanks, gonna change it

@lucamaraschi

lucamaraschi Mar 18, 2017

Contributor

Thanks, gonna change it

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@ChALkeR

ChALkeR Mar 18, 2017

Member

Not sure about this. /cc @jasnell, do we want to put this through a deprecation cycle?
Note that this changes both documented behavior and existing tests expectations.

@lucamaraschi to me it is not clear why this change should be done — that's not mentioned in this PR.

Member

ChALkeR commented Mar 18, 2017

Not sure about this. /cc @jasnell, do we want to put this through a deprecation cycle?
Note that this changes both documented behavior and existing tests expectations.

@lucamaraschi to me it is not clear why this change should be done — that's not mentioned in this PR.

Show outdated Hide outdated lib/fs.js
@@ -1164,8 +1164,8 @@ function toUnixTimestamp(time) {
if (typeof time === 'string' && +time == time) {
return +time;
}
if (typeof time === 'number') {
if (!Number.isFinite(time) || time < 0) {
if (typeof time === 'number' && !isNaN(time) && isFinite(time)) {

This comment has been minimized.

@cjihrig

cjihrig Mar 18, 2017

Contributor

isFinite() covers NaN, so I think the !isNaN(time) can be dropped.

@cjihrig

cjihrig Mar 18, 2017

Contributor

isFinite() covers NaN, so I think the !isNaN(time) can be dropped.

Show outdated Hide outdated test/parallel/test-fs-timestamp-parsing-error.js
'use strict';
const fs = require('fs');
const assert = require('assert');
require('../common');

This comment has been minimized.

@cjihrig

cjihrig Mar 18, 2017

Contributor

Can you move this up before the other require()s.

@cjihrig

cjihrig Mar 18, 2017

Contributor

Can you move this up before the other require()s.

Show outdated Hide outdated doc/api/fs.md
- If the value is `NaN` or `Infinity`, the value will get converted to
`Date.now() / 1000`.
- If the value is `NaN` or `Infinity` (either positive or negative), an Error is
going to be thrown.

This comment has been minimized.

@cjihrig

cjihrig Mar 18, 2017

Contributor

Agreed. Might be better to list NaN, Infinity, or -Infinity too.

@cjihrig

cjihrig Mar 18, 2017

Contributor

Agreed. Might be better to list NaN, Infinity, or -Infinity too.

Show outdated Hide outdated lib/fs.js
@@ -1164,8 +1164,8 @@ function toUnixTimestamp(time) {
if (typeof time === 'string' && +time == time) {
return +time;
}
if (typeof time === 'number') {
if (!Number.isFinite(time) || time < 0) {
if (typeof time === 'number' && !isNaN(time) && isFinite(time)) {

This comment has been minimized.

@mscdex

mscdex Mar 18, 2017

Contributor

I think this can be simplified to just typeof time === 'number' && Number.isFinite(time)

@mscdex

mscdex Mar 18, 2017

Contributor

I think this can be simplified to just typeof time === 'number' && Number.isFinite(time)

This comment has been minimized.

@not-an-aardvark

not-an-aardvark Mar 18, 2017

Member

I think Number.isFinite(time) would cover typeof time === 'number' anyway, so this is equivalent to just Number.isFinite(time).

@not-an-aardvark

not-an-aardvark Mar 18, 2017

Member

I think Number.isFinite(time) would cover typeof time === 'number' anyway, so this is equivalent to just Number.isFinite(time).

Show outdated Hide outdated test/parallel/test-fs-timestamp-parsing-error.js
});
const okInputs = [1, -1, '1', '-1', Date.now()];
okInputs.forEach((input) => assert.doesNotThrow(() =>

This comment has been minimized.

@mscdex

mscdex Mar 18, 2017

Contributor

I think using braces and better indentation for the forEach() callback will make this look better (especially since assert methods do not return meaningful values). Example:

okInputs.forEach((input) => {
  assert.doesNotThrow(() => fs._toUnixTimestamp(input));
});
@mscdex

mscdex Mar 18, 2017

Contributor

I think using braces and better indentation for the forEach() callback will make this look better (especially since assert methods do not return meaningful values). Example:

okInputs.forEach((input) => {
  assert.doesNotThrow(() => fs._toUnixTimestamp(input));
});

This comment has been minimized.

@lucamaraschi

lucamaraschi Mar 18, 2017

Contributor

Agreed @mscdex, I was trying to keep it as compact as possible tho.

@lucamaraschi

lucamaraschi Mar 18, 2017

Contributor

Agreed @mscdex, I was trying to keep it as compact as possible tho.

This comment has been minimized.

@mscdex

mscdex Mar 18, 2017

Contributor

IMHO compactness isn't as important in tests.

@mscdex

mscdex Mar 18, 2017

Contributor

IMHO compactness isn't as important in tests.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Mar 19, 2017

Member

I think this makes sense, silently converting these values to the current time is not something I’d expect as a user (at least not for ±∞).

do we want to put this through a deprecation cycle?

Hm, is there any reason not to?

Member

addaleax commented Mar 19, 2017

I think this makes sense, silently converting these values to the current time is not something I’d expect as a user (at least not for ±∞).

do we want to put this through a deprecation cycle?

Hm, is there any reason not to?

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 19, 2017

Member

I'm not convinced this behavior was intentional. It could be viewed as a bug fix and it is in a _-prefixed method. Unless we can find direct uses that would be problematic, I don't think a deprecation cycle should be necessary.

Member

jasnell commented Mar 19, 2017

I'm not convinced this behavior was intentional. It could be viewed as a bug fix and it is in a _-prefixed method. Unless we can find direct uses that would be problematic, I don't think a deprecation cycle should be necessary.

Show outdated Hide outdated test/parallel/test-fs-timestamp-parsing-error.js
const fs = require('fs');
const assert = require('assert');
[Number.POSITIVE_INFINITY, Number.NEGATIVE_INFINITY, NaN].forEach((input) => {

This comment has been minimized.

@TimothyGu

TimothyGu Mar 19, 2017

Member

nit: any reasons the shorter form -Infinity and Infinity are not used?

@TimothyGu

TimothyGu Mar 19, 2017

Member

nit: any reasons the shorter form -Infinity and Infinity are not used?

@cjihrig

LGTM once @not-an-aardvark's nit is addressed.

@mcollina

LGTM. I don't think it was intentional in the first place.

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Mar 20, 2017

Contributor

@lucamaraschi this needs a rebase. Could you squash the commits while you're at it.

Contributor

cjihrig commented Mar 20, 2017

@lucamaraschi this needs a rebase. Could you squash the commits while you're at it.

fs: (+/-)Infinity and NaN invalid unixtimestamp
Infinity and NaN are currently considered valid input when generating a
unix time stamp but are defaulted arbitrarly to Date.now()/1000. This
PR removes this behaviour and throw an exception like all the other
invalid input types.
@cjihrig

This comment has been minimized.

Show comment
Hide comment

jasnell added a commit that referenced this pull request Mar 22, 2017

fs: (+/-)Infinity and NaN invalid unixtimestamp
Infinity and NaN are currently considered valid input when generating a
unix time stamp but are defaulted arbitrarly to Date.now()/1000. This
PR removes this behaviour and throw an exception like all the other
invalid input types.

PR-URL: #11919
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 22, 2017

Member

Landed in eed87b1

Member

jasnell commented Mar 22, 2017

Landed in eed87b1

@jasnell jasnell closed this Mar 22, 2017

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment