feat(listBuckets): Add support for returning partial success #2678
Conversation
Implements the optional parameter `returnPartialSuccess`. If set to `true` and the API response contains an `unreachable` array, the function will return a successful callback (`err: null`) and pass the `unreachable` array as the 5th argument. This allows users to process the available buckets without the entire operation failing on soft errors.
New sample for partial success in ListBuckets (storage_list_buckets_partial_success)
|
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
| nextQuery?: {}, | ||
| apiResponse?: unknown | ||
| apiResponse?: unknown, | ||
| unreachable?: string[] |
There was a problem hiding this comment.
I don't think this is going to work. There is an issue with callbacks / promisify where if one of the arguments is null / undefined none of the subsequent parameters will get filled in correctly. If I remember correctly, if nextQuery is null / undefined apiResponse will always be missing.
There was a problem hiding this comment.
That's a valid concern regarding the positional nature of callback arguments and how utilities like promisify handle sparse (null/undefined) values.
However, based on my testing with the current implementation, we are successfully fetching subsequent parameters, even when nextQuery is null. It seems the underlying promise wrapping or argument handling is either explicitly preserving the position with null values or the consuming code is robust enough to handle the array gaps.
There was a problem hiding this comment.
Another approach would be to group all optional properties into a single object; however, this would result in a breaking change. Please let me know if my understanding is incorrect.
interface BucketMetadata {
nextQuery?: {};
apiResponse?: unknown;
unreachable?: string[];
}
export interface GetBucketsCallbackFixed {
(
err: Error | null,
buckets: Bucket[],
metadata?: BucketMetadata // All optional data is now combined in one object
): void;
}
There was a problem hiding this comment.
Are there any other approaches we can take here besides adding yet another field to the callback? I've never been a fan of how the callbacks were implemented in this library as they generally don't adhere to Node style which should be callback(err, data). Additionally, adding another parameter goes against the style we have generally followed in other places in this library.
There was a problem hiding this comment.
We can use a union type or a structured object in this case, but the customer would need to make some minor adjustments.
-
buckets: (Bucket | string)[]// This union type allows the array to contain both Bucket objects and strings. -
buckets: BucketData// This uses a structured object.
export interface BucketData {
reachableBuckets: Bucket[];
unreachableBuckets?: string[];
}
Description
This change implements the
returnPartialSuccessoption for thegetBuckets(orListBuckets) method to handle scenarios where the external API returns a successful response but includes anunreachablelist of resources.returnPartialSuccessis added to theGetBucketsRequestinterface.true: IfreturnPartialSuccessis set totrueand the API response containsresp.unreachableitems, the method will execute the callback successfully (witherr: null) and pass the array of unreachable bucket IDs as the fifth argument to the callback function.false(or not set) and unreachable items are present, the function follows the existing failure flow (or implicitly returns success, depending on the previous behavior).returnPartialSuccessparameter is correctly included in the query string (qs) sent to the API endpoint, as it is a known API parameter.Impact
This change significantly improves the robustness of the
ListBucketsoperation. Users can now choose to tolerate minor, non-critical failures (like temporary unavailability of a few regions) and successfully retrieve and process the majority of available buckets, preventing a complete application failure.Testing
returnPartialSuccess: trueis set and the API returns anunreachablearray.unreachable) is strictly optional (?) and is only provided when explicitly enabled via the new option.Additional Information
resp.unreachableproperty on a successful HTTP status code.unreachable) was chosen to avoid modifying the primary result type (Bucket[]) and to maintain compatibility with existing consumers who expect the 4-argument signature.Checklist
Fixes # 🦕