refactor: enforce clean layer separation across repository, service, and controller by anderslindho · Pull Request #216 · ChannelFinder/ChannelFinderService

anderslindho · 2026-05-07T13:18:02Z

Pushes the codebase toward cleaner layering: repositories are pure persistence adapters, services have no web framework dependencies, and the HTTP boundary is explicit.

Also adds a v0 DTO layer ahead of a planned v1 API, so the two can evolve independently without touching the service or domain layers.

georgweiss

Added in-line comment

github-actions · 2026-05-08T10:01:01Z

Overall Project	12.76% `-5.76%`	❌
Files changed	6.26%	❌

File	Coverage
ChannelProcessorService.java	51.38% `-4.59%`	❌
ChannelService.java	38.36%	🍏
PropertyService.java	22.57% `-0.75%`	❌
MetricsService.java	22.39% `-17.06%`	❌
PropertyRepository.java	0.56% `-9.47%`	❌
TagRepository.java	0.56% `-9.5%`	❌
ChannelRepository.java	0.26% `-5.18%`	❌
ScrollDto.java	0%	❌
PropertyDto.java	0%	❌
SearchResultDto.java	0%	❌
TagDto.java	0%	❌
ChannelDto.java	0%	❌
ChannelFinderEpicsService.java	0% `-1.01%`	❌
ChannelScrollService.java	0%	🍏
TagController.java	0% `-74.24%`	❌
ChannelController.java	0% `-79.49%`	❌
PropertyController.java	0% `-75%`	❌
ChannelProcessorController.java	0% `-13.89%`	❌
ChannelScrollController.java	0% `-81.25%`	❌
V0ExceptionHandler.java	0% `-9.63%`	❌
ChannelMapper.java	0%	❌
PropertyMapper.java	0%	❌
TagMapper.java	0%	❌
StorageException.java	0%	❌

github-actions · 2026-05-21T10:30:49Z

Overall Project	13.19% `-4.43%`	❌
Files changed	0%	❌

File	Coverage
PropertyService.java	22.57% `-0.75%`	❌
PropertyRepository.java	0.56% `-5.67%`	❌
TagRepository.java	0.56% `-5.69%`	❌
ChannelRepository.java	0.23% `-2.9%`	❌
ScrollDto.java	0%	❌
PropertyDto.java	0%	❌
SearchResultDto.java	0%	❌
TagDto.java	0%	❌
ChannelDto.java	0%	❌
TagController.java	0% `-74.24%`	❌
ChannelController.java	0% `-74.7%`	❌
PropertyController.java	0% `-75%`	❌
ChannelProcessorController.java	0% `-13.89%`	❌
ChannelScrollController.java	0% `-81.25%`	❌
V0ExceptionHandler.java	0% `-9.22%`	❌
ChannelMapper.java	0%	❌
PropertyMapper.java	0%	❌
TagMapper.java	0%	❌
RepositoryException.java	0%	❌

simon-ess · 2026-05-21T10:40:32Z

Also, now that you have reverted one commit... maybe just remove both commits from the PR?

github-actions · 2026-05-21T12:33:53Z

Overall Project	13.19% `-4.6%`	❌
Files changed	2.81%	❌

File	Coverage
ChannelService.java	54.3%	🍏
PropertyService.java	22.57% `-0.75%`	❌
PropertyRepository.java	0.56% `-5.89%`	❌
TagRepository.java	0.56% `-5.69%`	❌
ChannelRepository.java	0.23% `-4.07%`	❌
ScrollDto.java	0%	❌
PropertyDto.java	0%	❌
SearchResultDto.java	0%	❌
TagDto.java	0%	❌
ChannelDto.java	0%	❌
TagController.java	0% `-74.24%`	❌
ChannelController.java	0% `-74.7%`	❌
PropertyController.java	0% `-75%`	❌
ChannelProcessorController.java	0% `-13.89%`	❌
ChannelScrollController.java	0% `-81.25%`	❌
V0ExceptionHandler.java	0% `-9.22%`	❌
ChannelMapper.java	0%	❌
PropertyMapper.java	0%	❌
TagMapper.java	0%	❌
RepositoryException.java	0%	❌

simon-ess

There are still several bare Exceptions being caught and rethrown. There may be more, I stopped looking after 3.

github-actions · 2026-05-21T14:12:13Z

Overall Project	13.19% `-4.64%`	❌
Files changed	2.79%	❌

File	Coverage
ChannelService.java	54.3%	🍏
PropertyService.java	22.57% `-0.75%`	❌
PropertyRepository.java	0.56% `-5.89%`	❌
TagRepository.java	0.56% `-5.69%`	❌
ChannelRepository.java	0.23% `-4.35%`	❌
ScrollDto.java	0%	❌
PropertyDto.java	0%	❌
SearchResultDto.java	0%	❌
TagDto.java	0%	❌
ChannelDto.java	0%	❌
TagController.java	0% `-74.24%`	❌
ChannelController.java	0% `-74.7%`	❌
PropertyController.java	0% `-75%`	❌
ChannelProcessorController.java	0% `-13.89%`	❌
ChannelScrollController.java	0% `-81.25%`	❌
V0ExceptionHandler.java	0% `-9.22%`	❌
ChannelMapper.java	0%	❌
PropertyMapper.java	0%	❌
TagMapper.java	0%	❌
RepositoryException.java	0%	❌

Repositories must not know about HTTP. Replace all ResponseStatusException throws in ChannelRepository, TagRepository, and PropertyRepository with StorageException (for ES IO failures) or the existing typed domain exceptions (ChannelValidationException for bad query windows). Add a StorageException handler to V0ExceptionHandler mapping it to 500. The NOT_FOUND status used in findById catch blocks was also wrong: those are IO/connection failures, not 404s - replaced with StorageException.

…itory boundaries Services should preferably not depend on Spring types. Replace MultiValueMap<String, String> with Map<String, List<String>> in all service method signatures and repository public APIs. Controllers pass their MultiValueMap directly — no conversion needed since MultiValueMap IS a Map<String, List<String>>. Also replaces internal LinkedMultiValueMap usages in TagRepository and PropertyRepository scroll loops, MetricsService metric-combination generation, ChannelProcessorService.processAllChannels, and ChannelFinderEpicsService with standard java.util equivalents.

@repository

@repository and @configuration must not be combined on the same class. The @configuration annotation was causing repositories to participate in Spring's configuration processing, which is incorrect. Remove it from all three repositories and drop the now-unused import.

Add ChannelDto, TagDto, PropertyDto, SearchResultDto, ScrollDto under web/v0/dto/ and corresponding static mappers under web/v0/mapper/. All five controllers now map to/from DTOs; the service and domain layers are unchanged. This isolates the v0 API shape from the domain model so a future v1 API can evolve its own DTOs without touching service or repository code.

…nd repository boundaries" This reverts commit 2def995.

simon-ess · 2026-05-21T14:24:59Z

I'd approve it but I do not seem to have permission to do so.

sonarqubecloud · 2026-05-21T14:36:11Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

github-actions · 2026-05-21T14:47:29Z

Overall Project	13.17% `-4.73%`	❌
Files changed	2.73%	❌

File	Coverage
ChannelService.java	54.3%	🍏
PropertyService.java	22.57% `-0.75%`	❌
PropertyRepository.java	0.56% `-5.89%`	❌
TagRepository.java	0.56% `-5.69%`	❌
ChannelRepository.java	0.23% `-5.06%`	❌
ScrollDto.java	0%	❌
PropertyDto.java	0%	❌
SearchResultDto.java	0%	❌
TagDto.java	0%	❌
ChannelDto.java	0%	❌
TagController.java	0% `-74.24%`	❌
ChannelController.java	0% `-74.7%`	❌
PropertyController.java	0% `-75%`	❌
ChannelProcessorController.java	0% `-13.89%`	❌
ChannelScrollController.java	0% `-81.25%`	❌
V0ExceptionHandler.java	0% `-9.22%`	❌
ChannelMapper.java	0%	❌
PropertyMapper.java	0%	❌
TagMapper.java	0%	❌
RepositoryException.java	0%	❌

anderslindho requested review from domonkos-ess, georgweiss, imretoth-ess, jacomago, shroffk, simon-ess and tynanford May 7, 2026 13:18

georgweiss reviewed May 7, 2026

View reviewed changes

Comment thread src/main/java/org/phoebus/channelfinder/repository/ChannelRepository.java Outdated

georgweiss reviewed May 7, 2026

View reviewed changes

anderslindho force-pushed the refactor/layer-isolation branch 3 times, most recently from 87d7588 to a6dbe48 Compare May 8, 2026 07:09

anderslindho marked this pull request as ready for review May 8, 2026 07:16

anderslindho marked this pull request as draft May 8, 2026 07:17

anderslindho force-pushed the refactor/layer-isolation branch 3 times, most recently from 575892c to 6c44109 Compare May 8, 2026 09:50

anderslindho marked this pull request as ready for review May 8, 2026 10:45

jacomago reviewed May 8, 2026

View reviewed changes

Comment thread src/main/java/org/phoebus/channelfinder/exceptions/StorageException.java Outdated

Comment thread src/main/java/org/phoebus/channelfinder/web/v0/mapper/ChannelMapper.java

Comment thread src/test/java/org/phoebus/channelfinder/ChannelControllerIT.java Outdated

anderslindho force-pushed the refactor/layer-isolation branch from 6c44109 to 85b3acb Compare May 21, 2026 10:19

simon-ess reviewed May 21, 2026

View reviewed changes

Comment thread src/main/java/org/phoebus/channelfinder/repository/PropertyRepository.java Outdated

anderslindho force-pushed the refactor/layer-isolation branch 2 times, most recently from 576f05e to f5e9f6f Compare May 21, 2026 12:22

anderslindho requested review from jacomago and simon-ess May 21, 2026 12:24

simon-ess reviewed May 21, 2026

View reviewed changes

Comment thread src/main/java/org/phoebus/channelfinder/repository/ChannelRepository.java Outdated

Comment thread src/main/java/org/phoebus/channelfinder/repository/ChannelRepository.java Outdated

Comment thread src/main/java/org/phoebus/channelfinder/repository/ChannelRepository.java Outdated

anderslindho force-pushed the refactor/layer-isolation branch from f5e9f6f to edebe70 Compare May 21, 2026 14:00

anderslindho added 5 commits May 21, 2026 16:22

Revert "refactor(service): remove Spring MultiValueMap from service a…

66d767a

…nd repository boundaries" This reverts commit 2def995.

anderslindho force-pushed the refactor/layer-isolation branch from edebe70 to 66d767a Compare May 21, 2026 14:35

jacomago approved these changes May 22, 2026

View reviewed changes

anderslindho merged commit 39ce8db into master May 22, 2026
8 checks passed

Conversation

anderslindho commented May 7, 2026

Uh oh!

Uh oh!

georgweiss left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 8, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented May 21, 2026

Uh oh!

Uh oh!

simon-ess commented May 21, 2026

Uh oh!

github-actions Bot commented May 21, 2026

Uh oh!

simon-ess left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented May 21, 2026

Uh oh!

simon-ess commented May 21, 2026

Uh oh!

sonarqubecloud Bot commented May 21, 2026

Quality Gate passed

Uh oh!

github-actions Bot commented May 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants