Skip to content

Commit

Permalink
Fix floating point arithmetic bug in range value validation (#1687)
Browse files Browse the repository at this point in the history
* Fix floating point arithmetic bug in range value validation

* changelog

* PR feedback
  • Loading branch information
chandlerprall authored Mar 6, 2019
1 parent 87f565a commit cfb637e
Show file tree
Hide file tree
Showing 8 changed files with 79 additions and 4 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@
- Added `.eui-fullWidth` utility class ([#1665](https://github.com/elastic/eui/pull/1665))
- Added `EuiPopoverFooter` and converted `EuiPopoverTitle` to TS ([#1666](https://github.com/elastic/eui/pull/1666))

**Bug fixes**

- Fixed floating point arithmetic bug in `EuiRangeTrack`'s value validation ([#1687](https://github.com/elastic/eui/pull/1687))

## [`9.0.1`](https://github.com/elastic/eui/tree/v9.0.1)

**Bug fixes**
Expand Down
2 changes: 1 addition & 1 deletion src-docs/src/i18ntokens.json
Original file line number Diff line number Diff line change
Expand Up @@ -735,4 +735,4 @@
},
"filepath": "src/components/toast/toast.js"
}
]
]
1 change: 1 addition & 0 deletions src-docs/src/views/range/range.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export default class extends Component {
id={makeId()}
min={100}
max={200}
step={0.05}
value={this.state.value}
onChange={this.onChange}
showLabels
Expand Down
1 change: 1 addition & 0 deletions src-docs/src/views/range/range_example.js
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ export const RangeControlExample = {
snippet: `<EuiRange
min={100}
max={200}
step={0.05}
value={this.state.value}
onChange={this.onChange}
showLabels
Expand Down
3 changes: 2 additions & 1 deletion src/components/form/range/range_track.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import classNames from 'classnames';

import range from 'lodash/range';

import { isEvenlyDivisibleBy } from '../../../services';
import { EuiRangeLevels, LEVEL_COLORS } from './range_levels';
import { EuiRangeTicks } from './range_ticks';

Expand All @@ -18,7 +19,7 @@ export class EuiRangeTrack extends Component {
throw new Error(`The value of ${value} is higher than the max value of ${this.props.max}.`);
}
// Error out if the value doesn't line up with the sequence of steps
if ((value - this.props.min) % this.props.step > 0) {
if (!isEvenlyDivisibleBy(value - this.props.min, this.props.step !== undefined ? this.props.step : 1)) {
throw new Error(`The value of ${value} is not included in the possible sequence provided by the step of ${this.props.step}.`);
}
// Return the value if nothing fails
Expand Down
2 changes: 1 addition & 1 deletion src/services/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export {
formatText,
} from './format';

export { isWithinRange } from './number';
export { isEvenlyDivisibleBy, isWithinRange } from './number';

export { Pager } from './paging';

Expand Down
51 changes: 50 additions & 1 deletion src/services/number/number.test.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { isWithinRange } from './number';
import { isEvenlyDivisibleBy, isWithinRange } from './number';

describe('numbers', () => {
test('isWithinRange', () => {
Expand All @@ -18,4 +18,53 @@ describe('numbers', () => {
expect(isWithinRange(0, 100, -10)).toBe(false);
expect(isWithinRange(0, 100, '')).toBe(false);
});

describe('isEvenlyDivisibleBy', () => {
it('correctly computes for integers', () => {
expect(isEvenlyDivisibleBy(10, 1)).toBe(true);
expect(isEvenlyDivisibleBy(10, 2)).toBe(true);
expect(isEvenlyDivisibleBy(10, 10)).toBe(true);

expect(isEvenlyDivisibleBy(10, 3)).toBe(false);
expect(isEvenlyDivisibleBy(1, 3)).toBe(false);
});

it('correctly computes for decimals', () => {
expect(isEvenlyDivisibleBy(1, 0.1)).toBe(true);
expect(isEvenlyDivisibleBy(1, 0.2)).toBe(true);
expect(isEvenlyDivisibleBy(1, 0.25)).toBe(true);
expect(isEvenlyDivisibleBy(1, 0.5)).toBe(true);
expect(isEvenlyDivisibleBy(1, 0.3)).toBe(false);
expect(isEvenlyDivisibleBy(1, 0.51)).toBe(false);
expect(isEvenlyDivisibleBy(1, 0.9)).toBe(false);
expect(isEvenlyDivisibleBy(1000000, 0.00001)).toBe(true);
expect(isEvenlyDivisibleBy(1000000, 0.00002)).toBe(true);
expect(isEvenlyDivisibleBy(1000000, 0.00005)).toBe(true);
expect(isEvenlyDivisibleBy(15000000, 0.000075)).toBe(true);

expect(isEvenlyDivisibleBy(3, 0.5)).toBe(true);
expect(isEvenlyDivisibleBy(3, 1.5)).toBe(true);
expect(isEvenlyDivisibleBy(3, 0.61)).toBe(false);
expect(isEvenlyDivisibleBy(3, 2.9)).toBe(false);

expect(isEvenlyDivisibleBy(2.75, 0.25)).toBe(true);
expect(isEvenlyDivisibleBy(2.75, 0.55)).toBe(true);
expect(isEvenlyDivisibleBy(2.75, 1.375)).toBe(true);
expect(isEvenlyDivisibleBy(2.75, 0.1)).toBe(false);
expect(isEvenlyDivisibleBy(2.75, 0.5)).toBe(false);
expect(isEvenlyDivisibleBy(2.75, 1.374)).toBe(false);
expect(isEvenlyDivisibleBy(2.75, 1.376)).toBe(false);
});

it('returns false when factor is 0', () => {
expect(isEvenlyDivisibleBy(1, 0)).toBe(false);
expect(isEvenlyDivisibleBy(100, 0)).toBe(false);
expect(isEvenlyDivisibleBy(-100, 0)).toBe(false);
expect(isEvenlyDivisibleBy(1.5, 0)).toBe(false);
});

it('handles known floating point error cases', () => {
expect(isEvenlyDivisibleBy(1, 0.05)).toBe(true);
});
});
});
19 changes: 19 additions & 0 deletions src/services/number/number.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,22 @@ export const isWithinRange = (
const val = Number(value);
return Number(min) <= val && val <= Number(max);
};

// 1e-6 covers up to 10,000,000,000 factored by a decimal
const EPSILON = 1e-6;
export function isEvenlyDivisibleBy(num: number, factor: number) {
const remainder = num % factor;

// due to floating point issues the remainder needs to be within a margin instead of exactly 0
// 1 % 0.1 === 0.09999999999999995
// 1000000000 % 0.1 === 0.09999994448884877
// 1 % 0.05 === 0.04999999999999995

// Compare the smaller of (remainder, factor - remainder) to EPSILON
return (
Math.min(
remainder, // remainder may be smallest, it is 0 in the well-formed case
Math.abs(factor - remainder) // otherwise the positive difference between factor and remainder
) < EPSILON
);
}

0 comments on commit cfb637e

Please sign in to comment.