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

Regression to number types with respect to SDKv2 #5198

Closed
2 tasks
rix0rrr opened this issue Sep 10, 2023 · 3 comments
Closed
2 tasks

Regression to number types with respect to SDKv2 #5198

rix0rrr opened this issue Sep 10, 2023 · 3 comments
Assignees
Labels
feature-request New feature or enhancement. May require GitHub community feedback.

Comments

@rix0rrr
Copy link

rix0rrr commented Sep 10, 2023

Describe the feature

This issue is very similar to #5181.

Apparently, the same problem applies to number types. In SDKv2, number types that are accidentally passed as strings, the SDK converts them to the right type silently.

SDKv3 will not do the conversion, allowing the server call to fail because of mismatched types.

Example:

SDKv2

  const codedeploy = new AWS.CodeDeploy({ region: 'eu-west-1' });

  const input: AWS.CodeDeploy.CreateDeploymentConfigInput = {
    deploymentConfigName: 'testtest',
    computePlatform: 'Lambda',
    trafficRoutingConfig: {
      type: "TimeBasedLinear",
      timeBasedLinear: {
        linearInterval: "1" as any, // The type says 'number' but I'm forcing strings here
        linearPercentage:"5" as any,
      },
    }
  };


  // Following call happily succeeds
  console.log(await codedeploy.createDeploymentConfig(input).promise());

SDKv3

  const codedeploy = new CodeDeploy();

  const input: CreateDeploymentConfigCommandInput = {
    deploymentConfigName: 'testtest',
    computePlatform: 'Lambda',
    trafficRoutingConfig: {
      type: "TimeBasedLinear",
      timeBasedLinear: {
        linearInterval: "1" as any, // The type says 'number' but I'm forcing strings here
        linearPercentage:"5" as any,
      },
    }
  };

  await codedeploy.createDeploymentConfig(input);

The call fails with:

SerializationException: STRING_VALUE can not be converted to an Integer

Use Case

Why does this matter to us?

Same reason as in #5181, we allow users to drive SDK calls using arbitrary, user-controlled parameters that cannot be type checked. We are transparently migrating all SDKv2 calls to SDKv3 calls, and calls that used to work no longer work after the upgrade.

Proposed Solution

Accept the same argument types as in SDKv2

Other Information

No response

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

SDK version used

3.409.0

Environment details (OS name and version, etc.)

@rix0rrr rix0rrr added feature-request New feature or enhancement. May require GitHub community feedback. needs-triage This issue or PR still needs to be triaged. labels Sep 10, 2023
@kuhe
Copy link
Contributor

kuhe commented Sep 11, 2023

we're not likely to implement this, because SDKs have a directive against doing most client side validation and type coercions.

please convert the value at the SDK operation call site to its intended type.

@RanVaknin
Copy link
Contributor

Hi @rix0rrr
Just to add on top of what @kuhe said:

In the past versions of the AWS SDKs there were many customizations that were introduced over the years. The SDK organization as a whole has moved away from implementing customizations as they are not maintainable and instead, the newer SDKs are code generated directly from the API models of each AWS service and rely on the model to dictate the desired behavior.

Since CodeDeploy specifies linearInterval as an Integer, the SDK will be generated with that type:

linearInterval
The number of minutes between each incremental traffic shift of a TimeBasedLinear deployment.

Type: Integer

Unfortunately there is no plan to introduce similar behavior of this sort.

Thanks,
Ran

@RanVaknin RanVaknin closed this as not planned Won't fix, can't repro, duplicate, stale Sep 13, 2023
mergify bot pushed a commit to aws/aws-cdk that referenced this issue Sep 14, 2023
…cted, skips recursive types (#27112)

AWS SDK v3 strictly validates numeric types, as opposed to v2, which allowed, for example, "123" to be passed where an integer was expected.

This script adds all attributes of a numeric type ("byte", "short", "integer", "long", "bigInteger", "float", "double" and "bigDecimal") to a new type coercion map. It also compacts the map by trimming each part of the path to its unique prefix among all the paths in its operation.

We also change the data structure from a list of paths (forming a tree) to a graph/state machine. This is necessary in order to support self-recursive types, which can loop arbitrarily and therefore cannot be represented in fixed length prefix lists.


See aws/aws-sdk-js-v3#5198


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
MrArnoldPalmer pushed a commit to aws/aws-cdk that referenced this issue Sep 14, 2023
…cted, skips recursive types (#27112)

AWS SDK v3 strictly validates numeric types, as opposed to v2, which allowed, for example, "123" to be passed where an integer was expected.

This script adds all attributes of a numeric type ("byte", "short", "integer", "long", "bigInteger", "float", "double" and "bigDecimal") to a new type coercion map. It also compacts the map by trimming each part of the path to its unique prefix among all the paths in its operation.

We also change the data structure from a list of paths (forming a tree) to a graph/state machine. This is necessary in order to support self-recursive types, which can loop arbitrarily and therefore cannot be represented in fixed length prefix lists.


See aws/aws-sdk-js-v3#5198


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
mikewrighton pushed a commit to aws/aws-cdk that referenced this issue Sep 14, 2023
…cted, skips recursive types (#27112)

AWS SDK v3 strictly validates numeric types, as opposed to v2, which allowed, for example, "123" to be passed where an integer was expected.

This script adds all attributes of a numeric type ("byte", "short", "integer", "long", "bigInteger", "float", "double" and "bigDecimal") to a new type coercion map. It also compacts the map by trimming each part of the path to its unique prefix among all the paths in its operation.

We also change the data structure from a list of paths (forming a tree) to a graph/state machine. This is necessary in order to support self-recursive types, which can loop arbitrarily and therefore cannot be represented in fixed length prefix lists.


See aws/aws-sdk-js-v3#5198


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request New feature or enhancement. May require GitHub community feedback.
Projects
None yet
Development

No branches or pull requests

3 participants