Skip to content

fix array fqdn bug and add unit tests for JsonNodeMapper#37

Merged
vasanthm7 merged 9 commits intolowes:mainfrom
bettyand:add-element-identifiers
Sep 15, 2025
Merged

fix array fqdn bug and add unit tests for JsonNodeMapper#37
vasanthm7 merged 9 commits intolowes:mainfrom
bettyand:add-element-identifiers

Conversation

@bettyand
Copy link
Contributor

@bettyand bettyand commented Aug 5, 2025

Description

I noticed a bug while working on adding element metadata to make the data more searchable. I am raising a PR for this before continuing so it will be easier to review.

The bug specifically applies to top-level arrays, which is probably why it wasn't noticed sooner, but could be a problem if using this library to audit collections. The index was being added to the fqdn twice, creating confusing data and potential comparison issues. This PR enhances the JsonNodeMapper to improve handling of JSON arrays and ensure accurate FQDN generation with proper array indices. The changes include:

  • Added comprehensive test coverage for nested array structures
  • Fixed array index handling in FQDNs to maintain correct grouping
  • Added a helper method to improve readability and stay DRY

The changes maintain backward compatibility while adding more robust handling of complex nested structures.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have added examples for the new scenarios in the relevant modules

@bettyand bettyand marked this pull request as ready for review August 5, 2025 19:17
@bettyand bettyand requested a review from a team as a code owner August 5, 2025 19:17
Copy link
Member

@ankur4u007 ankur4u007 left a comment

Choose a reason for hiding this comment

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

Just few comments, LGTM otherwise.

@bettyand
Copy link
Contributor Author

comments addressed, ready for final review

@bettyand
Copy link
Contributor Author

added some explicit list order tests as suggested by Kunal

Copy link

@d4rk8l1tz d4rk8l1tz left a comment

Choose a reason for hiding this comment

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

@bettyand changes look good.
Question. Have we built a jar with these commits and tested it with an existing client implementation in staging yet or are we waiting for this to merge ?

@vasanthm7 vasanthm7 dismissed ankur4u007’s stale review September 15, 2025 13:54

Closing this since its been reviewed

Copy link
Member

@vasanthm7 vasanthm7 left a comment

Choose a reason for hiding this comment

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

Looks good to me

@vasanthm7 vasanthm7 merged commit 5d0eef0 into lowes:main Sep 15, 2025
1 check passed
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.

4 participants