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

Incorrect interface types in Amazon CloudFront clients #6165

Closed
3 tasks done
piotrekwitkowski opened this issue Jun 5, 2024 · 4 comments
Closed
3 tasks done

Incorrect interface types in Amazon CloudFront clients #6165

piotrekwitkowski opened this issue Jun 5, 2024 · 4 comments
Assignees
Labels
bug This issue is a bug. duplicate This issue is a duplicate. p2 This is a standard priority issue

Comments

@piotrekwitkowski
Copy link

piotrekwitkowski commented Jun 5, 2024

Checkboxes for prior research

Describe the bug

Many of the required strings in the client's models are incorrectly typed as string | undefined, but required in practice. They should be typed as string instead.

image

This applies for all commands of the client:

There are 13 occurrences of such bug across these 6 methods.

SDK version number

@aws-sdk/client-cloudfront@3.590.0
@aws-sdk/client-cloudfront-keyvaluestore@3.590.0

Which JavaScript Runtime is this issue in?

Node.js

Details of the browser/Node.js/ReactNative version

Node v18.19.0

Reproduction Steps

In the @aws-sdk/client-cloudfront:

import { CloudFrontClient, DescribeKeyValueStoreCommand } from "@aws-sdk/client-cloudfront"; 
const client = new CloudFrontClient({ region: "us-east-1" });
const input = { Name: undefined }; // <-- this field should be required, `string` only, not `string | undefined`
const command = new DescribeKeyValueStoreCommand(input); <- I expect linting error here, because the `Name` field is actually required by the API

Similar for the other client, from @aws-sdk/client-cloudfront-keyvaluestore

import { CloudFrontKeyValueStoreClient, DescribeKeyValueStoreCommand } from "@aws-sdk/client-cloudfront-keyvaluestore"; 
const client = new CloudFrontKeyValueStoreClient({ region: "us-east-1" });
const input = { Name: undefined }; // <-- this field should be required, `string` only, not `string | undefined`
const command = new DescribeKeyValueStoreCommand(input); <- I expect linting error here, because the `Name` field is actually required by the API

Observed Behavior

These types should be strings - these fields are required by the API, and there is a runtime error if the value is not set:

    "ValidationException: 1 validation error detected: Value null at 'ifMatch' failed to satisfy constraint: Member must not be null",
    "    at de_ValidationExceptionRes (/var/runtime/node_modules/@aws-sdk/client-cloudfront-keyvaluestore/dist-cjs/index.js:585:21)",
    "    at de_CommandError (/var/runtime/node_modules/@aws-sdk/client-cloudfront-keyvaluestore/dist-cjs/index.js:502:19)",
...

Expected Behavior

1/ SDK type mentioning string only,
2/ my IDE / TypeScript able to detect this behavior at lint/compile time.

Possible Solution

Fix autogenerated types

Additional Information/Context

I may be able to fix the bug, please just tell me where to start.

@piotrekwitkowski piotrekwitkowski added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 5, 2024
@aBurmeseDev
Copy link
Member

aBurmeseDev commented Jun 6, 2024

Hi @piotrekwitkowski - thanks for reaching out.

I'm not able to reproduce with the code below. Can you share reproducible code for the error?

import { CloudFrontClient, DescribeKeyValueStoreCommand } from "@aws-sdk/client-cloudfront"; 
const client = new CloudFrontClient({region: "us-west-1"});
const input = { 
  Name: "STRING_VALUE", 
};
const command = new DescribeKeyValueStoreCommand(input);
const response = await client.send(command);

Can you also share your Node & SDK version as well?

@aBurmeseDev aBurmeseDev self-assigned this Jun 6, 2024
@aBurmeseDev aBurmeseDev added response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. p2 This is a standard priority issue and removed needs-triage This issue or PR still needs to be triaged. labels Jun 6, 2024
@piotrekwitkowski piotrekwitkowski changed the title Incorrect interface types in client-cloudfront-keyvaluestore Incorrect interface types in Amazon CloudFront clients Jun 6, 2024
@piotrekwitkowski
Copy link
Author

piotrekwitkowski commented Jun 6, 2024

Hi, updated Node and SDK version above. The issue is not about the good case, when you provide the Name value. It is about the case where I don't provide the value, which is a problem that should be caught way earlier, to improve developer experience.

@aBurmeseDev
Copy link
Member

Thanks for your response. A few things to address here:

  • Name value is a param required by service API (CloudFront) and it throws error when not provided, which is correct behavior.
  • Which param required in every client call is defined in service models that are generated upstream by service teams and SDK doesn't have control over it.
  • As far as why we display type as string | undefined vs only string, it's for forward-compatibility with service models and has been addressed a few times on this repo (Typescript types for required and optional attributes are not generated correctly #5992).

Hope it clears things up, let me know if there's any further questions!

@aBurmeseDev aBurmeseDev added the duplicate This issue is a duplicate. label Jun 6, 2024
@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. label Jun 7, 2024
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 Jun 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue is a bug. duplicate This issue is a duplicate. p2 This is a standard priority issue
Projects
None yet
Development

No branches or pull requests

2 participants