Skip to content

refactor: enforce clean layer separation across repository, service, and controller #216

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
anderslindho wants to merge 4 commits into
base: master
Choose a base branch
from
Open

refactor: enforce clean layer separation across repository, service, and controller #216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
68ad08e
fix(repository): replace ResponseStatusException with domain exceptions
anderslindho May 7, 2026
2def995
refactor(service): remove Spring MultiValueMap from service and repos…
anderslindho May 7, 2026
074f9d6
fix(repository): remove spurious @Configuration from repository classes
anderslindho May 7, 2026
6c44109
refactor(web/v0): introduce DTO layer at the controller boundary
anderslindho May 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/main/java/org/phoebus/channelfinder/exceptions/StorageException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package org.phoebus.channelfinder.exceptions;

public class StorageException extends RuntimeException {
Copy link
Copy Markdown
Contributor

@jacomago jacomago May 8, 2026

Choose a reason for hiding this comment

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

Feel like there should be a better name

RepositoryException?


public StorageException(String message) {
super(message);
}

public StorageException(String message, Throwable cause) {
super(message, cause);
}
}
56 changes: 22 additions & 34 deletions src/main/java/org/phoebus/channelfinder/repository/ChannelRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,20 +57,16 @@
import org.phoebus.channelfinder.entity.Scroll;
import org.phoebus.channelfinder.entity.SearchResult;
import org.phoebus.channelfinder.entity.Tag;
import org.phoebus.channelfinder.exceptions.ChannelValidationException;
import org.phoebus.channelfinder.exceptions.StorageException;
import org.springframework.beans.factory.annotation.Qualifier;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.context.annotation.Configuration;
import org.springframework.data.repository.CrudRepository;
import org.springframework.http.HttpStatus;
import org.springframework.stereotype.Repository;
import org.springframework.util.LinkedMultiValueMap;
import org.springframework.util.MultiValueMap;
import org.springframework.web.server.ResponseStatusException;

// Jackson 2 required by elasticsearch-java 8.x JacksonJsonpMapper — migrate with ES 9

@Repository
@Configuration
public class ChannelRepository implements CrudRepository<Channel, String> {

private static final Logger logger = Logger.getLogger(ChannelRepository.class.getName());
Expand Down Expand Up @@ -126,7 +122,7 @@ public Channel index(Channel channel) {
} catch (Exception e) {
String message = MessageFormat.format(TextUtil.FAILED_TO_INDEX_CHANNEL, channel.toLog());
logger.log(Level.SEVERE, message, e);
throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, message, null);
throw new StorageException(message, e);
}
return null;
}
Expand Down Expand Up @@ -176,8 +172,7 @@ channel, new JacksonJsonpMapper(objectMapper)))))
} catch (IOException e) {
String message = MessageFormat.format(TextUtil.FAILED_TO_INDEX_CHANNELS, chunk);
logger.log(Level.SEVERE, message, e);
throw new ResponseStatusException(
HttpStatus.INTERNAL_SERVER_ERROR, message, null);
throw new StorageException(message, e);
}
return Collections.emptyList();
}));
Expand Down Expand Up @@ -218,7 +213,7 @@ public Channel save(String channelName, Channel channel) {
} catch (Exception e) {
String message = MessageFormat.format(TextUtil.FAILED_TO_INDEX_CHANNEL, channel.toLog());
logger.log(Level.SEVERE, message, e);
throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, message, null);
throw new StorageException(message, e);
}
return null;
}
Expand Down Expand Up @@ -310,8 +305,7 @@ public <S extends Channel> Iterable<S> saveAll(Iterable<S> channels) {
String message =
MessageFormat.format(TextUtil.FAILED_TO_INDEX_CHANNELS, channels);
logger.log(Level.SEVERE, message, e);
throw new ResponseStatusException(
HttpStatus.INTERNAL_SERVER_ERROR, message, null);
throw new StorageException(message, e);
}
return Collections.emptyList();
}));
Expand Down Expand Up @@ -354,7 +348,7 @@ public Optional<Channel> findById(String channelName) {
} catch (ElasticsearchException | IOException e) {
String message = MessageFormat.format(TextUtil.FAILED_TO_FIND_CHANNEL, channelName);
logger.log(Level.SEVERE, message, e);
throw new ResponseStatusException(HttpStatus.NOT_FOUND, message, null);
throw new StorageException(message, e);
}
}

Expand Down Expand Up @@ -383,8 +377,7 @@ public boolean existsByIds(List<String> channelIds) {
.containsAll(channelIds);
} catch (ElasticsearchException | IOException e) {
logger.log(Level.SEVERE, TextUtil.FAILED_TO_FIND_ALL_CHANNELS, e);
throw new ResponseStatusException(
HttpStatus.INTERNAL_SERVER_ERROR, TextUtil.FAILED_TO_FIND_ALL_CHANNELS, null);
throw new StorageException(TextUtil.FAILED_TO_FIND_ALL_CHANNELS, e);
}
}

Expand All @@ -404,7 +397,7 @@ public boolean existsById(String channelName) {
String message =
MessageFormat.format(TextUtil.FAILED_TO_CHECK_IF_CHANNEL_EXISTS, channelName);
logger.log(Level.SEVERE, message, e);
throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, message, null);
throw new StorageException(message, e);
}
}

Expand Down Expand Up @@ -440,14 +433,13 @@ public List<Channel> findAllById(Iterable<String> channelIds) {
return response.hits().hits().stream().map(Hit::source).collect(Collectors.toList());
} catch (ElasticsearchException | IOException e) {
logger.log(Level.SEVERE, TextUtil.FAILED_TO_FIND_ALL_CHANNELS, e);
throw new ResponseStatusException(
HttpStatus.INTERNAL_SERVER_ERROR, TextUtil.FAILED_TO_FIND_ALL_CHANNELS, null);
throw new StorageException(TextUtil.FAILED_TO_FIND_ALL_CHANNELS, e);
}
}

@Override
public long count() {
return this.count(new LinkedMultiValueMap<>());
return this.count(Map.of());
}

/**
Expand All @@ -468,7 +460,7 @@ public void deleteById(String channelName) {
} catch (ElasticsearchException | IOException e) {
String message = MessageFormat.format(TextUtil.FAILED_TO_DELETE_CHANNEL, channelName);
logger.log(Level.SEVERE, message, e);
throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, message, null);
throw new StorageException(message, e);
}
}

Expand Down Expand Up @@ -516,7 +508,7 @@ public void deleteAll() {
* @param searchParameters channel search parameters
* @return matching channels
*/
public SearchResult search(MultiValueMap<String, String> searchParameters) {
public SearchResult search(Map<String, List<String>> searchParameters) {
Copy link
Copy Markdown
Collaborator

@georgweiss georgweiss May 7, 2026

Choose a reason for hiding this comment

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

Why replacing MultiValuedMap?

Copy link
Copy Markdown
Contributor Author

@anderslindho anderslindho May 7, 2026

Choose a reason for hiding this comment

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

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>.

Copy link
Copy Markdown
Collaborator

@georgweiss georgweiss May 7, 2026

Choose a reason for hiding this comment

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

MultiValuedMap is a class in spring framework core, not in a web library.

Copy link
Copy Markdown
Contributor Author

@anderslindho anderslindho May 8, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator

@georgweiss georgweiss May 8, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@anderslindho anderslindho May 8, 2026

Choose a reason for hiding this comment

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

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.

BuiltQuery builtQuery = getBuiltQuery(searchParameters);
Integer finalSize = builtQuery.size;
Integer finalFrom = builtQuery.from;
Expand All @@ -527,7 +519,7 @@ public SearchResult search(MultiValueMap<String, String> searchParameters) {
TextUtil.SEARCH_FAILED_CAUSE,
searchParameters,
"Max search window exceeded, use the " + scrollResourceUri + " api.");
throw new ResponseStatusException(HttpStatus.BAD_REQUEST, message);
throw new ChannelValidationException(message);
}

try {
Expand All @@ -554,11 +546,11 @@ public SearchResult search(MultiValueMap<String, String> searchParameters) {
String message =
MessageFormat.format(TextUtil.SEARCH_FAILED_CAUSE, searchParameters, e.getMessage());
logger.log(Level.SEVERE, message, e);
throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, message, e);
throw new StorageException(message, e);
}
}

private BuiltQuery getBuiltQuery(MultiValueMap<String, String> searchParameters) {
private BuiltQuery getBuiltQuery(Map<String, List<String>> searchParameters) {
BoolQuery.Builder boolQuery = new BoolQuery.Builder();
int size = esService.getES_QUERY_SIZE();
int from = 0;
Expand Down Expand Up @@ -726,7 +718,7 @@ private record BuiltQuery(
* @param searchParameters channel search parameters
* @return count of the number of matches to the provided query
*/
public long count(MultiValueMap<String, String> searchParameters) {
public long count(Map<String, List<String>> searchParameters) {
BuiltQuery builtQuery = getBuiltQuery(searchParameters);

try {
Expand All @@ -742,7 +734,7 @@ public long count(MultiValueMap<String, String> searchParameters) {
String message =
MessageFormat.format(TextUtil.COUNT_FAILED_CAUSE, searchParameters, e.getMessage());
logger.log(Level.SEVERE, message, e);
throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, message, e);
throw new StorageException(message, e);
}
}

Expand All @@ -754,9 +746,7 @@ public long count(MultiValueMap<String, String> searchParameters) {
* @return count of the number of matches to the provided query
*/
public long countByProperty(String propertyName, String propertyValue) {
MultiValueMap<String, String> params = new LinkedMultiValueMap<>();
params.add(propertyName, propertyValue == null ? "*" : propertyValue);
return this.count(params);
return this.count(Map.of(propertyName, List.of(propertyValue == null ? "*" : propertyValue)));
}

/**
Expand All @@ -766,9 +756,7 @@ public long countByProperty(String propertyName, String propertyValue) {
* @return count of the number of matches to the provided query
*/
public long countByTag(String tagName) {
MultiValueMap<String, String> params = new LinkedMultiValueMap<>();
params.add("~tag", tagName);
return this.count(params);
return this.count(Map.of("~tag", List.of(tagName)));
}

/**
Expand All @@ -779,7 +767,7 @@ public long countByTag(String tagName) {
* @param searchParameters channel search parameters
* @return next page with its cursor
*/
public Scroll scroll(String scrollId, MultiValueMap<String, String> searchParameters) {
public Scroll scroll(String scrollId, Map<String, List<String>> searchParameters) {
BuiltQuery builtQuery = getBuiltQuery(searchParameters);
try {
SearchRequest.Builder builder =
Expand All @@ -800,7 +788,7 @@ public Scroll scroll(String scrollId, MultiValueMap<String, String> searchParame
} catch (IOException | ElasticsearchException e) {
String message =
MessageFormat.format(TextUtil.SEARCH_FAILED_CAUSE, searchParameters, e.getMessage());
throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, message, e);
throw new StorageException(message, e);
}
}

Expand Down
42 changes: 18 additions & 24 deletions src/main/java/org/phoebus/channelfinder/repository/PropertyRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@
import com.fasterxml.jackson.databind.ObjectMapper;
import java.io.IOException;
import java.text.MessageFormat;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.logging.Level;
import java.util.logging.Logger;
Expand All @@ -36,20 +38,15 @@
import org.phoebus.channelfinder.entity.Channel;
import org.phoebus.channelfinder.entity.Property;
import org.phoebus.channelfinder.entity.Property.OnlyNameOwnerProperty;
import org.phoebus.channelfinder.exceptions.StorageException;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Qualifier;
import org.springframework.context.annotation.Configuration;
import org.springframework.data.repository.CrudRepository;
import org.springframework.http.HttpStatus;
import org.springframework.stereotype.Repository;
import org.springframework.util.LinkedMultiValueMap;
import org.springframework.util.MultiValueMap;
import org.springframework.web.server.ResponseStatusException;

// Jackson 2 required by elasticsearch-java 8.x JacksonJsonpMapper — migrate with ES 9

@Repository
@Configuration
public class PropertyRepository implements CrudRepository<Property, String> {

private static final Logger logger = Logger.getLogger(PropertyRepository.class.getName());
Expand Down Expand Up @@ -108,7 +105,7 @@ public List<Property> indexAll(List<Property> properties) {
} catch (IOException e) {
String message = MessageFormat.format(TextUtil.FAILED_TO_INDEX_PROPERTIES, properties);
logger.log(Level.SEVERE, message, e);
throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, message, null);
throw new StorageException(message);
}
return null;
}
Expand Down Expand Up @@ -142,7 +139,7 @@ public <S extends Property> S save(String propertyName, S property) {
} catch (Exception e) {
String message = MessageFormat.format(TextUtil.FAILED_TO_INDEX_PROPERTY, property.toLog());
logger.log(Level.SEVERE, message, e);
throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, message, null);
throw new StorageException(message);
}
return null;
}
Expand Down Expand Up @@ -197,7 +194,7 @@ public <S extends Property> Iterable<S> saveAll(Iterable<S> properties) {
} catch (IOException e) {
String message = MessageFormat.format(TextUtil.FAILED_TO_UPDATE_SAVE_PROPERTIES, properties);
logger.log(Level.SEVERE, message, e);
throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, message, null);
throw new StorageException(message);
}
return null;
}
Expand Down Expand Up @@ -232,9 +229,8 @@ public Optional<Property> findById(String propertyName, boolean withChannels) {
logger.log(
Level.CONFIG, () -> MessageFormat.format(TextUtil.PROPERTY_FOUND, property.getName()));
if (withChannels) {
MultiValueMap<String, String> params = new LinkedMultiValueMap<>();
params.add(property.getName(), "*");
property.setChannels(channelRepository.search(params).channels());
property.setChannels(
channelRepository.search(Map.of(property.getName(), List.of("*"))).channels());
}
return Optional.of(property);
} else {
Expand All @@ -245,7 +241,7 @@ public Optional<Property> findById(String propertyName, boolean withChannels) {
} catch (ElasticsearchException | IOException e) {
String message = MessageFormat.format(TextUtil.FAILED_TO_FIND_PROPERTY, propertyName);
logger.log(Level.SEVERE, message, e);
throw new ResponseStatusException(HttpStatus.NOT_FOUND, message, null);
throw new StorageException(message);
}
}

Expand All @@ -258,7 +254,7 @@ public boolean existsById(String id) {
} catch (ElasticsearchException | IOException e) {
String message = MessageFormat.format(TextUtil.FAILED_TO_CHECK_IF_PROPERTY_EXISTS, id);
logger.log(Level.SEVERE, message, e);
throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, message, null);
throw new StorageException(message);
}
}

Expand All @@ -280,8 +276,7 @@ public Iterable<Property> findAll() {
return response.hits().hits().stream().map(Hit::source).toList();
} catch (ElasticsearchException | IOException e) {
logger.log(Level.SEVERE, TextUtil.FAILED_TO_FIND_ALL_PROPERTIES, e);
throw new ResponseStatusException(
HttpStatus.INTERNAL_SERVER_ERROR, TextUtil.FAILED_TO_FIND_ALL_PROPERTIES, null);
throw new StorageException(TextUtil.FAILED_TO_FIND_ALL_PROPERTIES);
}
}

Expand All @@ -306,8 +301,7 @@ public List<Property> findAllById(Iterable<String> propertyIds) {
return response.hits().hits().stream().map(Hit::source).toList();
} catch (ElasticsearchException | IOException e) {
logger.log(Level.SEVERE, TextUtil.FAILED_TO_FIND_ALL_PROPERTIES, e);
throw new ResponseStatusException(
HttpStatus.INTERNAL_SERVER_ERROR, TextUtil.FAILED_TO_FIND_ALL_PROPERTIES, null);
throw new StorageException(TextUtil.FAILED_TO_FIND_ALL_PROPERTIES);
}
}

Expand All @@ -322,7 +316,7 @@ public long count() {

String message = MessageFormat.format(TextUtil.COUNT_FAILED_CAUSE, "", e.getMessage());
logger.log(Level.SEVERE, message, e);
throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, message, e);
throw new StorageException(message, e);
}
}

Expand All @@ -346,8 +340,8 @@ public void deleteById(String propertyName) {

// Remove the Property from Channels
BulkRequest.Builder br = new BulkRequest.Builder().refresh(Refresh.True);
MultiValueMap<String, String> params = new LinkedMultiValueMap<>();
params.add(propertyName, "*");
Map<String, List<String>> params = new LinkedHashMap<>();
params.put(propertyName, List.of("*"));
List<Channel> channels = channelRepository.search(params).channels();
while (channels.size() > 0) {
for (Channel channel : channels) {
Expand Down Expand Up @@ -380,15 +374,15 @@ public void deleteById(String propertyName) {
} catch (IOException e) {
String message = MessageFormat.format(TextUtil.FAILED_TO_DELETE_PROPERTY, propertyName);
logger.log(Level.SEVERE, message, e);
throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, message, null);
throw new StorageException(message);
}
params.set("~search_after", channels.get(channels.size() - 1).getName());
params.put("~search_after", List.of(channels.get(channels.size() - 1).getName()));
channels = channelRepository.search(params).channels();
}
} catch (ElasticsearchException | IOException e) {
String message = MessageFormat.format(TextUtil.FAILED_TO_DELETE_PROPERTY, propertyName);
logger.log(Level.SEVERE, message, e);
throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, message, null);
throw new StorageException(message);
}
}

Expand Down
Loading
Loading