aws-sdk-js-v3 icon indicating copy to clipboard operation
aws-sdk-js-v3 copied to clipboard

Every field in DescribeOrganizationResponse is optional

Open LinusU opened this issue 1 year ago • 11 comments

Describe the bug

The typings for DescribeOrganizationResponse has every field specified as optional (using the ?: syntax). This makes the following code not compile with TypeScript:

import { WorkMail } from '@aws-sdk/client-workmail'

const workmail = new WorkMail({ region: 'eu-west-1' })

const org = await workmail.describeOrganization({ OrganizationId: 'example' })

// ERROR: `org.OrganizationId` can be undefined
const id: string = org.OrganizationId

This is a minimal example and it's of course easy to work around. But my actual use case involve multiple functions that takes organization: DescribeOrganizationResponse as a parameter.

Expected Behavior

I expected at the very least OrganizationId to be non-optional. And I think that ARN, Alias and State should be as well.

Current Behavior

They are optional, which forces me to either add special error handling that should never trigger, or silence the TypeScript compiler using @ts-expect-error.

Reproduction Steps

import { WorkMail } from '@aws-sdk/client-workmail'

const workmail = new WorkMail({ region: 'eu-west-1' })

const org = await workmail.describeOrganization({ OrganizationId: 'example' })

// ERROR: `org.OrganizationId` can be undefined
const id: string = org.OrganizationId

Possible Solution

Remove the ? for the affected fields:

--- a.ts	2024-01-18 15:58:53
+++ b.ts	2024-01-18 15:59:31
@@ -1826,17 +1826,17 @@
      * @public
      * <p>The identifier of an organization.</p>
      */
-    OrganizationId?: string;
+    OrganizationId: string;
     /**
      * @public
      * <p>The alias for an organization.</p>
      */
-    Alias?: string;
+    Alias: string;
     /**
      * @public
      * <p>The state of an organization.</p>
      */
-    State?: string;
+    State: string;
     /**
      * @public
      * <p>The identifier for the directory associated with an WorkMail organization.</p>
@@ -1868,7 +1868,7 @@
      * @public
      * <p>The Amazon Resource Name (ARN) of the organization.</p>
      */
-    ARN?: string;
+    ARN: string;
     /**
      * @public
      * <p>The user ID of the migration admin if migration is enabled for the organization.</p>

Additional Information/Context

No response

SDK version used

3.490.0

Environment details (OS name and version, etc.)

macOS 14, Node.js 20.9.0

LinusU avatar Jan 18 '24 15:01 LinusU

Similarly, Id on User is also optional:

import { WorkMail } from '@aws-sdk/client-workmail'

const workmail = new WorkMail({ region: 'eu-west-1' })

const list = await workmail.listUsers({ OrganizationId: 'example' })

for (const user of list.Users) {
  // ERROR: `user.Id` can be undefined
  const id: string = user.Id
}

(and also Users on ListUsersResponse)

LinusU avatar Jan 18 '24 15:01 LinusU

Hi there @LinusU - thanks for reaching out.

I was able to confirm that OrganizationId is required param which is also documented in our JavaScript SDK docs and it's working as expected. Can you elaborate more on your statement of "They are optional, which forces me to either add special error handling that should never trigger, or silence the TypeScript compiler using @ts-expect-error."?

aBurmeseDev avatar Jan 25 '24 06:01 aBurmeseDev

@aBurmeseDev hello, thanks for taking a look at this.

I did not mean in the request parameters, but rather in the response.

That is, in the value I have after called and awaited workmail.describeOrganization, the OrganizationId is typed as optional.

If you look at the code snippet under reproduction step, you can see that I cannot assign org.OrganizationId to something that expects a string.

Similarly, after doing listUsers, the Id inside the returned array is also typed as optional, which makes it very hard to work with.

LinusU avatar Jan 25 '24 14:01 LinusU

This seems to affect almost every part of the WorkMail API types, some examples:

  1. createOrganization has OrganizationId on the response as optional: https://github.com/aws/aws-sdk-js-v3/blob/40442fba7e910ad679be583e83cb375bd51d5b15/clients/client-workmail/src/models/models_0.ts#L1150-L1156
  2. descibeOrganization has OrganizationId, Alias, and State on the response as optional: https://github.com/aws/aws-sdk-js-v3/blob/40442fba7e910ad679be583e83cb375bd51d5b15/clients/client-workmail/src/models/models_0.ts#L2197-L2214
  3. createUser has UserId on the response as optional: https://github.com/aws/aws-sdk-js-v3/blob/40442fba7e910ad679be583e83cb375bd51d5b15/clients/client-workmail/src/models/models_0.ts#L1315-L1320
  4. listUsers has Id on the users list as optional: https://github.com/aws/aws-sdk-js-v3/blob/40442fba7e910ad679be583e83cb375bd51d5b15/clients/client-workmail/src/models/models_0.ts#L4639-L4644

LinusU avatar Jan 25 '24 15:01 LinusU

Similarly, in some input types the fields are typed as required but that they can contain undefined. e.g. the types allow calling registerToWorkMail like so:

await workmail.registerToWorkMail({
  Email: undefined,
  EntityId: undefined,
  OrganizationId: undefined
})

https://github.com/aws/aws-sdk-js-v3/blob/40442fba7e910ad679be583e83cb375bd51d5b15/clients/client-workmail/src/models/models_0.ts#L5034-L5062

LinusU avatar Jan 25 '24 15:01 LinusU

Hello @aBurmeseDev, did you get a chance to look further into this? Is there anything that I can do to help?

LinusU avatar Feb 19 '24 11:02 LinusU

Hi @LinusU - thanks for clarifying additional details and apology for the delay. I reached out to service team to get their thoughts on this and haven't heard back but I followed up on it. I will update here once I hear back. P123189247

aBurmeseDev avatar Feb 20 '24 20:02 aBurmeseDev

@aBurmeseDev - Hello! Did you receive an update from the services team? Thanks

LinusU avatar Mar 13 '24 15:03 LinusU

ping @aBurmeseDev - any updates? or anything that I can do from my side in order to help move this forward? 🙏

LinusU avatar Apr 11 '24 10:04 LinusU

Hi @LinusU - apologies for the delay. Service team is aware of this, however I can't say where this will end up on their priority unless it's a bug. If you look at other services like S3 listBucketsOutput, they're also optional: https://github.com/aws/aws-sdk-js-v3/blob/40442fba7e910ad679be583e83cb375bd51d5b15/clients/client-s3/src/models/models_0.ts#L11451-L11463

Also this's out of our (SDK) control as you can see here in smithy model where service API doesn't enforce it to be required: https://github.com/aws/aws-sdk-js-v3/blob/88213abea034a20c1ea1c91906a07916beffd7fb/codegen/sdk-codegen/aws-models/workmail.json#L2740-L2748

With that said, I will post back here when I hear more from service team or you can directly reach out to them via AWS Support.

aBurmeseDev avatar Apr 11 '24 23:04 aBurmeseDev

Thank you for keeping me informed. I feel that this impacts the ergonomics quite a bit so it would be great to have it fixed. Will stay notified for further updates!

LinusU avatar Apr 13 '24 11:04 LinusU