- 
                Notifications
    You must be signed in to change notification settings 
- Fork 83
feat(amazonq): add token methods to codewhisperer clients #1958
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| Codecov ReportAttention: Patch coverage is  
 Additional details and impacted files@@                    Coverage Diff                     @@
##           feature/flare-iam-base    #1958      +/-   ##
==========================================================
- Coverage                   65.96%   61.17%   -4.79%     
==========================================================
  Files                         238      240       +2     
  Lines                       50623    52174    +1551     
  Branches                     3385     3112     -273     
==========================================================
- Hits                        33393    31920    -1473     
- Misses                      17170    20198    +3028     
+ Partials                       60       56       -4     
 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
 | 
| | 'onOpenFileDialog' | ||
| | 'onListAvailableModels' | ||
| | 'sendSubscriptionDetails' | ||
| | 'onSubscriptionUpgrade' | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this diff should not be within the scope of this pr
| this.client = this.createAppropriateClient(credentialsProvider, options, sdkInitializator, logging) | ||
| } | ||
|  | ||
| private CreateCodeWhispererConfigurationOptions(): CodeWhispererConfigurationOptions { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create -> create
| if (!creds?.token) { | ||
| throw new Error('Authorization failed, bearer token is not set') | ||
| const options = this.CreateCodeWhispererConfigurationOptions() | ||
| this.client = this.createAppropriateClient(credentialsProvider, options, sdkInitializator, logging) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createAppropriateClient -> createCodeWhispererServiceClient
| nextToken: response.nextToken, | ||
| } | ||
| return this.mapCodeWhispererApiResponseToSuggestion(response, responseContext) | ||
| } else if (this.getCredentialsType() === 'iam') { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you use polymorphism to get rid of all the new  if IAM then else then conditions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be difficult, since CodeWhispererTokenClient and CodeWhispererSigv4Client are generated from service JSON files. Do you know if they can be changed safely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No this is a hard blocker for this PR. You need to get ride of those if else.
CodeWhispererTokenClient and CodeWhispererSigv4Client are generated from service JSON files --> You can build a type casting mechanism. We have implemented this before, see https://github.com/aws/aws-toolkit-vscode/blob/amazonq/v1.74.0/packages/core/src/codewhisperer/client/codewhisperer.ts
| export class CodeWhispererServiceToken extends CodeWhispererServiceBase { | ||
| client: CodeWhispererTokenClient | ||
| export class CodeWhispererService extends CodeWhispererServiceBase { | ||
| client: CodeWhispererClient | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To use polymorphism and eliminate all the if (this.getCredentialsType() === 'iam') { ... } else { ... } checks in your CodeWhispererService class, you need to refactor your code to leverage subclassing and method overriding. This means:
Define an abstract base class (CodeWhispererServiceBase) that has all the method signatures you need.
Implement one subclass for the IAM client (CodeWhispererServiceIAM) and one for the Bearer/Token client (CodeWhispererServiceToken).
Each subclass provides its own implementation for the relevant methods, so you never need to check "what type am I?" at runtime—the correct implementation is invoked automatically.
Example Refactor
- Abstract Base Class
 ts
 abstract class CodeWhispererServiceBase {
 abstract generateSuggestions(request: SomeRequest): Promise;
 abstract codeModernizerCreateUploadUrl(request: ...): Promise<...>;
 // ... all other methods
 }
- IAM Implementation
 ts
 class CodeWhispererServiceIAM extends CodeWhispererServiceBase {
 generateSuggestions(request: SomeRequest): Promise {
 // IAM-specific implementation
 return this.client.generateRecommendations(request).promise();
 }
 codeModernizerCreateUploadUrl(request: ...): Promise<...> {
 throw new Error('Not supported for IAM');
 }
 // ... other IAM-specific overrides
 }
- Token Implementation
 ts
 class CodeWhispererServiceToken extends CodeWhispererServiceBase {
 generateSuggestions(request: SomeRequest): Promise {
 // Token-specific implementation
 return this.client.generateCompletions(request).promise();
 }
 codeModernizerCreateUploadUrl(request: ...): Promise<...> {
 return this.client.createUploadUrl(request).promise();
 }
 // ... other Token-specific overrides
 }
- Construction
 When you create an instance, you decide which subclass to instantiate based on your credentials:
ts
let service: CodeWhispererServiceBase;
if (hasIAMCreds) {
service = new CodeWhispererServiceIAM(...);
} else {
service = new CodeWhispererServiceToken(...);
}
From here on, your code can just call:
ts
await service.generateSuggestions(request);
await service.codeModernizerCreateUploadUrl(request);
5. Advantages
No if/else or switch needed in method bodies.
Each credential type’s logic is isolated and clear.
Adding new credential types or behaviors is easy—just add a new subclass.
Summary:
Polymorphism works when you let the object type pick the correct method at runtime, rather than checking the type with if/else. In your case, this means two (or more) subclasses, each handling their credential-specific behavior.
Would you like a more concrete refactor using your current code as a starting point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
await service.generateSuggestions(request); here request is a union type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the union type to convert the bearer token types into IAM JSON generated types to avoid extra if else.
54c7086    to
    b35472f      
    Compare
  
    b35472f    to
    4f5fc2d      
    Compare
  
    4f5fc2d    to
    d093e42      
    Compare
  
    | } | ||
|  | ||
| public async codeModernizerCreateUploadUrl( | ||
| request: CodeWhispererTokenClient.CreateUploadUrlRequest | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this IAM client should not take the request input of type: CodeWhispererTokenClient.CreateUploadUrlRequest
82eeaf9    to
    e96e1c1      
    Compare
  
    
Problem
The agentic bundle for the CodeWhisperer LSP only supports SSO. If language clients want it to consume IAM credentials, they must start a completely separate bundle which includes the IAM implementation of the CodeWhisperer LSP. This prevents clients from seamlessly switching authentication methods at runtime.
Solution
This is part of #1981.
This PR adds abstract token methods to CodeWhispererServiceBase and StreamingClientServiceBase, so that they can be used polymorphically inside of AmazonQServiceManager.
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.