You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@syncope.apache.org by GitBox <gi...@apache.org> on 2022/10/12 10:51:03 UTC
[GitHub] [syncope] mmoayyed opened a new pull request, #381: SYNCOPE-1699: Extract key from path for UserUpdate ops if undefined in request body
mmoayyed opened a new pull request, #381:
URL: https://github.com/apache/syncope/pull/381
Please see https://issues.apache.org/jira/browse/SYNCOPE-1699
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@syncope.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] ilgrosso commented on a diff in pull request #381: SYNCOPE-1699: Extract key from path if undefined in request body
Posted by GitBox <gi...@apache.org>.
ilgrosso commented on code in PR #381:
URL: https://github.com/apache/syncope/pull/381#discussion_r993342196
##########
core/idrepo/rest-cxf/src/main/java/org/apache/syncope/core/rest/cxf/service/AbstractService.java:
##########
@@ -61,8 +61,23 @@ public abstract class AbstractService implements JAXRSService {
protected String getActualKey(final AnyDAO<?> dao, final String pretendingKey) {
String actualKey = pretendingKey;
- if (!SyncopeConstants.UUID_PATTERN.matcher(pretendingKey).matches()) {
- actualKey = dao.findKey(pretendingKey);
+ if (uriInfo.getPathParameters(true).containsKey("key")) {
Review Comment:
This is how I'd refactor the current proposal:
```java
protected String getActualKey(final AnyDAO<?> dao, final String pretendingKey) {
String actualKey = pretendingKey;
if (actualKey == null && uriInfo.getPathParameters(true).containsKey("key")) {
final String keyInPath = uriInfo.getPathParameters(true).get("key").get(0);
if (actualKey != null && !actualKey.equals(keyInPath)) {
SyncopeClientException sce = SyncopeClientException.build(ClientExceptionType.InvalidRequest);
sce.getElements().add("Key specified in request does not match key in the path");
throw sce;
}
actualKey = keyInPath;
}
if (actualKey == null) {
SyncopeClientException sce = SyncopeClientException.build(ClientExceptionType.InvalidRequest);
sce.getElements().add("Key is null");
throw sce;
}
if (!SyncopeConstants.UUID_PATTERN.matcher(actualKey).matches()) {
actualKey = dao.findKey(actualKey);
}
return actualKey;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@syncope.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] mmoayyed commented on a diff in pull request #381: SYNCOPE-1699: Extract key from path if undefined in request body
Posted by GitBox <gi...@apache.org>.
mmoayyed commented on code in PR #381:
URL: https://github.com/apache/syncope/pull/381#discussion_r993389942
##########
core/idrepo/rest-cxf/src/main/java/org/apache/syncope/core/rest/cxf/service/AbstractService.java:
##########
@@ -61,8 +61,23 @@ public abstract class AbstractService implements JAXRSService {
protected String getActualKey(final AnyDAO<?> dao, final String pretendingKey) {
String actualKey = pretendingKey;
- if (!SyncopeConstants.UUID_PATTERN.matcher(pretendingKey).matches()) {
- actualKey = dao.findKey(pretendingKey);
+ if (uriInfo.getPathParameters(true).containsKey("key")) {
Review Comment:
You're right, thanks for clarifying. I'll revisit this shortly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@syncope.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] ilgrosso commented on a diff in pull request #381: SYNCOPE-1699: Extract key from path if undefined in request body
Posted by GitBox <gi...@apache.org>.
ilgrosso commented on code in PR #381:
URL: https://github.com/apache/syncope/pull/381#discussion_r993326009
##########
core/idrepo/rest-cxf/src/main/java/org/apache/syncope/core/rest/cxf/service/AbstractService.java:
##########
@@ -61,8 +61,23 @@ public abstract class AbstractService implements JAXRSService {
protected String getActualKey(final AnyDAO<?> dao, final String pretendingKey) {
String actualKey = pretendingKey;
- if (!SyncopeConstants.UUID_PATTERN.matcher(pretendingKey).matches()) {
- actualKey = dao.findKey(pretendingKey);
+ if (uriInfo.getPathParameters(true).containsKey("key")) {
Review Comment:
what I was meaning is that if you call `getActualKey(dao, null)` the current proposed implementation will not even look at path parameters, but just raise the exception `Key is null`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@syncope.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] ilgrosso commented on a diff in pull request #381: SYNCOPE-1699: Extract key from path if undefined in request body
Posted by GitBox <gi...@apache.org>.
ilgrosso commented on code in PR #381:
URL: https://github.com/apache/syncope/pull/381#discussion_r993307335
##########
core/idrepo/rest-cxf/src/main/java/org/apache/syncope/core/rest/cxf/service/AbstractService.java:
##########
@@ -61,8 +61,23 @@ public abstract class AbstractService implements JAXRSService {
protected String getActualKey(final AnyDAO<?> dao, final String pretendingKey) {
String actualKey = pretendingKey;
- if (!SyncopeConstants.UUID_PATTERN.matcher(pretendingKey).matches()) {
- actualKey = dao.findKey(pretendingKey);
+ if (uriInfo.getPathParameters(true).containsKey("key")) {
Review Comment:
In the current form we will never be looking to path parameters if the passed `pretendingKey` is `null`, no?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@syncope.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] mmoayyed commented on a diff in pull request #381: SYNCOPE-1699: Extract key from path if undefined in request body
Posted by GitBox <gi...@apache.org>.
mmoayyed commented on code in PR #381:
URL: https://github.com/apache/syncope/pull/381#discussion_r993344004
##########
core/idrepo/rest-cxf/src/main/java/org/apache/syncope/core/rest/cxf/service/AbstractService.java:
##########
@@ -61,8 +61,23 @@ public abstract class AbstractService implements JAXRSService {
protected String getActualKey(final AnyDAO<?> dao, final String pretendingKey) {
String actualKey = pretendingKey;
- if (!SyncopeConstants.UUID_PATTERN.matcher(pretendingKey).matches()) {
- actualKey = dao.findKey(pretendingKey);
+ if (uriInfo.getPathParameters(true).containsKey("key")) {
Review Comment:
In this version, looks like if actual-key is not null, you would never catch the mismatch between the path key and the request key.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@syncope.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] mmoayyed merged pull request #381: SYNCOPE-1699: Extract key from path if undefined in request body
Posted by GitBox <gi...@apache.org>.
mmoayyed merged PR #381:
URL: https://github.com/apache/syncope/pull/381
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@syncope.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] ilgrosso commented on a diff in pull request #381: SYNCOPE-1699: Extract key from path if undefined in request body
Posted by GitBox <gi...@apache.org>.
ilgrosso commented on code in PR #381:
URL: https://github.com/apache/syncope/pull/381#discussion_r993347291
##########
core/idrepo/rest-cxf/src/main/java/org/apache/syncope/core/rest/cxf/service/AbstractService.java:
##########
@@ -61,8 +61,23 @@ public abstract class AbstractService implements JAXRSService {
protected String getActualKey(final AnyDAO<?> dao, final String pretendingKey) {
String actualKey = pretendingKey;
- if (!SyncopeConstants.UUID_PATTERN.matcher(pretendingKey).matches()) {
- actualKey = dao.findKey(pretendingKey);
+ if (uriInfo.getPathParameters(true).containsKey("key")) {
Review Comment:
you're right about the mismatch in my proposal; still, the current one will not even attempt to look at path parameters if the payload key is `null`
So it seems we are attempting to pursue two objectives here:
1. avoid NPE in case the payload bears a `null` key
2. if both path parameter and payload's `key` are provided, check if they are the same
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@syncope.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] ilgrosso commented on a diff in pull request #381: SYNCOPE-1699: Extract key from path if undefined in request body
Posted by GitBox <gi...@apache.org>.
ilgrosso commented on code in PR #381:
URL: https://github.com/apache/syncope/pull/381#discussion_r993326009
##########
core/idrepo/rest-cxf/src/main/java/org/apache/syncope/core/rest/cxf/service/AbstractService.java:
##########
@@ -61,8 +61,23 @@ public abstract class AbstractService implements JAXRSService {
protected String getActualKey(final AnyDAO<?> dao, final String pretendingKey) {
String actualKey = pretendingKey;
- if (!SyncopeConstants.UUID_PATTERN.matcher(pretendingKey).matches()) {
- actualKey = dao.findKey(pretendingKey);
+ if (uriInfo.getPathParameters(true).containsKey("key")) {
Review Comment:
what I was meaning is that if you call `getActualKey(dao, null)` the current proposed implemenation will not even look at path parameters, but just raise the exception `Key is null`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@syncope.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] ilgrosso commented on pull request #381: SYNCOPE-1699: Extract key from path if undefined in request body
Posted by GitBox <gi...@apache.org>.
ilgrosso commented on PR #381:
URL: https://github.com/apache/syncope/pull/381#issuecomment-1276033757
I think you can also add a test case to `UserIssuesITCase` like the following, which would fail with NPE in the current `master`
```java
@Test
public void issueSYNCOPE1699() throws JsonProcessingException {
UserTO userTO = createUser(UserITCase.getUniqueSample("syncope1337@apache.org")).getEntity();
UserUR req = new UserUR();
req.setUsername(new StringReplacePatchItem.Builder().value("newUsername" + getUUIDString()).build());
WebClient webClient = WebClient.create(ADDRESS + "/users/" + userTO.getKey(), ADMIN_UNAME, ADMIN_PWD, null).
accept(MediaType.APPLICATION_JSON_TYPE).
type(MediaType.APPLICATION_JSON_TYPE);
Response response = webClient.invoke("PATCH", JSON_MAPPER.writeValueAsString(req));
assertEquals(Response.Status.OK.getStatusCode(), response.getStatus());
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@syncope.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] ilgrosso commented on a diff in pull request #381: SYNCOPE-1699: Extract key from path if undefined in request body
Posted by GitBox <gi...@apache.org>.
ilgrosso commented on code in PR #381:
URL: https://github.com/apache/syncope/pull/381#discussion_r993304181
##########
core/idrepo/rest-cxf/src/main/java/org/apache/syncope/core/rest/cxf/service/AbstractService.java:
##########
@@ -61,8 +61,23 @@ public abstract class AbstractService implements JAXRSService {
protected String getActualKey(final AnyDAO<?> dao, final String pretendingKey) {
String actualKey = pretendingKey;
- if (!SyncopeConstants.UUID_PATTERN.matcher(pretendingKey).matches()) {
- actualKey = dao.findKey(pretendingKey);
+ if (uriInfo.getPathParameters(true).containsKey("key")) {
Review Comment:
Can we add here `actualKey == null` as first condition the `if` clause? I'd rather avoid decoding path parameters if not strictly needed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@syncope.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] ilgrosso commented on a diff in pull request #381: SYNCOPE-1699: Extract key from path if undefined in request body
Posted by GitBox <gi...@apache.org>.
ilgrosso commented on code in PR #381:
URL: https://github.com/apache/syncope/pull/381#discussion_r993342196
##########
core/idrepo/rest-cxf/src/main/java/org/apache/syncope/core/rest/cxf/service/AbstractService.java:
##########
@@ -61,8 +61,23 @@ public abstract class AbstractService implements JAXRSService {
protected String getActualKey(final AnyDAO<?> dao, final String pretendingKey) {
String actualKey = pretendingKey;
- if (!SyncopeConstants.UUID_PATTERN.matcher(pretendingKey).matches()) {
- actualKey = dao.findKey(pretendingKey);
+ if (uriInfo.getPathParameters(true).containsKey("key")) {
Review Comment:
This is how I'd refactor the current proposal:
```java
protected String getActualKey(final AnyDAO<?> dao, final String pretendingKey) {
String actualKey = pretendingKey;
if (actualKey == null && uriInfo.getPathParameters(true).containsKey("key")) {
final String keyInPath = uriInfo.getPathParameters(true).get("key").get(0);
if (actualKey != null && !actualKey.equals(keyInPath)) {
SyncopeClientException sce = SyncopeClientException.build(ClientExceptionType.InvalidRequest);
sce.getElements().add("Key specified in request does not match key in the path");
throw sce;
}
}
if (actualKey == null) {
SyncopeClientException sce = SyncopeClientException.build(ClientExceptionType.InvalidRequest);
sce.getElements().add("Key is null");
throw sce;
}
if (!SyncopeConstants.UUID_PATTERN.matcher(actualKey).matches()) {
actualKey = dao.findKey(actualKey);
}
return actualKey;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@syncope.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] mmoayyed commented on pull request #381: SYNCOPE-1699: Extract key from path if undefined in request body
Posted by GitBox <gi...@apache.org>.
mmoayyed commented on PR #381:
URL: https://github.com/apache/syncope/pull/381#issuecomment-1276119538
I think my original implementation of this was correct. Please take another look. I included your test as well as one extra one to show how this would work. (Thanks for the test By the way!)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@syncope.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] mmoayyed commented on a diff in pull request #381: SYNCOPE-1699: Extract key from path if undefined in request body
Posted by GitBox <gi...@apache.org>.
mmoayyed commented on code in PR #381:
URL: https://github.com/apache/syncope/pull/381#discussion_r993324192
##########
core/idrepo/rest-cxf/src/main/java/org/apache/syncope/core/rest/cxf/service/AbstractService.java:
##########
@@ -61,8 +61,23 @@ public abstract class AbstractService implements JAXRSService {
protected String getActualKey(final AnyDAO<?> dao, final String pretendingKey) {
String actualKey = pretendingKey;
- if (!SyncopeConstants.UUID_PATTERN.matcher(pretendingKey).matches()) {
- actualKey = dao.findKey(pretendingKey);
+ if (uriInfo.getPathParameters(true).containsKey("key")) {
Review Comment:
You would, if the path parameter has a key that does not match the supplied key in the request body. You could in theory be passing one key in the URL, and use another key in the body.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@syncope.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] ilgrosso commented on a diff in pull request #381: SYNCOPE-1699: Extract key from path if undefined in request body
Posted by GitBox <gi...@apache.org>.
ilgrosso commented on code in PR #381:
URL: https://github.com/apache/syncope/pull/381#discussion_r993347291
##########
core/idrepo/rest-cxf/src/main/java/org/apache/syncope/core/rest/cxf/service/AbstractService.java:
##########
@@ -61,8 +61,23 @@ public abstract class AbstractService implements JAXRSService {
protected String getActualKey(final AnyDAO<?> dao, final String pretendingKey) {
String actualKey = pretendingKey;
- if (!SyncopeConstants.UUID_PATTERN.matcher(pretendingKey).matches()) {
- actualKey = dao.findKey(pretendingKey);
+ if (uriInfo.getPathParameters(true).containsKey("key")) {
Review Comment:
you're right about the mismatch in my proposal; still, the current one will not even attempt to look at path parameters if the payload key is `null`
So it seems we are attempting to pursue to objectives here:
1. avoid NPE in case the payload bears a `null` key
2. if both path parameter and payload's `key` are provided, check if they are the same
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@syncope.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] ilgrosso commented on pull request #381: SYNCOPE-1699: Extract key from path if undefined in request body
Posted by GitBox <gi...@apache.org>.
ilgrosso commented on PR #381:
URL: https://github.com/apache/syncope/pull/381#issuecomment-1276123377
> I think my original implementation of this was correct. Please take another look.
I agree, all began with my tendence to try to optimize around decoding path parameters if not necessary; clarifying that we are pursuing two different things made my original request void.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@syncope.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org