Skip to content

Modernize CaseIgnoreObjectNode usages. #302

Merged
kqarryzada merged 2 commits into
masterfrom
DS-51630-update-CaseIgnoreObjectNode
Jun 18, 2026
Merged

Modernize CaseIgnoreObjectNode usages. #302
kqarryzada merged 2 commits into
masterfrom
DS-51630-update-CaseIgnoreObjectNode

Conversation

@kqarryzada

Copy link
Copy Markdown
Collaborator

This commit primarily updates CaseIgnoreObjectNode to be more aligned
with the most recent version of ObjectNode.java from Jackson. These
changes were focused on maintainability by leveraging more code re-use.
Additionally, case-insensitive evaluations have been updated to use
equalsIgnoreCase() where possible, for better performance.

This also enforces the use of a CaseIgnoreObjectNode within the
GenericScimResource class to ensure that these resources always use
case-insensitive fields. This commit uncovered a defect where the
fields would not be treated correctly if a standard ObjectNode was
provided to the constructor. This issue has now been fixed.

Reviewer: dougbulkley
Reviewer: vyhhuang

JiraIssue: DS-51630

@kqarryzada kqarryzada self-assigned this Jun 18, 2026
private record CaseIgnoreKey(@NotNull String key)
{
@NotNull
private final String key;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted to use records for these internal classes to reduce boiler plate.

*/
@Override
@Nullable
public JsonNode findValue(@NotNull final String fieldName)

@kqarryzada kqarryzada Jun 18, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jackson helpfully routes calls like this to the plural variant and returns the first result, so the explicit method override is not needed. It's a tradeoff between maintainability and performance (i.e., walking the whole tree), but I don't think there's that much overhead with this approach.

@kqarryzada
kqarryzada force-pushed the DS-51630-update-CaseIgnoreObjectNode branch from c9d4c46 to 5607560 Compare June 18, 2026 18:39
}
}
return localFoundSoFar;
List<JsonNode> parsed = findParents(propertyName, null)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These explicitly pass null to call our method directly instead of relying on Jackson to do it.

@vyhhuang vyhhuang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good to me.

@kqarryzada
kqarryzada force-pushed the DS-51630-update-CaseIgnoreObjectNode branch from 5607560 to 0518c70 Compare June 18, 2026 21:11
This commit primarily updates CaseIgnoreObjectNode to be more aligned
with the most recent version of ObjectNode.java from Jackson. These
changes were focused on maintainability by leveraging more code re-use.
Additionally, case-insensitive evaluations have been updated to use
equalsIgnoreCase() where possible, for better performance.

This also enforces the use of a CaseIgnoreObjectNode within the
GenericScimResource class to ensure that these resources always use
case-insensitive fields. This commit uncovered a defect where the fields
would not be treated correctly if a standard ObjectNode was provided to
the constructor. This issue has now been fixed.

Reviewer: dougbulkley
Reviewer: vyhhuang

JiraIssue: DS-51630
@kqarryzada
kqarryzada force-pushed the DS-51630-update-CaseIgnoreObjectNode branch from 0518c70 to 70b3352 Compare June 18, 2026 21:21
@kqarryzada
kqarryzada merged commit 02e9262 into master Jun 18, 2026
6 checks passed
@kqarryzada
kqarryzada deleted the DS-51630-update-CaseIgnoreObjectNode branch June 18, 2026 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants