You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@directory.apache.org by "bdemers (via GitHub)" <gi...@apache.org> on 2023/12/07 15:41:17 UTC
[PR] Draft: Adding tests for Patch Generation [directory-scimple]
bdemers opened a new pull request, #435:
URL: https://github.com/apache/directory-scimple/pull/435
(no comment)
--
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@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Improve Patch Generation logic and add tests [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on PR #435:
URL: https://github.com/apache/directory-scimple/pull/435#issuecomment-1858521965
splitting this PR up, see: ##451
--
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@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Draft: Adding tests for Patch Generation [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on code in PR #435:
URL: https://github.com/apache/directory-scimple/pull/435#discussion_r1419171006
##########
scim-core/src/main/java/org/apache/directory/scim/core/repository/PatchGenerator.java:
##########
@@ -305,7 +306,7 @@ private List<PatchOperation> handleAttributes(JsonNode valueNode, PatchOperation
} else {
log.debug("Attribute: null doesn't implement TypedAttribute, can't create ValueFilterExpression");
}
- valueFilterExpression = new AttributeComparisonExpression(new AttributeReference("value"), CompareOperator.EQ, "?");
+ valueFilterExpression = new AttributeComparisonExpression(new AttributeReference("value"), CompareOperator.EQ, valueNode.textValue());
Review Comment:
The bits were wrong `"?"`, but assuming there is always a simple value here isn't right either
--
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@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Improve Patch Generation logic and add tests [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on code in PR #435:
URL: https://github.com/apache/directory-scimple/pull/435#discussion_r1426198499
##########
scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/schema/Schemas.java:
##########
@@ -201,6 +202,7 @@ private static List<Schema.Attribute> createAttributes(String urn, List<Field> f
log.debug("Attempting to set the attribute type, raw value = " + typeName);
switch (typeName) {
case STRING_TYPE_IDENTIFIER:
+ case STRING_TYPE:
Review Comment:
I need to add an issue to make coverage is good here.
These issues likely only affect Extensions, but that is still important.
The issue I was seeing is one of the Test object Extensions has a list of strings. Without this fix that list was, the list was showing up as a complex type, and would not diff correctly
--
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@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Improve Patch Generation logic and add tests [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on PR #435:
URL: https://github.com/apache/directory-scimple/pull/435#issuecomment-1858148133
Before merging, i'll break this up into more isolated changes
--
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@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Improve Patch Generation logic and add tests [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers merged PR #435:
URL: https://github.com/apache/directory-scimple/pull/435
--
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@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Improve Patch Generation logic and add tests [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on code in PR #435:
URL: https://github.com/apache/directory-scimple/pull/435#discussion_r1428524926
##########
scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/schema/Schemas.java:
##########
@@ -201,6 +202,7 @@ private static List<Schema.Attribute> createAttributes(String urn, List<Field> f
log.debug("Attempting to set the attribute type, raw value = " + typeName);
switch (typeName) {
case STRING_TYPE_IDENTIFIER:
+ case STRING_TYPE:
Review Comment:
Not related to this PR, but there was a similar issue in #425
Added more robust fix and tests in #452
--
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@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Draft: Adding tests for Patch Generation [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on code in PR #435:
URL: https://github.com/apache/directory-scimple/pull/435#discussion_r1419170038
##########
scim-core/src/main/java/org/apache/directory/scim/core/repository/PatchGenerator.java:
##########
@@ -205,6 +205,7 @@ private List<PatchOperation> createPatchOperations() throws IllegalArgumentExcep
JsonNode node2 = objectMapper.valueToTree(resource);
nullEmptyLists(node2);
JsonNode differences = JsonDiff.asJson(node1, node2, DiffFlags.dontNormalizeOpIntoMoveAndCopy());
+// JsonNode differences = JsonDiff.asJson(node1, node2, EnumSet.of(DiffFlags.ADD_EXPLICIT_REMOVE_ADD_ON_REPLACE, DiffFlags.OMIT_MOVE_OPERATION, DiffFlags.OMIT_COPY_OPERATION, DiffFlags.ADD_ORIGINAL_VALUE_ON_REPLACE));
Review Comment:
I think we may need to change the flags used to generate the diffs.
If we do that, i assume some of the logic that maps the diff to a SCIM patch request may need to change
--
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@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org