[Cosmos] Fix strictness issues in retryUtility.ts

[bzhang0]

This is a PR for discussion on solving strictness issues in src/retry/retryUtility.ts in #12745.

One of the main things I'm working with is getting around the properties of RequestContext, which defines many fields as ``?```, indicating it could be undefined.

azure-sdk-for-js/sdk/cosmosdb/cosmos/src/request/RequestContext.ts

Lines 17 to 36 in 581aeea

	export interface RequestContext {
	path?: string;
	operationType?: OperationType;
	client?: ClientContext;
	retryCount?: number;
	resourceType?: ResourceType;
	resourceId?: string;
	globalEndpointManager: GlobalEndpointManager;
	connectionPolicy: ConnectionPolicy;
	requestAgent: Agent;
	body?: any;
	headers?: CosmosHeaders;
	endpoint?: string;
	method: HTTPMethod;
	partitionKeyRangeId?: string;
	options: FeedOptions | RequestOptions;
	plugins: PluginConfig[];
	partitionKey?: PartitionKey;
	pipeline?: Pipeline;
	}

However, when creating other objects, I run into strict errors as these are required to be defined.

EndpointDiscoveryRetryPolicy

azure-sdk-for-js/sdk/cosmosdb/cosmos/src/retry/retryUtility.ts

Line 47 in 581aeea

endpointDiscoveryRetryPolicy: new EndpointDiscoveryRetryPolicy(

azure-sdk-for-js/sdk/cosmosdb/cosmos/src/retry/endpointDiscoveryRetryPolicy.ts

Lines 29 to 30 in 581aeea

	private globalEndpointManager: GlobalEndpointManager,
	private operationType: OperationType

or

ResourceThrottleRetryPolicy,

azure-sdk-for-js/sdk/cosmosdb/cosmos/src/retry/retryUtility.ts

Line 51 in 581aeea

resourceThrottleRetryPolicy: new ResourceThrottleRetryPolicy(

azure-sdk-for-js/sdk/cosmosdb/cosmos/src/retry/resourceThrottleRetryPolicy.ts

Lines 27 to 28 in 581aeea

	private maxTries: number = 9,
	private fixedRetryIntervalInMs: number = 0,

I've drafted two solutions, one with undefined and one with null, but I'm not sure if that's the correct direction to go in, especially as TypeScript isn't my primary language - still trying to learn, though! I would really appreciate some guidance on how to approach these issues!

I've intentionally taken retryUtility.ts out of tsconfig.strict.json so you can see the errors I'm dealing with (currently working on line 47 through 60)

azure-sdk-for-js/sdk/cosmosdb/cosmos/src/retry/retryUtility.ts

Lines 47 to 60 in 581aeea

	endpointDiscoveryRetryPolicy: new EndpointDiscoveryRetryPolicy(
	requestContext.globalEndpointManager,
	requestContext.operationType ? requestContext.operationType : null
	),
	resourceThrottleRetryPolicy: new ResourceThrottleRetryPolicy(
	requestContext.connectionPolicy.retryOptions.maxRetryAttemptCount
	? requestContext.connectionPolicy.retryOptions.maxRetryAttemptCount
	: null,
	requestContext.connectionPolicy.retryOptions.fixedRetryIntervalInMilliseconds
	? requestContext.connectionPolicy.retryOptions.fixedRetryIntervalInMilliseconds
	: null,
	requestContext.connectionPolicy.retryOptions.maxWaitTimeInSeconds
	? requestContext.connectionPolicy.retryOptions.maxWaitTimeInSeconds
	: null

Thank you!

[ghost]

Thank you for your contribution bzhang0! We will review the pull request and get back to you soon.

[deyaaeldeen]

Thanks! here're a few thoughts

I've drafted two solutions, one with undefined and one with null, but I'm not sure if that's the correct direction to go in, especially as TypeScript isn't my primary language - still trying to learn, though! I would really appreciate some guidance on how to approach these issues!

No worries, feel free to ask any questions. I would propose defining a helper function as follows:

function errorIfNotDefined<T>(input: T | undefined): T {
  if (input === undefined) {
    throw new Error("Expected input to be defined but it is not!");
  }
  return input;
}

and call it on all such optional properties. However, please note that doing this means that we will throw an error at the time of the call even if the optional property is not needed eventually. My advice here is to check the code that uses these properties and determine if they're needed to be defined. If it turns out a property is not always needed, consider changing the input type for it in the using code to allow for undefined (similar to the input type in the helper function above).

Also, it seems like this module you're fixing is importing things that are not strict, could we start with modules that do not have such imports first as I explained in the issue?

[bzhang0]

Thanks! here're a few thoughts

I've drafted two solutions, one with undefined and one with null, but I'm not sure if that's the correct direction to go in, especially as TypeScript isn't my primary language - still trying to learn, though! I would really appreciate some guidance on how to approach these issues!

No worries, feel free to ask any questions. I would propose defining a helper function as follows:

function errorIfNotDefined<T>(input: T | undefined): T {
  if (input === undefined) {
    throw new Error("Expected input to be defined but it is not!");
  }
  return input;
}

and call it on all such optional properties. However, please note that doing this means that we will throw an error at the time of the call even if the optional property is not needed eventually. My advice here is to check the code that uses these properties and determine if they're needed to be defined. If it turns out a property is not always needed, consider changing the input type for it in the using code to allow for undefined (similar to the input type in the helper function above).

Also, it seems like this module you're fixing is importing things that are not strict, could we start with modules that do not have such imports first as I explained in the issue?

Sorry for the late reply - been caught up with school work. I understand that I'll need to work with modules that don't import things that are not strict. With that in mind, I've been looking at src/common/helper.ts, which imports the following:

azure-sdk-for-js/sdk/cosmosdb/cosmos/src/common/helper.ts

Lines 3 to 4 in 581aeea

	import { CosmosClientOptions } from "../CosmosClientOptions";
	import { OperationType, ResourceType } from "./constants";

I'm looking through my rush build log and I don't see any errors coming from these files, so I think I will start working on this file instead. What do you think?

[deyaaeldeen]

Sorry for the late reply - been caught up with school work. I understand that I'll need to work with modules that don't import things that are not strict. With that in mind, I've been looking at src/common/helper.ts, which imports the following:

azure-sdk-for-js/sdk/cosmosdb/cosmos/src/common/helper.ts

Lines 3 to 4 in 581aeea

	import { CosmosClientOptions } from "../CosmosClientOptions";
	import { OperationType, ResourceType } from "./constants";

I'm looking through my rush build log and I don't see any errors coming from these files, so I think I will start working on this file instead. What do you think?

Sounds great!