Updated ContactMapper for participantList deserialization with new SDK#282
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the v2 contact mapping layer to support participantList deserialization behavior for the newer Cardinal SDK model by introducing a version/config switch and propagating suspendable mapping through controllers and pagination helpers.
Changes:
- Add
CardinalVersionConfigand use it inContactV2Mapperto decide whether to mapparticipantsvsparticipantList. - Convert several v2 mappers and mapping call-sites to
suspendto allow config-dependent (potentially I/O) mapping decisions. - Update pagination utilities to accept suspend mappers and bump
kmapversion; remove multiple mapper unit test files.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| mapper/src/test/kotlin/org/taktik/icure/services/external/rest/v2/mapper/ContactMapperTest.kt | Deleted v2 contact mapper tests that previously covered participants/participantList behavior. |
| mapper/src/test/kotlin/org/taktik/icure/services/external/rest/v1/mapper/embed/MeasureMapperTest.kt | Deleted v1 measure mapper tests. |
| mapper/src/test/kotlin/org/taktik/icure/services/external/rest/v1/mapper/ContactMapperTest.kt | Deleted v1 contact mapper tests. |
| mapper/src/main/kotlin/org/taktik/icure/services/external/rest/v2/mapper/requests/ContactBulkShareResultV2Mapper.kt | Makes mapping suspend to align with suspend ContactV2Mapper.map. |
| mapper/src/main/kotlin/org/taktik/icure/services/external/rest/v2/mapper/embed/ImportResultMapper.kt | Makes import result mapping suspend (transitively depends on suspend mappers). |
| mapper/src/main/kotlin/org/taktik/icure/services/external/rest/v2/mapper/ContactMapper.kt | Adds config-driven participant mapping logic and makes mapper methods suspend. |
| libs.versions.toml | Updates kmap version (now includes a -dirty suffix). |
| domain/src/main/kotlin/org/taktik/icure/pagination/PaginationUtils.kt | Changes mapElements to accept a suspend mapper. |
| domain/src/main/kotlin/org/taktik/icure/config/CardinalVersionConfig.kt | Introduces new config interface used by mapping layer. |
| core/src/main/kotlin/org/taktik/icure/services/external/rest/v2/utils/FlowUtils.kt | Changes paginatedList mapper parameter to suspend. |
| core/src/main/kotlin/org/taktik/icure/services/external/rest/v2/controllers/core/ContactController.kt | Adapts controller endpoints to suspend mapping; removes deduplicate param from /match. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ): Flux<ContactDto> = flow { | ||
| val contacts = contactService.createContacts(contactDtos.map { f -> contactV2Mapper.map(f) }) | ||
| return contacts.map { f -> contactV2Mapper.map(f) }.injectReactorContext() | ||
| } | ||
| emitAll(contacts.map { f -> contactV2Mapper.map(f) }) |
There was a problem hiding this comment.
Same issue as above: contactDtos.map { ... } uses the non-suspending List.map but calls the suspending contactV2Mapper.map(...), which will not compile. Please perform the DTO->entity mapping using a suspend-capable approach (e.g. via asFlow().map { ... }.toList() inside the flow {}) before passing the collection to contactService.createContacts(...).
| @Operation(summary = "Get the ids of the Contacts matching the provided filter.") | ||
| @PostMapping("/match", produces = [APPLICATION_JSON_VALUE]) | ||
| fun matchContactsBy( | ||
| @RequestBody filter: AbstractFilterDto<ContactDto>, | ||
| @RequestParam(required = false) deduplicate: Boolean? = null, | ||
| ): Flux<String> = contactService |
There was a problem hiding this comment.
The deduplicate query parameter was removed from the public /rest/v2/contact/match endpoint. Even if it was unused, removing it is a breaking API change for clients still sending the parameter. Consider keeping it (possibly deprecated/ignored) or ensuring this change is versioned/communicated appropriately.
| interface CardinalVersionConfig { | ||
|
|
||
| companion object { | ||
| val minCardinalModelVersion = SemanticVersion("2.0.0") | ||
| } | ||
|
|
||
| suspend fun getUserCardinalVersion(): SemanticVersion? | ||
|
|
||
| suspend fun shouldUseCardinalModel(): Boolean |
There was a problem hiding this comment.
CardinalVersionConfig is introduced as a dependency and is now injected into ContactV2Mapper (via uses = [...]). In this repo there is currently no Spring bean implementing this interface, so application startup will fail with an unsatisfied dependency when the mapper is created. Provide a default implementation (e.g. in core under the app profile) or otherwise ensure a bean is defined wherever these mappers/controllers are used.
libs.versions.toml
Outdated
| kotest = "5.9.1" | ||
| hibernateValidator = "8.0.1.Final" | ||
| kmap = "0.1.84-g107c985e93" | ||
| kmap = "0.1.92-gd1d925cb84-dirty" |
There was a problem hiding this comment.
The kmap version is set to a -dirty build identifier. This is typically not reproducible and is often not published to artifact repositories, which can break dependency resolution in CI/release builds. Prefer pinning to a clean, published version/tag (without -dirty) or a released semver.
| it.groupingBy { participantDto -> participantDto.type }.eachCount().any { entry -> entry.value > 1 } // If there are multiple participant with the same type | ||
| || it.any { (type) -> type == ParticipantTypeDto.Recorder } // or at least one participant with type "Recorder" | ||
| || cardinalVersionConfig.shouldUseCardinalModel() // or the contact was created with the Cardinal SDK |
There was a problem hiding this comment.
Comment typos: "multiple participant" should be "multiple participants".
| ): Flux<ContactDto> = flow { | ||
| val contacts = contactService.modifyContacts(contactDtos.map { f -> contactV2Mapper.map(f) }) | ||
| return contacts.map { f -> contactV2Mapper.map(f) }.injectReactorContext() | ||
| } | ||
| emitAll(contacts.map { f -> contactV2Mapper.map(f) }) |
There was a problem hiding this comment.
In this flow {} block, contactDtos.map { ... } is the stdlib List.map, whose lambda is not suspend. Since contactV2Mapper.map(...) is now suspend, this will not compile. Convert the list to a Flow and map with a suspend lambda (or otherwise perform the DTO->entity mapping in a suspend-aware way) before calling contactService.modifyContacts(...).
TestaDiRapa
deleted the
patch/sdk-version-aware-inverse-mapping-of-contact
branch
No description provided.