Decide on versioning policy when switching to core-rest-pipeline

[maorleger]

Came up in a discussion: does switching a GA package to corev2 warrant a major or minor version bump?

Some more details from @xirzec below:

There are 3 relatively minor breaking changes:

• Optional property mode was removed from retryOptions since it only ever had a single enum value and was never used.
• handleRedirects was removed from redirectOptions, as the policy can now be controlled by removing it from the pipeline or setting maxRedirects to 0.
• keepAliveOptions were removed. Keep alive can be disabled on a per-request basis with disableKeepAlive.

These were all fairly niche options that tended to be used internally by our clients rather than set by consumers, but since we did expose them, we should consider what versioning implication this poses.

Alongside the change to remove _response, a strict semver interpretation would be to major version the package, but debatably a minor bump could be sufficient. /cc @chradek @ramya-rao-a @jeremymeng - I think we should have broad agreement on what our policy is here.

Originally posted by @xirzec in https://github.com/Azure/azure-sdk-for-js/pull/15881#discussion_r656365233

[xirzec]

One determinant could be if any of these options/properties show up in existing README snippets or samples. If not, I think we could argue a minor bump is sufficient.

[jeremymeng]

Yes it's unlikely that customers use these options directly. Minor bump is fine.

[chradek]

I feel like the _response change by itself is enough to necessitate the major version bump unless you never exposed it for this particular package. The optional parameters going away could cause compile-time issues if customers are in-lining their options and were previously specifying those parameters, and the disableKeepAlive no longer being honored would be very frustrating if you were a customer that decided they did want to disable it.

Anyway, my vote would be to respect semver and do a major version bump.

[ramya-rao-a]

cc @sarangan12 and @witemple-msft as we were talking about the same recently

[bterlson]

I'm a fan of pedantic semver fwiw. We shouldn't break people and can message it as such. That said, there may be downsides I'm not considering - would be good to surface those more explicitly.

[xirzec]

The only downside of being pedantic in this case appears to be policy around supporting major versions for an extra year, so we'd increase our dev costs each time we did it.

[sarangan12]

My 2 cents: The _response removal should be a major version bump. It could cause overhead for us. But, we could protect customers from unwanted surprises and that should take priority

[xirzec]

Seems like we have consensus. One question though: does the answer change if the sdk in question did work to not expose _response, as ACS did?

[sarangan12]

If the SDK has done work to intentionally not expose _response, then the migration cannot be considered a breaking change solely based on the removal of _response. (But, if there is some other change that might cause a breaking change, that would be a different story)

[ramya-rao-a]

For packages that did expose _response, can we have something in the convenience layer to use the new callback and continue to support the _response?

[maorleger]

We will experiment with supporting compatibility for existing GA'd packages. Starting with KV Admin, which is already migrated over, GA'd, and is currently in beta for the next version, I will try creating a PR preserving compatibility for the October milestone

[willmtemple]

Summary of our discussion (CC @maorleger)

For _response, we can instrument the convenience layer to retrieve requests with onResponse:

• need to preserve the user's onResponse handler if it is set
• only applies to packages that exposed _response in the public API surface.

For removed options, ingest them into the convenience layer request options:

• Deprecate retryOptions.mode, has no effect
• redirectOptions.handleRedirects
  • false => maxRedirects: 0
  • true => maxRedirects: undefined
  • (what do you do if maxRedirects is also specified? Maybe we can throw an Error?)
• keepAliveOptions.enabled
  • false => disableKeepAlive: true
  • true => do nothing

[ramya-rao-a]

what do you do if maxRedirects is also specified? Maybe we can throw an Error

We can document the older option as being kept for backward compat and point folks to the new one. If both are set to conflicting values, I am fine throwing an error asking them to choose 1.

[xirzec]

Since core-http works on WebResourceLikes and core-rest-pipeline works on PipelineRequests, there are some differences:

export interface WebResourceLike {
    // shared members are omitted
    clone(): WebResourceLike; //removed
    decompressResponse?: boolean; // removed
    headers: HttpHeadersLike; // some small differences in this shape
    keepAlive?: boolean; // renamed to disableKeepAlive and flipped
    prepare(options: RequestPrepareOptions): WebResourceLike; //removed
    query?: { // removed, part of URL
        [key: string]: any;
    };
    spanOptions?: SpanOptions; // inside OperationTracingOptions
    // @deprecated (undocumented)
    streamResponseBody?: boolean; // removed, since deprecated
    tracingContext?: Context; // inside OperationTracingOptions
    validateRequestProperties(): void; // removed
}

clone is the worst one, since implementing it actually breaks the way the new pipeline model works, since requests are expected to have a durable object identity

[xirzec]

what do you do if maxRedirects is also specified? Maybe we can throw an Error

We can document the older option as being kept for backward compat and point folks to the new one. If both are set to conflicting values, I am fine throwing an error asking them to choose 1.

You can set maxRedirects to something positive and handleRedirects to false today, but effectively that's the same as setting maxRedirects to 0 again as maxRedirects won't be consulted in that case. I don't think we should throw an error for an edge case we didn't validate previously.

[willmtemple]

Since core-http works on WebResourceLikes and core-rest-pipeline works on PipelineRequests, there are some differences:

export interface WebResourceLike {

    // shared members are omitted

    clone(): WebResourceLike; //removed

    decompressResponse?: boolean; // removed

    headers: HttpHeadersLike; // some small differences in this shape

    keepAlive?: boolean; // renamed to disableKeepAlive and flipped

    prepare(options: RequestPrepareOptions): WebResourceLike; //removed

    query?: { // removed, part of URL

        [key: string]: any;

    };

    spanOptions?: SpanOptions; // inside OperationTracingOptions

    // @deprecated (undocumented)

    streamResponseBody?: boolean; // removed, since deprecated

    tracingContext?: Context; // inside OperationTracingOptions

    validateRequestProperties(): void; // removed

}

clone is the worst one, since implementing it actually breaks the way the new pipeline model works, since requests are expected to have a durable object identity

You know, for all the topics we discussed today about the options types, we glossed over HttpClient itself. It will be difficult to reconcile this in a non-breaking way.

Do you think it's possible to project a PipelineRequest into an interface that is API compatible with WebResourceLike? All optional fields that were removed, we could provide undefined.

For the clone() method, is the assumption of object identity because the pipeline stores a ref to the request and assumes it is the same one that is ultimately passed to a transport?

[jeremymeng]

Storage disables decompression in NodeJS. Could it be achieved by not adding decompression policy into the pipeline?

[xirzec]

For the clone() method, is the assumption of object identity because the pipeline stores a ref to the request and assumes it is the same one that is ultimately passed to a transport?

Nah, it's because core-client uses object identity to associate the request with ServiceClient specific metadata (i.e. OperationSpec and friends) rather than setting these directly on the request object (and requiring clone to be aware of how to copy these properties)

[xirzec]

Storage disables decompression in NodeJS. Could it be achieved by not adding decompression policy into the pipeline?

Yeah they can just remove the policy from the pipeline (or create their own pipeline without it.)

[willmtemple]

I guess the only way this really gets leaked from the Form Recognizer client is through the httpClient constructor option. To be able to support this we'd need to be able to convert:

• PipelineRequest -> WebResourceLike (but I don't think it really matters if clone adheres to the object identity rules of the pipeline, since the HttpClient is the last step in the pipeline)
• HttpOperationResponse -> PipelineResponse

Basically the question is whether we can project a core-http transport policy into a core-rest-pipeline HttpClient.

[maorleger]

I haven't caught up with the conversations yet but as far as delivering on this experiment I probably don't have the bandwidth to take in all the increased scope here...

I am happy to pair or divide-and-conquer with @willmtemple or @xirzec on the rest but I really can only commit to the original scope given other priorities. We should decide what's the best way to get this resolved by the October milestone though if possible - I'll reach out offline

[ramya-rao-a]

@sarangan12 is planning to make the move to core-rest-pipeline in the search package that he owns. He will be taking over the investigations here

[sarangan12]

I have completed the migration for search-documents SDK and documented the steps at the wiki

I do not see any breaking changes except for handling of the _response in the search-documents SDK. Even if other SDKs could possibly have some breaking changes (we could determine it only on case by case basis), they could be handled in the custom layer.

[maorleger]

Another potential breaking change might be that

FullOperationResponse is not 100% compatible with HttpOperationResponse. For example, the http headers type is different. We may have to re-define.

From https://github.com/Azure/azure-sdk-for-js/pull/18736#discussion_r755370326