-
Notifications
You must be signed in to change notification settings - Fork 545
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
Add support to connect to the LoginFlow AI service #6260
base: master
Are you sure you want to change the base?
Conversation
# Conflicts: # components/application-mgt/org.wso2.carbon.identity.application.mgt/pom.xml # pom.xml
- Define constants for reusable keys - Remove token decoding and get the clientId from the AI KEY
# Conflicts: # components/ai-services-mgt/org.wso2.carbon.ai.service.mgt/src/main/java/org/wso2/carbon/ai/service/mgt/constants/AIConstants.java # components/ai-services-mgt/org.wso2.carbon.ai.service.mgt/src/main/java/org/wso2/carbon/ai/service/mgt/token/AIAccessTokenManager.java # components/ai-services-mgt/org.wso2.carbon.ai.service.mgt/src/main/java/org/wso2/carbon/ai/service/mgt/util/AIHttpClientUtil.java # components/application-mgt/org.wso2.carbon.identity.application.mgt/pom.xml # pom.xml
...on.mgt/src/main/java/org/wso2/carbon/identity/application/mgt/ai/LoginFlowAIManagerImpl.java
Outdated
Show resolved
Hide resolved
...on.mgt/src/main/java/org/wso2/carbon/identity/application/mgt/ai/LoginFlowAIManagerImpl.java
Outdated
Show resolved
Hide resolved
92b41bd
to
73a9b36
Compare
*/ | ||
public enum ErrorMessages { | ||
|
||
MAXIMUM_RETRIES_EXCEEDED("AI_10000", "Maximum retries exceeded to retrieve the access token."), |
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.
Make the error code consistent with other components. IIRC we don't us "_" instead use "-"
...i.service.mgt/src/main/java/org/wso2/carbon/ai/service/mgt/exceptions/AIServerException.java
Outdated
Show resolved
Hide resolved
public class AIServerException extends Exception { | ||
|
||
private String errorCode; | ||
private AIHttpClientUtil.HttpResponseWrapper loginFlowAIResponse; |
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.
Let's stop transmitting the error details as a object and include the details directly into the exception
...i.service.mgt/src/main/java/org/wso2/carbon/ai/service/mgt/exceptions/AIClientException.java
Outdated
Show resolved
Hide resolved
return new AccessTokenRequestHelper(AI_KEY, AI_TOKEN_ENDPOINT, | ||
// Here we keep the default HTTP client to send the token request. | ||
// We open and close it for each request. | ||
HttpAsyncClients.createDefault()); |
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.
I don't see a reason to use the Async Client here. Lets check and switch to closable http client
....ai.service.mgt/src/main/java/org/wso2/carbon/ai/service/mgt/token/AIAccessTokenManager.java
Outdated
Show resolved
Hide resolved
HTTP_CONNECTION_POOL_SIZE_PROPERTY_NAME) != null ? Integer.parseInt(IdentityUtil.getProperty( | ||
HTTP_CONNECTION_POOL_SIZE_PROPERTY_NAME)) : 20; | ||
private static final int HTTP_CONNECTION_TIMEOUT = IdentityUtil.getProperty( | ||
HTTP_CONNECTION_TIMEOUT_PROPERTY_NAME) != null ? Integer.parseInt(IdentityUtil.getProperty( | ||
HTTP_CONNECTION_TIMEOUT_PROPERTY_NAME)) : 60000; // Making the default timeout 60 seconds. |
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.
move the defaults to a constant.
|
||
try { | ||
String accessToken = AIAccessTokenManager.getInstance().getAccessToken(false); | ||
String orgName = AIAccessTokenManager.getInstance().getClientId(); |
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.
lets rename to clientId
Map<String, Object> requestBody = new HashMap<>(); | ||
requestBody.put(USER_QUERY_PROPERTY, userQuery); | ||
try { | ||
// Convert JSONArray to List. |
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.
Lets remove unnecessary comments.
* @throws AIClientException When an error occurs while generating the authentication sequence. | ||
*/ | ||
@Override | ||
public String generateAuthenticationSequence(String userQuery, JSONArray userClaims, |
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.
lets rename userClaims to mean the what we are actually getting (the claim metadata)
MAXIMUM_RETRIES_EXCEEDED("AILF_10000", "Maximum retries exceeded to retrieve the access token."), | ||
UNABLE_TO_ACCESS_AI_SERVICE_WITH_RENEW_ACCESS_TOKEN("AILF_10003", "Unable to access the " + | ||
"AI service with the renewed access token."), |
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.
Are these being used?
import static org.wso2.carbon.base.MultitenantConstants.SUPER_TENANT_DOMAIN_NAME; | ||
import static org.wso2.carbon.base.MultitenantConstants.SUPER_TENANT_ID; | ||
|
||
public class LoginFlowAIManagerTest { |
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.
Missing class comment
- Make the component as identity component (Rename from carbon to carbon.identity) - Remove usage Async http client - Clean the code
aa4eea3
to
f177247
Compare
*/ | ||
public interface LoginFlowAIManager { | ||
|
||
String generateAuthenticationSequence(String userQuery, JSONArray userClaims, JSONObject availableAuthenticators) |
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.
Add method comments
c31c746
to
e61ae0d
Compare
PR builder started |
PR builder completed |
e61ae0d
to
458e913
Compare
PR builder started |
Quality Gate passedIssues Measures |
PR builder completed |
Proposed changes in this pull request
With this PR, the product-is will be able to connect the loginflow AI microservice and generate AI results accordingly
Related Issue(s)