refactor: enforce clean layer separation across repository, service, and controller #216
refactor: enforce clean layer separation across repository, service, and controller #216anderslindho wants to merge 4 commits intomasterfrom
Conversation
| * @return matching channels | ||
| */ | ||
| public SearchResult search(MultiValueMap<String, String> searchParameters) { | ||
| public SearchResult search(Map<String, List<String>> searchParameters) { |
There was a problem hiding this comment.
Why replacing MultiValuedMap?
There was a problem hiding this comment.
See commit message for that change:
Services must not depend on Spring web binding types. Replace
MultiValueMap<String, String> with Map<String, List> in all
service method signatures and repository public APIs. Controllers pass
their MultiValueMap directly - no conversion needed since MultiValueMap
IS-A Map<String, List>.
There was a problem hiding this comment.
MultiValuedMap is a class in spring framework core, not in a web library.
There was a problem hiding this comment.
Good catch, misleading commit message. I still think it is an improvement as it is pure Java, which I think generally is preferably for repo and service layers. I will fix the commit message for now.
There was a problem hiding this comment.
Disagree. Why should a service layer be "pure Java"? Spring offers a lot of APIs that make perfect sense in a service layer. Or: it would be quite limiting to restrict a service layer to only JDK APIs.
There was a problem hiding this comment.
Fair point. To be more clear, I the benefit is that the signature is not tied to a Spring interface - even if Spring is used elsewhere in the same (or lower) layer(s) for other reasons. And I guess the counter-argument is that MultiValueMap communicates intent more explicitly and is available either way.
TBH I have no strong feelings about this particular change, so I can drop it if you prefer it one way over the other.
georgweiss
left a comment
There was a problem hiding this comment.
Added in-line comment
87d7588 to
a6dbe48
Compare
ce722ea to
575892c
Compare
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 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.
575892c to
6c44109
Compare
|
|
| @@ -0,0 +1,12 @@ | |||
| package org.phoebus.channelfinder.exceptions; | |||
|
|
|||
| public class StorageException extends RuntimeException { | |||
There was a problem hiding this comment.
Feel like there should be a better name
RepositoryException?
| import org.phoebus.channelfinder.entity.Channel; | ||
| import org.phoebus.channelfinder.web.v0.dto.ChannelDto; | ||
|
|
||
| public final class ChannelMapper { |
There was a problem hiding this comment.
Why do we need a mapper class? Can we not do the mapping on the types themselves?
i.e.
Channel.toDto
ChannelDto.toChannel
?
Or the otherway round
Channel.fromDto
ChannelDto.fromChannel
There was a problem hiding this comment.
Probably because my mind was on MapStruct and I did not think of that 🫣
Channel.toDto sounds wrong - domain layer would depend on controller layer which is the wrong way around. So it'd have to be more like
ChannelDto.from
ChannelDto.toDomain
But it's not immediately clear to me if this will fit later on with v1 API. And it's to prep for that that I set up these (currently unnecessary) mappers in the first place.
There was a problem hiding this comment.
I think I would rather the conversion happens inside of the service layer, since that is the mapping from controller layer to repository.
| void tearDown() throws IOException { | ||
| ElasticConfigIT.teardown(esService); | ||
| } | ||
|
|
There was a problem hiding this comment.
These methods feel kind of weird to be in test file.
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.