Skip to content

refactor(geoip): replace ObjectMapper instantiation with JsonMapper (Jackson 2 -> Jackson 3)#139

Merged
balazs-szucs merged 2 commits into
grimmory-tools:developfrom
balazs-szucs:bump-Jackson-3
Mar 23, 2026
Merged

refactor(geoip): replace ObjectMapper instantiation with JsonMapper (Jackson 2 -> Jackson 3)#139
balazs-szucs merged 2 commits into
grimmory-tools:developfrom
balazs-szucs:bump-Jackson-3

Conversation

@balazs-szucs

@balazs-szucs balazs-szucs commented Mar 23, 2026

Copy link
Copy Markdown
Contributor

Description

Linked Issue: Fixes #

Changes

Summary by CodeRabbit

  • Refactor
    • Updated internal dependency usage for improved service reliability in geolocation resolution.

@coderabbitai

coderabbitai Bot commented Mar 23, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

The GeoIpService's Jackson dependency imports were updated from the default Jackson namespace to a tools.jackson variant, with the ObjectMapper instantiation method changed from direct instantiation to using JsonMapper's builder pattern. This affects JSON response parsing in country code resolution.

Changes

Cohort / File(s) Summary
Jackson Dependency Update
booklore-api/src/main/java/org/booklore/service/audit/GeoIpService.java
Replaced Jackson imports (com.fasterxml.jackson.databind.*) with tools variant (tools.jackson.databind.*); changed ObjectMapper initialization from new ObjectMapper() to JsonMapper.builder().build().

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐰 A little change, so clean and neat,
With Jackson tools, the imports meet,
The builder pattern's sleek design,
Makes mapper code so fine!

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is incomplete—it contains only the template structure with no actual content describing the changes, objectives, or linked issues. Fill in the description section with details about the Jackson migration, the specific changes made, and any relevant issue numbers or testing information.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: switching from ObjectMapper to JsonMapper as part of a Jackson 2 to Jackson 3 migration in the GeoIP service.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
booklore-api/src/main/java/org/booklore/service/audit/GeoIpService.java (1)

28-28: Prefer using the Spring-managed mapper instead of a local instance.

Line 28 creates a mapper with JsonMapper.builder().build(), which can drift from global JSON config used elsewhere. Injecting the configured mapper keeps behavior consistent.

Proposed refactor
-import tools.jackson.databind.json.JsonMapper;
@@
-    private final ObjectMapper objectMapper = JsonMapper.builder().build();
+    private final ObjectMapper objectMapper;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@booklore-api/src/main/java/org/booklore/service/audit/GeoIpService.java` at
line 28, Replace the locally constructed ObjectMapper in GeoIpService (the field
named objectMapper built with JsonMapper.builder().build()) with the
Spring-managed mapper: remove the direct instantiation and accept/inject the
application ObjectMapper bean (e.g., via constructor parameter or `@Autowired`
field) so GeoIpService uses the globally configured mapper instance instead of a
private JsonMapper-built one.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@booklore-api/src/main/java/org/booklore/service/audit/GeoIpService.java`:
- Line 28: Replace the locally constructed ObjectMapper in GeoIpService (the
field named objectMapper built with JsonMapper.builder().build()) with the
Spring-managed mapper: remove the direct instantiation and accept/inject the
application ObjectMapper bean (e.g., via constructor parameter or `@Autowired`
field) so GeoIpService uses the globally configured mapper instance instead of a
private JsonMapper-built one.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 852c2d6b-2134-4c9a-9213-8dc228de4987

📥 Commits

Reviewing files that changed from the base of the PR and between dd4bed6 and 37c034e.

📒 Files selected for processing (1)
  • booklore-api/src/main/java/org/booklore/service/audit/GeoIpService.java

@balazs-szucs balazs-szucs merged commit b6a2394 into grimmory-tools:develop Mar 23, 2026
9 checks passed
zachyale pushed a commit to zachyale/grimmory that referenced this pull request Apr 17, 2026
zachyale pushed a commit to zachyale/grimmory that referenced this pull request Apr 17, 2026
zachyale pushed a commit that referenced this pull request Apr 17, 2026
zachyale pushed a commit that referenced this pull request Apr 22, 2026
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.

1 participant