Skip to content

create session#48639

Open
browndav-msft wants to merge 44 commits intoAzure:bifrost-basefrom
browndav-msft:bifrost/createSession
Open

create session#48639
browndav-msft wants to merge 44 commits intoAzure:bifrost-basefrom
browndav-msft:bifrost/createSession

Conversation

@browndav-msft
Copy link
Copy Markdown
Member

No description provided.

@github-actions github-actions Bot added the Storage Storage Service (Queues, Blobs, Files) label Mar 30, 2026
- downgrade blobserviceversion to 2026_04_06
- change AZURE_LIVE_TEST_SERVICE_VERSION to V2026_04_06 in ci.system.properties in azure-storage-common
- create both sync and async
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 35 out of 35 changed files in this pull request and generated 13 comments.

Comment on lines 18 to 20
use: '@autorest/java@4.1.63'
input-file: https://raw.githubusercontent.com/Azure/azure-rest-api-specs/15d7f54a5389d5906ffb4e56bb2f38fe5525c0d3/specification/storage/data-plane/Microsoft.BlobStorage/stable/2026-06-06/blob.json
input-file: https://raw.githubusercontent.com/nickliu-msft/azure-rest-api-specs/013866b01623e6f2cc6c313b44c9c6460de3e91e/specification/storage/data-plane/Microsoft.BlobStorage/stable/2026-10-06/blob.json
java: true
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The input-file points to a personal GitHub fork (nickliu-msft/azure-rest-api-specs) and references a future-dated spec folder (2026-10-06). This makes generation non-reproducible and likely breaks for others/CI; please point to an official Azure/azure-rest-api-specs commit and a spec version that exists there.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 35 out of 35 changed files in this pull request and generated 8 comments.

Comment on lines +1 to 2
AZURE_LIVE_TEST_SERVICE_VERSION=V2026_04_06
AZURE_STORAGE_SAS_SERVICE_VERSION=2026-06-06
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AZURE_LIVE_TEST_SERVICE_VERSION was downgraded to V2026_04_06, but AZURE_STORAGE_SAS_SERVICE_VERSION remains 2026-06-06. Given this PR also removes 2026-06-06 from BlobServiceVersion, keeping the SAS service version on 2026-06-06 risks tests using a version that the client no longer recognizes/supports. Consider aligning AZURE_STORAGE_SAS_SERVICE_VERSION with the live test version (or documenting why it must remain newer).

Copilot uses AI. Check for mistakes.
}

return new HttpPipelineBuilder().policies(policies.toArray(new HttpPipelinePolicy[0]))
.httpClient(basePipeline.getHttpClient())
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrapWithSessionPolicy rebuilds a new HttpPipeline but only carries over the HttpClient and policies. This drops pipeline-level configuration like the existing Tracer (and any initialization that depended on ClientOptions), and will reinitialize InstrumentationPolicy with default settings. Consider preserving basePipeline.getTracer() (and, if possible, the original ClientOptions), or avoid rebuilding the pipeline by delegating to the existing basePipeline after inserting the session auth behavior.

Suggested change
.httpClient(basePipeline.getHttpClient())
.httpClient(basePipeline.getHttpClient())
.tracer(basePipeline.getTracer())

Copilot uses AI. Check for mistakes.
Comment on lines +84 to +98
public void policyRefreshesNearExpiryWithoutBlockingSyncRequests() {
StorageSessionCredential nearExpiry = credentialWithToken(FIRST_TOKEN, OffsetDateTime.now().plusSeconds(2));
StorageSessionCredential refreshed = credentialWithToken(SECOND_TOKEN);

when(sessionClient.createSessionSync()).thenReturn(nearExpiry);
when(sessionClient.createSessionAsync()).thenReturn(Mono.just(refreshed));

StorageSessionCredential initial = policy.getValidSessionSync();
StorageSessionCredential duringRefresh = policy.getValidSessionSync();
StorageSessionCredential afterRefresh = policy.getValidSessionSync();

assertEquals(FIRST_TOKEN, initial.getSessionToken());
assertEquals(FIRST_TOKEN, duringRefresh.getSessionToken());
assertEquals(SECOND_TOKEN, afterRefresh.getSessionToken());
verify(sessionClient, times(1)).createSessionSync();
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

policyRefreshesNearExpiryWithoutBlockingSyncRequests asserts that the second getValidSessionSync() call returns the pre-refresh token, but refreshSessionInBackground() subscribes immediately. With a fast/inline createSessionAsync() (as in this mock), the refresh can complete before the second call, making the assertion timing-dependent and potentially flaky. Consider making the test tolerant to either outcome or explicitly controlling scheduling/completion of the refresh Mono.

Copilot uses AI. Check for mistakes.
Comment on lines +2176 to +2182
HttpPipelinePolicy capturePolicy = (context, next) -> {
String auth = context.getHttpRequest().getHeaders().getValue(HttpHeaderName.AUTHORIZATION);
if (auth != null) {
capturedAuthHeaders.add(auth);
}
return next.process();
};
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The capturePolicy is added via builder.addPolicy(...), which defaults to a PER_CALL policy. In the pipeline, PER_CALL policies run before auth policies, so context.getHttpRequest().getHeaders().getValue(AUTHORIZATION) will still be null here and capturedAuthHeaders will remain empty, making the assertions below fail. To reliably capture the final Authorization header, capture it after next.process() (or set the policy position to PER_RETRY / after-auth).

Suggested change
HttpPipelinePolicy capturePolicy = (context, next) -> {
String auth = context.getHttpRequest().getHeaders().getValue(HttpHeaderName.AUTHORIZATION);
if (auth != null) {
capturedAuthHeaders.add(auth);
}
return next.process();
};
HttpPipelinePolicy capturePolicy = (context, next) -> next.process().doOnSuccess(response -> {
String auth = context.getHttpRequest().getHeaders().getValue(HttpHeaderName.AUTHORIZATION);
if (auth != null) {
capturedAuthHeaders.add(auth);
}
});

Copilot uses AI. Check for mistakes.
Comment on lines +183 to +189
List<HttpPipelinePolicy> bearerPolicies = new ArrayList<>(policies);
httpsValidation(tokenCredential, "bearer token", endpoint, logger);
String scope = audience != null
? ((audience.toString().endsWith("/") ? audience + ".default" : audience + "/.default"))
: Constants.STORAGE_SCOPE;
bearerPolicies.add(new StorageBearerTokenChallengeAuthorizationPolicy(tokenCredential, scope));

Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addSessionPolicyIfEnabled, the bearerPipeline used for CreateSession is built from the current policies list before later pipeline policies are appended (per-retry policies, after-retry providers like InstrumentationPolicy, response validation, logging, etc.). This means CreateSession requests may behave differently (missing retries/logging/validation) than normal requests. Consider building the bearer-only pipeline from the final policy list (minus the session policy) so CreateSession calls get the same cross-cutting behavior as other requests.

Copilot uses AI. Check for mistakes.
Comment on lines +1521 to +1523
@ServiceMethod(returns = ReturnType.SINGLE)
CreateSessionResponse createSession() {
return createSessionWithResponse(null, Context.NONE).getValue();
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These createSession* methods are package-private even though they’re annotated with @ServiceMethod and return a generated implementation.models.* type. If Create Session is intended to be a customer-facing API, these should be public and ideally return a public model type; otherwise, consider removing @ServiceMethod to avoid implying a supported public surface.

Copilot uses AI. Check for mistakes.
Comment on lines +2187 to +2193
HttpPipelinePolicy capturePolicy = (context, next) -> {
String auth = context.getHttpRequest().getHeaders().getValue(HttpHeaderName.AUTHORIZATION);
if (auth != null) {
capturedAuthHeaders.add(auth);
}
return next.process();
};
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The capturePolicy is added via builder.addPolicy(...), which defaults to a PER_CALL policy. In the pipeline, PER_CALL policies run before auth policies, so context.getHttpRequest().getHeaders().getValue(AUTHORIZATION) will still be null here and capturedAuthHeaders will remain empty, making the assertions below fail. To reliably capture the final Authorization header, capture it after next.process() (or set the policy position to PER_RETRY / after-auth).

Suggested change
HttpPipelinePolicy capturePolicy = (context, next) -> {
String auth = context.getHttpRequest().getHeaders().getValue(HttpHeaderName.AUTHORIZATION);
if (auth != null) {
capturedAuthHeaders.add(auth);
}
return next.process();
};
HttpPipelinePolicy capturePolicy = (context, next) -> next.process().doOnSuccess(ignored -> {
String auth = context.getHttpRequest().getHeaders().getValue(HttpHeaderName.AUTHORIZATION);
if (auth != null) {
capturedAuthHeaders.add(auth);
}
});

Copilot uses AI. Check for mistakes.
@browndav-msft browndav-msft marked this pull request as ready for review April 21, 2026 19:47
@browndav-msft
Copy link
Copy Markdown
Member Author

/azp run java - pullrequest

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@ibrandes
Copy link
Copy Markdown
Member

/azp run java - storage - ci

@azure-pipelines
Copy link
Copy Markdown

No pipelines are associated with this pull request.

@ibrandes
Copy link
Copy Markdown
Member

/azp run java - storage - tests

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Storage Storage Service (Queues, Blobs, Files)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants