Add a new schema checker option - #301
Conversation
Added a new ALLOW_OBJECT_VALUED_STRING_SUB_ATTRIBUTES schema checker option to allow string-type sub-attributes whose value is a json object. Reviewer: kqarryzada Reviewer: dougbulkley JiraIssue: DS-51562
6722cd7 to
3c32a75
Compare
kqarryzada
left a comment
kqarryzada
left a comment
There was a problem hiding this comment.
This looks good to me. I had some minor comments.
| * Per RFC 7643, complex sub-attributes are not supported; this option | ||
| * relaxes that constraint for non-conformant resources. | ||
| */ | ||
| ALLOW_OBJECT_VALUED_STRING_SUB_ATTRIBUTES |
There was a problem hiding this comment.
This should have a comma at the end.
| { | ||
| if (enabledOptions.contains( | ||
| Option.ALLOW_OBJECT_VALUED_STRING_SUB_ATTRIBUTES) | ||
| && node.isObject() && path.size() > 1) |
There was a problem hiding this comment.
The node type is checked twice, which we can avoid (along with the copied code) by rewriting this with the following. I also think we can rename the option:
case STRING:
if (node.isObject() && path.size() > 1
&& enabledOptions.contains(Option.ALLOW_OBJECT_AS_STRING_SUB_ATTR))
{
return;
}
// Fall through for string evaluation.
case DATETIME:
...| // Build a schema with a complex parent attribute whose sub-attribute is | ||
| // STRING-typed but stores a JSON object value. | ||
| AttributeDefinition.Builder builder = new AttributeDefinition.Builder(); | ||
| builder.setName("iamSvcUsrData"); |
There was a problem hiding this comment.
The old tests don't do this much, but we should leverage the builder pattern on this:
AttributeDefinition.Builder builder = new AttributeDefinition.Builder()
.setName(...)
.setType(...)| assertEquals(results.getSyntaxIssues().size(), 1, | ||
| results.getSyntaxIssues().toString()); | ||
| assertTrue(containsIssueWith(results.getSyntaxIssues(), | ||
| "must be a JSON string")); |
There was a problem hiding this comment.
New unit tests should use AssertJ's assertion statements instead of TestNG's. For example, this should be:
assertThat(results.getSyntaxIssues()).hasSize(1);
assertThat(results.getSyntaxIssues().get(0))
.contains("must be a JSON string");For assertion statements that currently have a reason string, you can use .as() if you really want, but I think it's also okay to drop them from this test.
|
|
||
| List<PatchOperation> patchOps = Collections.singletonList( | ||
| PatchOperation.replace( | ||
| Path.root().attribute("iamServiceUser"), iamServiceUserNode)); |
There was a problem hiding this comment.
If you want, this can use the string attribute directly since the method throws an exception.
2 participants
Added a new ALLOW_OBJECT_VALUED_STRING_SUB_ATTRIBUTES schema checker option to allow string-type sub-attributes whose value is a json object.
Reviewer: kqarryzada
Reviewer: dougbulkley
JiraIssue: DS-51562