MOSIP-20836 - fix holiday “Missing Data” false positives by reusing existing holidayId on create #1267
Conversation
WalkthroughAdds a repository query to fetch holidays by date and location (no language filter) and updates saveHoliday to reuse an existing holiday's holidayId and isActive when the same date and location exist in another language; tests added to verify this reuse behavior. Changes
Sequence DiagramsequenceDiagram
participant Client
participant HolidayServiceImpl
participant HolidayRepository
participant Database
Client->>HolidayServiceImpl: saveHoliday(holidayDto)
HolidayServiceImpl->>HolidayRepository: findFirstByHolidayByHolidayDateLocationCodeLangCode(date, loc, lang)
alt No existing row for same date+loc+lang
HolidayServiceImpl->>HolidayRepository: findFirstByHolidayDateAndLocationCode(date, loc)
HolidayRepository->>Database: Query by date & location (non-deleted)
Database-->>HolidayRepository: existingHoliday?
alt existingHoliday found
HolidayServiceImpl->>HolidayServiceImpl: reuse holidayId & isActive
else no existingHoliday
HolidayServiceImpl->>HolidayRepository: findMaxHolidayId()
HolidayRepository-->>HolidayServiceImpl: maxId
HolidayServiceImpl->>HolidayServiceImpl: assign new holidayId = maxId + 1
end
end
HolidayServiceImpl->>HolidayRepository: save(holiday)
HolidayRepository->>Database: persist
Database-->>HolidayRepository: persisted entity
HolidayRepository-->>HolidayServiceImpl: saved entity
HolidayServiceImpl-->>Client: result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/HolidayServiceImpl.java (1)
259-268: Consider removing the large commented-out blockThe long commented block after
holidayRepository.save(entity)adds noise to an already busy method. If this logic is no longer needed, consider deleting it (or moving it to documentation/ADR) to keepsaveHolidayeasier to read and maintain.admin/kernel-masterdata-service/src/test/java/io/mosip/kernel/masterdata/test/service/HolidayServiceImplTest.java (1)
147-148: Optional: Use a fixed LocalDate in the test for determinismThe test uses
LocalDate.now()fordate. While this works, using a fixed constant (e.g.,LocalDate.of(2025, 1, 1)) would make the test fully deterministic and a bit easier to reason about over time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/repository/HolidayRepository.java(1 hunks)admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/HolidayServiceImpl.java(1 hunks)admin/kernel-masterdata-service/src/test/java/io/mosip/kernel/masterdata/test/service/HolidayServiceImplTest.java(4 hunks)
🔇 Additional comments (3)
admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/HolidayServiceImpl.java (1)
246-255: ID reuse logic across languages looks correctThe new
existingSameHolidaybranch correctly reuses theholidayIdandisActiveof any existing holiday with the same(holidayDate, locationCode)and falls back tofindMaxHolidayId() + 1only when no such record exists. This matches the described fix and keeps the empty-table (count() <= 0) behavior intact.admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/repository/HolidayRepository.java (1)
166-168: Repository helper for cross-language lookup is well-scopedThe new
findFirstByHolidayDateAndLocationCodequery cleanly exposes the “same date and location, any language, non-deleted” lookup thatsaveHolidayneeds, and itsOptional<Holiday>return type matches the conditional reuse logic in the service.admin/kernel-masterdata-service/src/test/java/io/mosip/kernel/masterdata/test/service/HolidayServiceImplTest.java (1)
3-4: New test setup correctly validates holidayId reuse across languagesThe added imports, mocks, and the
saveHoliday_reusesExistingHolidayId_whenSameDateAndLocationDifferentLanguagetest form a solid, focused regression test:
- Mocks
HolidayRepository,LocationRepository, andAuditUtiljust enough for thesaveHolidaypath.- Simulates an existing holiday in one language and a new DTO in another for the same
(holidayDate, locationCode).- Asserts that the saved entity and returned
HolidayIDDtoreuse the existingholidayIdandisActive, and verifies thatfindMaxHolidayId()is never called while the expected repository methods are invoked.This gives good coverage of the new ID-reuse behavior and guards against regressions.
Also applies to: 9-15, 19-22, 32-32, 41-48, 139-236
…xisting holidayId on create Signed-off-by: Youssef MAHTAT <[email protected]>
ymahtat-dev
force-pushed
the
draft-develop-3
branch
from
ddd7a74 to
4ad49d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/HolidayServiceImpl.java (1)
259-267: Consider removing commented-out code.The commented block appears to be obsolete logic. Since version control preserves the history, it's cleaner to remove dead code rather than leave it commented out.
Apply this diff to remove the commented-out code:
holiday = holidayRepository.save(entity); - /* - * if (holiday.getIsActive() == true && - * supportedLang.contains(holiday.getLangCode())) { Holiday - * primholiday=holidayRepository. - * findHolidayByHolidayNameHolidayDateLocationCodeLangCode(holiday. - * getHolidayName(),holiday.getHolidayDate(), - * holiday.getLocationCode(),primaryLang); primholiday.setIsActive(true); - * holidayRepository.update(primholiday); } - */ MapperUtils.map(holiday, holidayId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/repository/HolidayRepository.java(1 hunks)admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/HolidayServiceImpl.java(1 hunks)admin/kernel-masterdata-service/src/test/java/io/mosip/kernel/masterdata/test/service/HolidayServiceImplTest.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/repository/HolidayRepository.java
- admin/kernel-masterdata-service/src/test/java/io/mosip/kernel/masterdata/test/service/HolidayServiceImplTest.java
🧰 Additional context used
🧬 Code graph analysis (1)
admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/HolidayServiceImpl.java (1)
admin/admin-service/src/main/java/io/mosip/admin/config/MapperUtils.java (1)
MapperUtils(15-150)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-maven-admin-services / maven-build
- GitHub Check: build-maven-apitest-masterdata / maven-build
🔇 Additional comments (2)
admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/HolidayServiceImpl.java (2)
246-255: Repository method verified—logic correctly addresses the PR objective.The repository method
findFirstByHolidayDateAndLocationCodeexists and is properly implemented with the correct @query annotation at line 167 of HolidayRepository.java. The query filters by date and location without a language filter, which is the intended behavior for reusingholidayIdandisActiveacross language variants and eliminating "Missing Data" false positives.The new conditional logic (lines 246-255) correctly implements the stated PR objective by falling back to this language-agnostic lookup when an exact language-specific match is not found.
254-254: Verify concurrent safety of ID generation in high-concurrency scenarios.The pattern
findMaxHolidayId() + 1is vulnerable to race conditions: two concurrent requests could fetch the same max ID and attempt simultaneous inserts with identical IDs. While theholidayIdfield has a unique constraint (@Column(unique = true)), this prevents data corruption but causes exceptions rather than preventing the race condition.The condition at line 254 is reached only after checking for existing holidays by (date, location, lang-code) and (date, location), so duplicate scenarios should be rare. However, in high-concurrency environments, consider verifying:
- Whether the current transaction isolation level and unique constraint are sufficient for your deployment.
- If this pattern has caused issues in production (constraint violation exceptions on concurrent creates).
- Whether a database sequence or transactional lock would better suit your requirements.
Note: This is a pre-existing pattern, not introduced by this PR. The original review comment's claim about other usages appears inaccurate—only this single usage was found in the codebase.
2 participants
When saving a Holiday for the same (holidayDate, locationCode) in a new language,
reuse the existing group’s holidayId (and isActive) instead of allocating a new id.
This aligns creation with the backend missing-IDs anti-join (by id) and prevents
other languages rows from staying “missing” after cloning to any secondary language.
Changes:
No API changes. Backward compatible.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.