Skip to content

Comments

[REFACTOR] JsonInjector and NetworkDeserializer classes#607

Merged
horgh merged 4 commits intomaxmind:mainfrom
AKakkar28:refactor-code
Sep 28, 2025
Merged

[REFACTOR] JsonInjector and NetworkDeserializer classes#607
horgh merged 4 commits intomaxmind:mainfrom
AKakkar28:refactor-code

Conversation

@AKakkar28
Copy link
Contributor

refactor: improve JsonInjector and NetworkDeserializer for clarity, safety and added JUnit tests as well

  • Replaced conditional chain in JsonInjector with precomputed injectable map and reused Traits object.
  • Added input validation and specific exceptions in NetworkDeserializer, including prefix length checks.
  • Extracted CIDR parsing logic into a helper method for better readability.
  • Added comprehensive JUnit tests for both JsonInjector and NetworkDeserializer, including edge cases and error handling.
  • Fixed NetworkDeserializer tests to compare network properties instead of object equality.

@horgh
These are my changes can you please review these and approve if everything looks good?
I have added test cases, made sure the build passes and not used lightweight prefix validation.

Thanks!

…arsing, validation, and created test classes as well
@AKakkar28
Copy link
Contributor Author

@horgh does this look good?

@horgh
Copy link
Contributor

horgh commented Sep 26, 2025

Hi @AKakkar28! Sorry I haven't had time to look at this fully yet. However while reviewing it I noticed that JsonInjector was mostly not used (note how we pass the second two parameters as null in the only use). So I made #608 to remove it. Thanks for helping us notice that! You'll need to rebase to incorporate that removal though.

@AKakkar28
Copy link
Contributor Author

@horgh I see got it. I have rebased and now this PR focuses on validating CIDR format and prefix (IPv4: 0–32, IPv6: 0–128)
and extracts parsing helper and adds unit tests for valid inputs.

Copy link
Contributor

@horgh horgh 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! Thanks for the tests!

I have a couple minor comments.

@AKakkar28
Copy link
Contributor Author

@horgh Done and I updated the test as well. Please review and approve it.
Thanks.

@AKakkar28 AKakkar28 requested a review from horgh September 27, 2025 20:08
@horgh horgh merged commit cadff97 into maxmind:main Sep 28, 2025
12 checks passed
@horgh
Copy link
Contributor

horgh commented Sep 28, 2025

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants