Skip to content

Commit

Permalink
fix(core): 1 hour renders as 60 minutes (#15125)
Browse files Browse the repository at this point in the history
When asked to convert full hours to human strings, due to floating point errors, they get rendered as minutes. 1 hour gets converted to 60 minutes. 2 hours converts to 1 hour 60 minutes.

This request fixes that by first multiplying `amount` with the `fromUnit.inMillis` before dividing by `toUnit.inMillis`. This ensures that if the amount is big enough, it gets divided properly.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
Dolvic authored Jun 22, 2021
1 parent e5c0d59 commit adcd8c3
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 8 deletions.
10 changes: 5 additions & 5 deletions packages/@aws-cdk/aws-cognito/test/user-pool-client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -670,7 +670,7 @@ describe('User Pool Client', () => {
accessTokenValidity: validity,
refreshTokenValidity: Duration.hours(1),
});
}).toThrow(`accessTokenValidity: Must be a duration between 5 minutes and 60 minutes (inclusive); received ${validity.toHumanString()}.`);
}).toThrow(`accessTokenValidity: Must be a duration between 5 minutes and 1 hour (inclusive); received ${validity.toHumanString()}.`);
});

test.each([
Expand All @@ -686,23 +686,23 @@ describe('User Pool Client', () => {
idTokenValidity: validity,
refreshTokenValidity: Duration.hours(1),
});
}).toThrow(`idTokenValidity: Must be a duration between 5 minutes and 60 minutes (inclusive); received ${validity.toHumanString()}.`);
}).toThrow(`idTokenValidity: Must be a duration between 5 minutes and 1 hour (inclusive); received ${validity.toHumanString()}.`);
});

test.each([
Duration.minutes(0),
Duration.minutes(59),
Duration.days(10 * 365).plus(Duration.minutes(1)),
Duration.days(10 * 365 + 1),
])('validates refreshTokenValidity is a duration between 60 minutes and 10 years', (validity) => {
])('validates refreshTokenValidity is a duration between 1 hour and 10 years', (validity) => {
const stack = new Stack();
const pool = new UserPool(stack, 'Pool');
expect(() => {
pool.addClient('Client1', {
userPoolClientName: 'Client1',
refreshTokenValidity: validity,
});
}).toThrow(`refreshTokenValidity: Must be a duration between 60 minutes and 3650 days (inclusive); received ${validity.toHumanString()}.`);
}).toThrow(`refreshTokenValidity: Must be a duration between 1 hour and 3650 days (inclusive); received ${validity.toHumanString()}.`);
});

test.each([
Expand Down Expand Up @@ -758,7 +758,7 @@ describe('User Pool Client', () => {
Duration.minutes(120),
Duration.days(365),
Duration.days(10 * 365),
])('validates refreshTokenValidity is a duration between 60 minutes and 10 years (valid)', (validity) => {
])('validates refreshTokenValidity is a duration between 1 hour and 10 years (valid)', (validity) => {
const stack = new Stack();
const pool = new UserPool(stack, 'Pool');

Expand Down
3 changes: 1 addition & 2 deletions packages/@aws-cdk/core/lib/duration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -309,12 +309,11 @@ class TimeUnit {

function convert(amount: number, fromUnit: TimeUnit, toUnit: TimeUnit, { integral = true }: TimeConversionOptions) {
if (fromUnit.inMillis === toUnit.inMillis) { return amount; }
const multiplier = fromUnit.inMillis / toUnit.inMillis;

if (Token.isUnresolved(amount)) {
throw new Error(`Unable to perform time unit conversion on un-resolved token ${amount}.`);
}
const value = amount * multiplier;
const value = (amount * fromUnit.inMillis) / toUnit.inMillis;
if (!Number.isInteger(value) && integral) {
throw new Error(`'${amount} ${fromUnit}' cannot be converted into a whole number of ${toUnit}.`);
}
Expand Down
7 changes: 6 additions & 1 deletion packages/@aws-cdk/core/test/duration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,13 @@ nodeunitShim({
test.equal(Duration.minutes(0).toHumanString(), '0 minutes');
test.equal(Duration.minutes(Lazy.number({ produce: () => 5 })).toHumanString(), '<token> minutes');

test.equal(Duration.minutes(10).toHumanString(), '10 minutes');
test.equal(Duration.days(1).toHumanString(), '1 day');
test.equal(Duration.hours(1).toHumanString(), '1 hour');
test.equal(Duration.minutes(1).toHumanString(), '1 minute');
test.equal(Duration.seconds(1).toHumanString(), '1 second');
test.equal(Duration.millis(1).toHumanString(), '1 milli');

test.equal(Duration.minutes(10).toHumanString(), '10 minutes');

test.equal(Duration.minutes(62).toHumanString(), '1 hour 2 minutes');

Expand Down

0 comments on commit adcd8c3

Please sign in to comment.