-
Notifications
You must be signed in to change notification settings - Fork 149
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
Fix Getting 500 error response for client errors in SCIM/Groups PATCH #478
Fix Getting 500 error response for client errors in SCIM/Groups PATCH #478
Conversation
...y.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManager.java
Outdated
Show resolved
Hide resolved
@@ -3366,6 +3367,11 @@ private void doPatchGroup(String groupId, String currentGroupName, Map<String, L | |||
temporaryMembers.clear(); | |||
|
|||
for (String member : deletedMembers) { | |||
if (addedMembers.isEmpty() && (member == null || member.isEmpty())) { |
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 refactor this.
eg:
for (String member : deletedMembers) {
if (addedMembers.isEmpty() && StringUtils.isBlank(member)) {
throw new BadRequestException(ResponseCodeConstants.INVALID_VALUE);
}
if (StringUtils.isNotBlank(member)) {
String username = UserCoreUtil.addDomainToName(UserCoreUtil.removeDomainFromName(member), userStoreDomainForGroup);
temporaryMembers.add(username);
}
}
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.
Refactored in this fedd887
.github/workflows/pr-builder.yml
Outdated
@@ -19,7 +19,7 @@ jobs: | |||
|
|||
strategy: | |||
matrix: | |||
java-version: [ 11, 17 ] | |||
java-version: [ 11.0.19+7 ] |
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.
any reason for removing java version 17?
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 followed this https://github.com/wso2-extensions/identity-inbound-auth-oauth/blob/6cb4c30737e62537aad1d94c07e88283dcf80198/.github/workflows/pr-builder.yml#L23 as a workaround for the build failure.
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.
include jdk-17.0.7+7 as well
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.
Added in bea2a6a
if (e instanceof org.wso2.carbon.user.core.UserStoreException && StringUtils | ||
.equals(UserCoreErrorConstants.ErrorMessages.ERROR_CODE_NON_EXISTING_USER.getCode(), | ||
((org.wso2.carbon.user.core.UserStoreException) e).getErrorCode())) { | ||
throw new BadRequestException(ResponseCodeConstants.INVALID_VALUE); |
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.
why aren't we passing the throwable?
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.
public class BadRequestException extends AbstractCharonException {
public BadRequestException() {
this("invalid request");
}
public BadRequestException(String scimType) {
this("Request is unparsable, syntactically incorrect, or violates schema.", scimType);
}
public BadRequestException(String details, String scimType) {
super(400, details, scimType);
}
}
This is the implementation of BadRequestException
. There's no way to pass the throwable to the BadRequestException
.
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.
Added a log with the throwable as suggested offline.
PR builder started |
PR builder completed |
PR builder started |
PR builder completed |
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.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/5808197251
Purpose
This PR aims to address the issues mentioned in wso2/product-is#16097.
Goals
The goals of this PR are to:
remove and add a user
scenario by using the user ID. However, this was working with the user display name.