refactor(storage): add foundation deleteObject client

[ashwinkumar6]

Description of changes

• Add foundation DeleteObject client.
• Duplicate code for the time being until all clients can be refactored into foundation. (smaller review PRs)
• Add TODO, Question where appropriate

foundation
├── constants.ts
└── factories
    └── serviceClients
        ├── index.ts
        ├── s3data
        │   ├── constants.ts
        │   ├── deleteObject
        │   │   ├── createDeleteObjectClient.ts
        │   │   ├── createDeleteObjectDeserializer.ts
        │   │   └── createDeleteObjectSerializer.ts
        │   ├── endpointResolver.ts
        │   ├── index.ts
        │   ├── types
        │   │   ├── index.ts
        │   │   ├── sdk.ts
        │   │   └── serviceClient.ts
        │   └── validators
        │       └── isDnsCompatibleBucketName.ts
        └── shared
            ├── retryDecider.ts
            └── serdeUtils
                ├── deserializeBoolean.ts
                ├── deserializeMetadata.ts
                ├── serializeObjectConfigsToHeaders.ts
                ├── serializePathnameObjectKey.ts

Testing

• Tested remove API on sampleApp

Checklist

• PR description included
• yarn test passes
• Unit Tests are changed or added
• Relevant documentation is changed or added (and PR referenced)

Checklist for repo maintainers

• Verify E2E tests for existing workflows are working as expected or add E2E tests for newly added workflows
• New source file paths included in this PR have been added to CODEOWNERS, if appropriate

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[ashika112]

High level looks good to me.

[AllanZhengYP]

The folder structure under factories/serviceClient should better be this IMO:

factories/serviceClient
|_shared
   |_deserialization
   |_serialization
   |_parsePayload.ts // this can be moved into deserialization as well
   |_retryDecider.ts
|_index.ts
|_s3data
   |_constants.ts
   |_createDeleteObjectClient.ts
   |_index.ts
   |_validators
      |_isDnsCompatibleBucketName.ts

Let me what you think! @ashwinkumar6 @cshfang @HuiSF

[HuiSF]

|_deserialization
|_serialization

We are using serde else where :D

[ashwinkumar6]

Updated to

foundation
├── constants.ts
└── factories
    └── serviceClients
        ├── index.ts
        ├── s3data
        │   ├── constants.ts
        │   ├── createDeleteObjectClient.ts
        │   ├── endpointResolver.ts
        │   ├── index.ts
        │   ├── shared
        │   │   └── serde
        │   │       ├── CreateDeleteObjectDeserializer.ts
        │   │       ├── CreateDeleteObjectSerializer.ts
        │   │       └── index.ts
        │   ├── types
        │   │   ├── index.ts
        │   │   ├── sdk.ts
        │   │   └── serviceClient.ts
        │   └── validators
        │       └── isDnsCompatibleBucketName.ts
        └── shared
            ├── retryDecider.ts
            └── serde
                ├── deserializeBoolean.ts
                ├── deserializeMetadata.ts
                ├── serializeObjectConfigsToHeaders.ts
                ├── serializePathnameObjectKey.ts......

[cshfang]

What are the things inside s3data/shared shared between? From the way it looks right now, the two functions don't look share-able?

[ashwinkumar6]

Tied to keep it similar to auth so it's easy to find clients/serde across packages.
But i see your point we can always add shared/serde if needed in future. updated

[ashwinkumar6]

After further discussion, every client has it's own specific serializer deserializer so we can create a folder for each client in this manner

s3data/deleteObject
|--createClient.ts
|--createDeserializer.ts
|--createSerializer.ts

dir structure

foundation
└── factories
    └── serviceClients
        ├── index.ts
        ├── s3data
        │   ├── constants.ts
        │   ├── deleteObject
        │   │   ├── createClient.ts
        │   │   ├── createDeserializer.ts
        │   │   └── createSerializer.ts
        │   ├── endpointResolver.ts
        │   ├── index.ts
        │   ├── types
        │   │   ├── index.ts
        │   │   ├── sdk.ts
        │   │   └── serviceClient.ts
        │   └── validators
        │       └── isDnsCompatibleBucketName.ts
        └── shared
            ├── retryDecider.ts
            └── serde
                ├── deserializeMetadata.ts
                ├── deserializeNumber.ts
                ├── serializeObjectConfigsToHeaders.ts
                ├── serializePathnameObjectKey.ts

[ashika112]

Questions for me here:

• having shared/serde makes it read like its all serializer or deserializer. Rather since these are utils for serde should we put it as shared/utils/serde or shared/serdeUtils?

[ashwinkumar6]

Updated to foundation/factories/serviceClients/shared/serde -> foundation/factories/serviceClients/shared/serdeUtils

latest dir structure cc @ashika112 @cshfang @HuiSF @AllanZhengYP

foundation
├── constants.ts
└── factories
    └── serviceClients
        ├── index.ts
        ├── s3data
        │   ├── constants.ts
        │   ├── deleteObject
        │   │   ├── createDeleteObjectClient.ts
        │   │   ├── createDeleteObjectDeserializer.ts
        │   │   └── createDeleteObjectSerializer.ts
        │   ├── endpointResolver.ts
        │   ├── index.ts
        │   ├── types
        │   │   ├── index.ts
        │   │   ├── sdk.ts
        │   │   └── serviceClient.ts
        │   └── validators
        │       └── isDnsCompatibleBucketName.ts
        └── shared
            ├── retryDecider.ts
            └── serdeUtils
                ├── deserializeBoolean.ts
                ├── deserializeMetadata.ts
                ├── serializeObjectConfigsToHeaders.ts
                ├── serializePathnameObjectKey.ts

[HuiSF]

Suggested change

	return [] as any as T;
	return [] as unknown as T;

Is this double casting really needed? as [] should conform to any[]?

[ashwinkumar6]

Updated to return [] as unknown as T;
Looks like double casting is needed [] as T gives error:

'never[]' is assignable to the constraint of type 'T', but 'T' could be instantiated with a different subtype of constraint 'any[]'

[cshfang]

Does this belong in serde?

[ashwinkumar6]

Proposal comment to rename serde to serdeUtils since technically all of these are just utils/helpers not serde in itself.

With that in mind validateS3RequiredParameter is used by createDeleteObjectSerializer

[AllanZhengYP]

This should me moved to the validators/ folder

[ashwinkumar6]

Sounds good, will address this on the next PR

[AllanZhengYP]

Looks good to me. I only had 1 non-blocking comment. feel free address it in follow PRs?