Skip to content
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

Tokens: be able to represent as numbers #1455

Closed
eladb opened this issue Dec 30, 2018 · 12 comments · Fixed by #2534 or MechanicalRock/tech-radar#14 · May be fixed by MechanicalRock/cdk-constructs#5, MechanicalRock/cdk-constructs#6 or MechanicalRock/cdk-constructs#7
Labels
feature-request A feature should be added or improved.

Comments

@eladb
Copy link
Contributor

eladb commented Dec 30, 2018

We are able to represent tokens as strings (token.toString()) and as string arrays (token.toList()), but in certain cases, there's a need to represent them as numbers. Introduce token.toNumber().

We need to somehow encode the fact that this is a token into a number. It's not going to be elegant (e.g. -66737346654.<random> can be the key to the token map).

Related to #744
Related to #1453

@rix0rrr
Copy link
Contributor

rix0rrr commented Dec 31, 2018

Agreed. This requires VERY careful thought and testing at the jsii serialization border. For example, numbers are PROBABLY represented as doubles in jsii client languages, but now they HAVE to be or we'll lose bits that we use to encode a token number. Also, our stringification and parsing on both sides shouldn't lose any precision and be reversible in all languages.

My original (most future-proofed) idea was to store the token identifier in the unused bits of a NaN, but that definitely requires additional JSII implementation effort on both sides (to detect the case of special NaNs, extract and transport the hidden bits explicitly, on both sides), so just picking a decimal number that "just" happens to stringify and parse correctly may be a more expedient solution.

@rix0rrr
Copy link
Contributor

rix0rrr commented Dec 31, 2018

double memory layout

We could for example say that a sign of 1 and an exponent of 00000000001 means a Token and then we have 53 bits left to store a Token number in. That would mean we take a range of very very small negative numbers out of commission, which seems okay to lose.

@eladb eladb mentioned this issue Jan 3, 2019
6 tasks
@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 4, 2019

Use case: @mipearson for example wants to use a CloudFormation parameter as the input to minSize and maxSize of an AutoScalingGroup.

This would also require changing the source of ASG to check for "real" values and only do the validation then.

@mipearson
Copy link

This would also require changing the source of ASG to check for "real" values and only do the validation then.

So .. this isn't a real use case, so much as something I thought would work but didn't.

I was working out how to integrate CDK with existing tooling that can use external tools to compile CFN templates. What I ended up doing instead was using that tool's "compile time parameters" feature to pass the parameters in via JSON to javascript.

IMO properly supporting this use case would complicate most constructs - what I'd prefer is that the construct instead insist on "real" values and throw a descriptive error if it's passed references when it can't work with them.

@mipearson
Copy link

In my own code I'm going to be optimising for providers instead and writing my own - eg if minSize needs to be dynamic, write a provider to pull that value from the relevant source so that it's available at synth time and cached in the context.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 4, 2019

IMO properly supporting this use case would complicate most constructs

Sure, but I think we can add some utilities to make this easier to write for TypeScript construct authors. Something like:

cdk.validate(props.minSize, props.maxSize, (minSize, maxSize) => {
  // Callback only executed if neither minSize nor maxSize are unresolved
  if (!(minSize <= maxSize)) {
    throw new Error('...');
  }
});

Or even this:

cdk.validate('minSize should be <= maxSize', props.minSize, props.maxSize, (minSize, maxSize) =>minSize <= maxSize);
});

// Will throw something like:
// "minSize should be <= maxSize, got: 23, 5"

eladb pushed a commit that referenced this issue Jan 7, 2019
Now that we can represent attributes as native strings and string lists,
this covers the majority of resource attributes in CloudFormation.

This change:

* String attributes are represented as `string` (like before).
* String list attribute are now represented as `string[]`.
* Any other attribute types are represented as `cdk.Token`.

Attributes that are represented as Tokens as of this change:

* amazonmq has a bunch of `Integer` attributes (will be solved by #1455)
* iot1click has a bunch of `Boolean` attributes
* cloudformation has a JSON attribute
* That's all.

A few improvements to cfn2ts:

* Auto-detect cfn2ts scope from package.json so it is more self-contained
  and doesn't rely on cdk-build-tools to run.
* Added a "cfn2ts" npm script to all modules so it is now possible
  to regenerate all L1 via "lerna run cfn2ts".
* Removed the premature optimization for avoiding code regeneration
  (it saved about 0.5ms).

Fixes #1406
@eladb eladb added the feature-request A feature should be added or improved. label Jan 7, 2019
eladb pushed a commit that referenced this issue Jan 8, 2019
Now that we can represent attributes as native strings and string lists,
this covers the majority of resource attributes in CloudFormation.

This change:

* String attributes are represented as `string` (like before).
* String list attribute are now represented as `string[]`.
* Any other attribute types are represented as `cdk.Token`.

Attributes that are represented as Tokens as of this change:

* amazonmq has a bunch of `Integer` attributes (will be solved by #1455)
* iot1click has a bunch of `Boolean` attributes
* cloudformation has a JSON attribute
* That's all.

A few improvements to cfn2ts:

* Auto-detect cfn2ts scope from package.json so it is more self-contained
  and doesn't rely on cdk-build-tools to run.
* Added a "cfn2ts" npm script to all modules so it is now possible
  to regenerate all L1 via "lerna run cfn2ts".
* Removed the premature optimization for avoiding code regeneration
  (it saved about 0.5ms).

Fixes #1406

BREAKING CHANGE: any `CfnXxx` resource attributes that represented a list of strings are now typed as `string[]`s (via #1144). Attributes that represent strings, are still typed as `string` (#712) and all other attribute types are represented as `cdk.Token`.
@eladb
Copy link
Contributor Author

eladb commented Jan 8, 2019

This does bring up a more general concern around input validation which exists not only for numbers and I suspect we are violating already in a few places.

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 19, 2019

var buf = new ArrayBuffer(8);
var bytes = new Uint8Array(buf);

// IEEE double in LE
// [mmmmmmm][mmmmmmm][mmmmmmm][mmmmmmm][mmmmmmm][mmmmmmm][eeeeemm][Seeeeee]
// \------------------- mantissa 52 bits ------------------/\----expo---/^sign

bytes[7] = 0x80;
bytes[6] = 0x04;
bytes[5] = 0x01;

const f = (new Float64Array(buf))[0];
const fs = JSON.stringify(f);
console.log(fs);
g = JSON.parse(fs);

console.log(f == g);
-5.568116955492875e-309
true

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 19, 2019

Java:

    Double f = Double.parseDouble("-5.568116955492875e-309");    
    System.out.println(f);
-5.568116955492875E-309

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 19, 2019

Python

>>> json.dumps(json.loads("-5.568116955492875E-309"))
'-5.568116955492875e-309'

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 19, 2019

A finite binary number can never need an infinite decimal representation, says someone smarter than me on StackExchange:

https://math.stackexchange.com/questions/261988/number-with-finite-binary-representation-and-infinite-decimal-representation

@rix0rrr
Copy link
Contributor

rix0rrr commented Apr 24, 2019

By the way, I just realized something. We should make the number VERY LARGE as opposed to VERY SMALL, so that if users manipulate it by accident it'll reduce the opportunity for it to break the token:

tiny_token + 1 === 1
huge_token + 1 === huge_token

rix0rrr added a commit that referenced this issue May 13, 2019
Numbers can now be encoded into a set of (hugely negative) numbers,
and the encoding can be reversed.

This allows APIs that take numbers to take lazy values and intrinsics
that evaluate to numbers.

This change only introduces the capability, it does not use it in any
of the construct libraries yet.

Fixes #1455.
rix0rrr added a commit that referenced this issue May 13, 2019
Numbers can now be encoded into a set of (hugely negative) numbers,
and the encoding can be reversed.

This allows APIs that take numbers to take lazy values and intrinsics
that evaluate to numbers.

This change only introduces the capability, it does not use it in any
of the construct libraries yet.

Fixes #1455.
rix0rrr added a commit that referenced this issue May 13, 2019
Numbers can now be encoded into a set of (hugely negative) numbers,
and the encoding can be reversed.

This allows APIs that take numbers to take lazy values and intrinsics
that evaluate to numbers.

This change only introduces the capability, it does not use it in any
of the construct libraries yet.

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