From b4694ca985481046081d58bffaf63b1cf82a836e Mon Sep 17 00:00:00 2001 From: Joris Borgdorff Date: Thu, 15 Jun 2023 13:55:56 +0200 Subject: [PATCH 1/7] Cache questionnaires and standardize github client access - Use GithubClient inside GithubProtocolFetcherStrategy to improve readability and remove deplication. - Use GithubService as a proxy for GithubClient in endpoint. This caches any values from the questionnaire. - Created FunctionCache that caches the result of a throwing function for a given amount of time. - Added more checks on the request being performed: number of bytes for content length, more strict URL validation (only https://api.github.com is allowed now instead of for example http://api.github.com.my.domain. --- .../resources/docker/docker-compose.yml | 2 +- .../appserver/controller/GithubEndpoint.java | 15 +- .../org/radarbase/appserver/entity/User.java | 31 ++- .../appserver/service/GithubClient.java | 67 +++++-- .../appserver/service/GithubService.java | 33 ++++ .../GithubProtocolFetcherStrategy.java | 185 +++++++----------- .../appserver/util/FunctionCache.java | 104 ++++++++++ 7 files changed, 292 insertions(+), 145 deletions(-) create mode 100644 src/main/java/org/radarbase/appserver/service/GithubService.java create mode 100644 src/main/java/org/radarbase/appserver/util/FunctionCache.java diff --git a/src/integrationTest/resources/docker/docker-compose.yml b/src/integrationTest/resources/docker/docker-compose.yml index 916e89b4..dfb5159e 100644 --- a/src/integrationTest/resources/docker/docker-compose.yml +++ b/src/integrationTest/resources/docker/docker-compose.yml @@ -64,7 +64,7 @@ services: # Management Portal # #---------------------------------------------------------------------------# managementportal: - image: radarbase/management-portal:0.5.6 + image: radarbase/management-portal:2.0.0 ports: - "8081:8081" environment: diff --git a/src/main/java/org/radarbase/appserver/controller/GithubEndpoint.java b/src/main/java/org/radarbase/appserver/controller/GithubEndpoint.java index fc164158..33ff4b9e 100644 --- a/src/main/java/org/radarbase/appserver/controller/GithubEndpoint.java +++ b/src/main/java/org/radarbase/appserver/controller/GithubEndpoint.java @@ -22,7 +22,7 @@ package org.radarbase.appserver.controller; import org.radarbase.appserver.config.AuthConfig; -import org.radarbase.appserver.service.GithubClient; +import org.radarbase.appserver.service.GithubService; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; @@ -31,17 +31,16 @@ import org.springframework.web.bind.annotation.RestController; import radar.spring.auth.common.Authorized; -import java.io.IOException; import java.net.MalformedURLException; @RestController public class GithubEndpoint { - private transient GithubClient githubClient; + private final transient GithubService githubService; @Autowired - public GithubEndpoint(GithubClient githubClient) { - this.githubClient = githubClient; + public GithubEndpoint(GithubService githubService) { + this.githubService = githubService; } @Authorized( @@ -51,13 +50,13 @@ public GithubEndpoint(GithubClient githubClient) { PathsUtil.GITHUB_PATH + "/" + PathsUtil.GITHUB_CONTENT_PATH) - public ResponseEntity getGithubContent(@RequestParam() String url + public ResponseEntity getGithubContent(@RequestParam() String url ) { try { - return ResponseEntity.ok().body(this.githubClient.getGithubContent(url)); + return ResponseEntity.ok().body(this.githubService.getGithubContent(url)); } catch (MalformedURLException e) { return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(e.getMessage()); - } catch (IOException | InterruptedException e) { + } catch (Exception e) { return ResponseEntity.status(HttpStatus.BAD_GATEWAY).body("Error getting content from Github."); } } diff --git a/src/main/java/org/radarbase/appserver/entity/User.java b/src/main/java/org/radarbase/appserver/entity/User.java index 226b3dea..f59f95a5 100644 --- a/src/main/java/org/radarbase/appserver/entity/User.java +++ b/src/main/java/org/radarbase/appserver/entity/User.java @@ -22,16 +22,23 @@ package org.radarbase.appserver.entity; import com.fasterxml.jackson.annotation.JsonIgnore; - -import java.io.Serializable; -import java.time.Instant; -import java.util.HashMap; -import java.util.Map; -import java.util.Objects; -import jakarta.persistence.*; +import jakarta.persistence.CascadeType; +import jakarta.persistence.CollectionTable; +import jakarta.persistence.Column; +import jakarta.persistence.ElementCollection; +import jakarta.persistence.Entity; +import jakarta.persistence.FetchType; +import jakarta.persistence.GeneratedValue; +import jakarta.persistence.GenerationType; +import jakarta.persistence.Id; +import jakarta.persistence.JoinColumn; +import jakarta.persistence.ManyToOne; +import jakarta.persistence.MapKeyColumn; +import jakarta.persistence.OneToOne; +import jakarta.persistence.Table; +import jakarta.persistence.UniqueConstraint; import jakarta.validation.constraints.NotEmpty; import jakarta.validation.constraints.NotNull; - import lombok.Getter; import lombok.ToString; import org.hibernate.annotations.OnDelete; @@ -39,6 +46,12 @@ import org.radarbase.appserver.dto.fcm.FcmUserDto; import org.springframework.lang.Nullable; +import java.io.Serializable; +import java.time.Instant; +import java.util.HashMap; +import java.util.Map; +import java.util.Objects; + /** * {@link Entity} for persisting users. The corresponding DTO is {@link FcmUserDto}. A {@link * Project} can have multiple {@link User} (Many-to-One). @@ -97,7 +110,7 @@ public class User extends AuditModel implements Serializable { @CollectionTable(name = "attributes_map") @MapKeyColumn(name = "key", nullable = true) @Column(name = "value") - private Map attributes = new HashMap(); + private Map attributes = new HashMap<>(); public User setSubjectId(String subjectId) { this.subjectId = subjectId; diff --git a/src/main/java/org/radarbase/appserver/service/GithubClient.java b/src/main/java/org/radarbase/appserver/service/GithubClient.java index 2c49a3a1..2db2a4fe 100644 --- a/src/main/java/org/radarbase/appserver/service/GithubClient.java +++ b/src/main/java/org/radarbase/appserver/service/GithubClient.java @@ -21,7 +21,6 @@ package org.radarbase.appserver.service; -import com.fasterxml.jackson.databind.ObjectMapper; import lombok.SneakyThrows; import lombok.extern.slf4j.Slf4j; import org.springframework.beans.factory.annotation.Autowired; @@ -33,6 +32,7 @@ import org.springframework.web.server.ResponseStatusException; import java.io.IOException; +import java.io.InputStream; import java.net.MalformedURLException; import java.net.URI; import java.net.http.HttpClient; @@ -44,25 +44,26 @@ @Component @Scope(value = ConfigurableBeanFactory.SCOPE_SINGLETON) public class GithubClient { - private static final String GITHUB_API_URI = "api.github.com"; private static final String GITHUB_API_ACCEPT_HEADER = "application/vnd.github.v3+json"; - private static final String LOCATION_HEADER = "location"; - private final transient ObjectMapper objectMapper; private final transient HttpClient client; @Value("${security.github.client.token}") private transient String githubToken; + @Value("${security.github.client.timeout:PT10s}") + private transient Duration httpTimeout; + + @Value("${security.github.client.maxContentLength:1000000}") + private transient int maxContentLength; + @SneakyThrows @Autowired - public GithubClient(ObjectMapper objectMapper) { - this.objectMapper = objectMapper; - client = HttpClient.newBuilder().followRedirects(HttpClient.Redirect.NORMAL).connectTimeout(Duration.ofSeconds(10)).build(); - } - - private static boolean isSuccessfulResponse(HttpResponse response) { - return response.statusCode() >= 200 && response.statusCode() < 300; + public GithubClient() { + client = HttpClient.newBuilder() + .followRedirects(HttpClient.Redirect.NORMAL) + .connectTimeout(httpTimeout) + .build(); } public String getGithubContent(String url) throws IOException, InterruptedException { @@ -70,9 +71,22 @@ public String getGithubContent(String url) throws IOException, InterruptedExcept if (!this.isValidGithubUri(uri)) { throw new MalformedURLException("Invalid Github url."); } - HttpResponse response = client.send(getRequest(uri), HttpResponse.BodyHandlers.ofString()); - if (isSuccessfulResponse(response)) { - return response.body().toString(); + HttpResponse response; + try { + response = client.send(getRequest(uri), HttpResponse.BodyHandlers.ofInputStream()); + } catch (IOException ex) { + log.error("Failed to retrieve data from github: {}", ex.toString()); + throw new ResponseStatusException(HttpStatus.BAD_GATEWAY, "Github responded with an error."); + } + + if (response.statusCode() >= 200 && response.statusCode() < 300) { + checkContentLengthHeader(response); + + try (InputStream inputStream = response.body()) { + byte[] bytes = inputStream.readNBytes(maxContentLength + 1); + checkContentLength(bytes.length); + return new String(bytes); + } } else { log.error("Error getting Github content from URL {} : {}", url, response); throw new ResponseStatusException( @@ -80,15 +94,36 @@ public String getGithubContent(String url) throws IOException, InterruptedExcept } } + private void checkContentLengthHeader(HttpResponse response) { + response.headers().firstValue("Content-Length") + .map((l) -> { + try { + return Integer.valueOf(l); + } catch (NumberFormatException ex) { + return null; + } + }) + .ifPresent(this::checkContentLength); + } + + private void checkContentLength(int contentLength) { + if (contentLength > maxContentLength) { + throw new ResponseStatusException( + HttpStatus.BAD_REQUEST, "Github content is too large"); + } + } + public boolean isValidGithubUri(URI uri) { - return uri.getHost().contains(GITHUB_API_URI); + return uri.getHost().equalsIgnoreCase(GITHUB_API_URI) + && uri.getScheme().equalsIgnoreCase("https") + && (uri.getPort() == -1 || uri.getPort() == 443); } private HttpRequest getRequest(URI uri) { HttpRequest.Builder request = HttpRequest.newBuilder(uri) .header("Accept", GITHUB_API_ACCEPT_HEADER) .GET() - .timeout(Duration.ofSeconds(10)); + .timeout(httpTimeout); if (githubToken != null && !githubToken.isEmpty()) { request.header("Authorization", "Bearer " + githubToken); } diff --git a/src/main/java/org/radarbase/appserver/service/GithubService.java b/src/main/java/org/radarbase/appserver/service/GithubService.java new file mode 100644 index 00000000..2063bb2a --- /dev/null +++ b/src/main/java/org/radarbase/appserver/service/GithubService.java @@ -0,0 +1,33 @@ +package org.radarbase.appserver.service; + +import org.radarbase.appserver.util.FunctionCache; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.beans.factory.config.ConfigurableBeanFactory; +import org.springframework.context.annotation.Scope; +import org.springframework.stereotype.Component; + +import java.time.Duration; + +@Component +@Scope(value = ConfigurableBeanFactory.SCOPE_SINGLETON) +public class GithubService { + + private final transient FunctionCache cache; + + @Autowired + public GithubService( + GithubClient githubClient, + @Value("${security.github.service.cacheDuration:PT1H}") + Duration cacheTime, + @Value("${security.github.service.retryDuration:PT1M}") + Duration retryTime, + @Value("${security.github.service.cacheSize:10000}") + int maxSize) { + this.cache = new FunctionCache<>(githubClient::getGithubContent, cacheTime, retryTime, maxSize); + } + + public String getGithubContent(String url) throws Exception { + return this.cache.getOrThrow(url); + } +} diff --git a/src/main/java/org/radarbase/appserver/service/questionnaire/protocol/GithubProtocolFetcherStrategy.java b/src/main/java/org/radarbase/appserver/service/questionnaire/protocol/GithubProtocolFetcherStrategy.java index 8779728e..1a476b63 100644 --- a/src/main/java/org/radarbase/appserver/service/questionnaire/protocol/GithubProtocolFetcherStrategy.java +++ b/src/main/java/org/radarbase/appserver/service/questionnaire/protocol/GithubProtocolFetcherStrategy.java @@ -25,16 +25,6 @@ import com.fasterxml.jackson.databind.DeserializationFeature; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; - -import java.io.IOException; -import java.net.URI; -import java.net.http.HttpClient; -import java.net.http.HttpRequest; -import java.net.http.HttpResponse; -import java.time.Duration; -import java.util.*; -import java.util.stream.Collectors; - import com.fasterxml.jackson.databind.node.ObjectNode; import com.google.common.collect.Maps; import lombok.SneakyThrows; @@ -43,7 +33,6 @@ import org.radarbase.appserver.dto.protocol.GithubContent; import org.radarbase.appserver.dto.protocol.Protocol; import org.radarbase.appserver.dto.protocol.ProtocolCacheEntry; -import org.radarbase.appserver.entity.Project; import org.radarbase.appserver.entity.User; import org.radarbase.appserver.repository.ProjectRepository; import org.radarbase.appserver.repository.UserRepository; @@ -53,10 +42,20 @@ import org.springframework.beans.factory.annotation.Value; import org.springframework.beans.factory.config.ConfigurableBeanFactory; import org.springframework.context.annotation.Scope; -import org.springframework.http.HttpStatus; import org.springframework.stereotype.Component; import org.springframework.web.server.ResponseStatusException; +import java.io.IOException; +import java.net.URI; +import java.time.Duration; +import java.util.Collections; +import java.util.Comparator; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; + @Slf4j @Component @Scope(value = ConfigurableBeanFactory.SCOPE_SINGLETON) @@ -66,7 +65,6 @@ public class GithubProtocolFetcherStrategy implements ProtocolFetcherStrategy { private final transient ProjectRepository projectRepository; private static final String GITHUB_API_URI = "https://api.github.com/repos/"; - private static final String GITHUB_API_ACCEPT_HEADER = "application/vnd.github.v3+json"; private final transient String protocolRepo; private final transient String protocolFileName; private final transient String protocolBranch; @@ -74,13 +72,8 @@ public class GithubProtocolFetcherStrategy implements ProtocolFetcherStrategy { private final transient ObjectMapper localMapper; // Keeps a cache of github URI's associated with protocol for each project private final transient CachedMap projectProtocolUriMap; - private final transient HttpClient client; - private final transient GithubClient githubClient; - @Value("${security.github.client.token}") - private transient String githubToken; - @SneakyThrows @Autowired public GithubProtocolFetcherStrategy( @@ -106,100 +99,96 @@ public GithubProtocolFetcherStrategy( this.objectMapper = objectMapper; this.localMapper = this.objectMapper.copy(); this.localMapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false); - client = HttpClient.newBuilder().connectTimeout(Duration.ofSeconds(10)).build(); this.userRepository = userRepository; this.projectRepository = projectRepository; this.githubClient = githubClient; } - private static boolean isSuccessfulResponse(HttpResponse response) { - return response.statusCode() >= 200 && response.statusCode() < 300; - } - @Override - public synchronized Map fetchProtocols() throws IOException { - Map subjectProtocolMap = new HashMap<>(); + public synchronized Map fetchProtocols() { List users = this.userRepository.findAll(); - Map protocolUriMap; - try { - protocolUriMap = projectProtocolUriMap.get(); - } catch (IOException e) { - // Failed to get the Uri Map. try using the cached value - protocolUriMap = projectProtocolUriMap.getCache(); - } - - if (protocolUriMap == null) { - return subjectProtocolMap; + Set protocolPaths = getProtocolPaths(); + if (protocolPaths == null) { + return Map.of(); } - Set protocolPaths = protocolUriMap.keySet(); - subjectProtocolMap = users.parallelStream() - .map(u -> { - ProtocolCacheEntry entry = this.fetchProtocolForSingleUser(u, u.getProject().getProjectId(), protocolPaths); - return entry; - }) + Map subjectProtocolMap = users.parallelStream() + .map(u -> this.fetchProtocolForSingleUser(u, u.getProject().getProjectId(), protocolPaths)) .filter(c -> c.getProtocol() != null) - .collect(Collectors.toMap(p -> p.getId(), p -> p.getProtocol())); + .collect(Collectors.toMap(ProtocolCacheEntry::getId, ProtocolCacheEntry::getProtocol)); log.info("Refreshed Protocols from Github"); return subjectProtocolMap; } private ProtocolCacheEntry fetchProtocolForSingleUser(User u, String projectId, Set protocolPaths) { - Map attributes = u.getAttributes(); - Map pathMap = protocolPaths.stream().filter(k -> k.contains(projectId)) + Map attributes = u.getAttributes() != null ? u.getAttributes() : Map.of(); + Map pathMap = protocolPaths.stream() + .filter(k -> k.contains(projectId)) .map(p -> { Map path = this.convertPathToAttributeMap(p, projectId); return Maps.difference(attributes, path).entriesInCommon(); - }).max(Comparator.comparingInt(Map::size)).orElse(Collections.emptyMap()); + }) + .max(Comparator.comparingInt(Map::size)) + .orElse(Collections.emptyMap()); + try { String attributePath = this.convertAttributeMapToPath(pathMap, projectId); if (projectProtocolUriMap.get().containsKey(attributePath)) { URI uri = projectProtocolUriMap.get(attributePath); return new ProtocolCacheEntry(u.getSubjectId(), getProtocolFromUrl(uri)); + } else { + return new ProtocolCacheEntry(u.getSubjectId(), null); } - return new ProtocolCacheEntry(u.getSubjectId(), null); } catch (IOException | InterruptedException | ResponseStatusException e) { return new ProtocolCacheEntry(u.getSubjectId(), null); } } @Override - public synchronized Map fetchProtocolsPerProject() throws IOException { - Map projectProtocolMap = new HashMap<>(); - List projects = this.projectRepository.findAll(); + public synchronized Map fetchProtocolsPerProject() { + Set protocolPaths = getProtocolPaths(); - Map protocolUriMap; - try { - protocolUriMap = projectProtocolUriMap.get(); - } catch (IOException e) { - // Failed to get the Uri Map. try using the cached value - protocolUriMap = projectProtocolUriMap.getCache(); + if (protocolPaths == null) { + return Map.of(); } - if (protocolUriMap == null) { - return projectProtocolMap; - } - - Set protocolPaths = protocolUriMap.keySet(); - projectProtocolMap = projects.parallelStream() + Map projectProtocolMap = projectRepository.findAll() + .parallelStream() .map(project -> { String projectId = project.getProjectId(); - String path = protocolPaths.stream().filter(k -> k.contains(projectId)).findFirst().get(); - try { - URI uri = projectProtocolUriMap.get(path); - Protocol protocol = getProtocolFromUrl(uri); - return new ProtocolCacheEntry(projectId, protocol); - } catch (IOException | InterruptedException | ResponseStatusException e) { - return new ProtocolCacheEntry(projectId, null); - } - }).collect(Collectors.toMap(p -> p.getId(), p -> p.getProtocol())); + Protocol protocol = protocolPaths.stream() + .filter(k -> k.contains(projectId)) + .findFirst() + .map(path -> { + try { + URI uri = projectProtocolUriMap.get(path); + return getProtocolFromUrl(uri); + } catch (IOException | InterruptedException + | ResponseStatusException e) { + return null; + } + }).orElse(null); + return new ProtocolCacheEntry(projectId, protocol); + }) + .collect(Collectors.toMap(ProtocolCacheEntry::getId, ProtocolCacheEntry::getProtocol)); log.info("Refreshed Protocols from Github"); return projectProtocolMap; } + private Set getProtocolPaths() { + Map uriMap; + try { + uriMap = projectProtocolUriMap.get(); + } catch (IOException e) { + // Failed to get the Uri Map. try using the cached value + uriMap = projectProtocolUriMap.getCache(); + } + return uriMap != null ? uriMap.keySet() : null; + } + public Map convertPathToAttributeMap(String path, String projectId) { String[] parts = path.split("/"); String key = ""; @@ -232,36 +221,20 @@ private Map getProtocolDirectories() throws IOException { Map protocolUriMap = new HashMap<>(); try { - HttpResponse response = - client.send( - getRequest( - URI.create(GITHUB_API_URI + protocolRepo + "/branches/" + protocolBranch)), - HttpResponse.BodyHandlers.ofString()); - if (isSuccessfulResponse(response)) { - ObjectNode result = getArrayNode(response.body().toString()); - String treeSha = result.findValue("tree").findValue("sha").asText(); - URI treeUri = URI.create(GITHUB_API_URI + protocolRepo + "/git/trees/" + treeSha + "?recursive=true"); - HttpResponse treeResponse = client.send(getRequest(treeUri), HttpResponse.BodyHandlers.ofString()); - - if (isSuccessfulResponse(treeResponse)) { - JsonNode tree = getArrayNode(treeResponse.body().toString()).get("tree"); - for (JsonNode jsonNode : tree) { - String path = jsonNode.get("path").asText(); - if (path.contains(this.protocolFileName)) { - protocolUriMap.put( - path, - URI.create(jsonNode.get("url").asText())); - } - } - } - } - else { - log.warn("Failed to retrieve protocols URIs from github: {}.", response); - throw new ResponseStatusException( - HttpStatus.valueOf(response.statusCode()), - "Failed to retrieve protocols URIs from github."); + String content = githubClient.getGithubContent(GITHUB_API_URI + protocolRepo + "/branches/" + protocolBranch); + ObjectNode result = getArrayNode(content); + String treeSha = result.findValue("tree").findValue("sha").asText(); + String treeContent = githubClient.getGithubContent(GITHUB_API_URI + protocolRepo + "/git/trees/" + treeSha + "?recursive=true"); + + JsonNode tree = getArrayNode(treeContent).get("tree"); + for (JsonNode jsonNode : tree) { + String path = jsonNode.get("path").asText(); + if (path.contains(this.protocolFileName)) { + protocolUriMap.put( + path, + URI.create(jsonNode.get("url").asText())); + } } - } catch (InterruptedException | ResponseStatusException e) { throw new IOException("Failed to retrieve protocols URIs from github", e); } @@ -269,25 +242,15 @@ private Map getProtocolDirectories() throws IOException { } private Protocol getProtocolFromUrl(URI uri) throws IOException, InterruptedException { - String contentString = this.githubClient.getGithubContent(uri.toString()); + String contentString = githubClient.getGithubContent(uri.toString()); GithubContent content = localMapper.readValue(contentString, GithubContent.class); return localMapper.readValue(content.getContent(), Protocol.class); } - private HttpRequest getRequest(URI uri) { - HttpRequest.Builder request = HttpRequest.newBuilder(uri) - .header("Accept", GITHUB_API_ACCEPT_HEADER) - .header("Authorization", "Bearer " + this.githubToken) - .GET() - .timeout(Duration.ofSeconds(10)); - - return request.build(); - } - @SneakyThrows private ObjectNode getArrayNode(String json) { try (JsonParser parserProtocol = objectMapper.getFactory().createParser(json)) { return objectMapper.readTree(parserProtocol); - } + } } } diff --git a/src/main/java/org/radarbase/appserver/util/FunctionCache.java b/src/main/java/org/radarbase/appserver/util/FunctionCache.java new file mode 100644 index 00000000..fd8bfd5d --- /dev/null +++ b/src/main/java/org/radarbase/appserver/util/FunctionCache.java @@ -0,0 +1,104 @@ +package org.radarbase.appserver.util; + +import org.springframework.util.function.ThrowingFunction; + +import java.lang.ref.SoftReference; +import java.time.Duration; +import java.time.Instant; +import java.util.Iterator; +import java.util.LinkedHashMap; +import java.util.Map; + +public class FunctionCache { + private final Duration cacheTime; + + private final Duration retryTime; + + private final int maxSize; + + private final Map>> cachedMap; + private final ThrowingFunction function; + + public FunctionCache(ThrowingFunction function, + Duration cacheTime, + Duration retryTime, + int maxSize) { + this.cacheTime = cacheTime; + this.retryTime = retryTime; + this.maxSize = maxSize; + this.cachedMap = new LinkedHashMap<>(16, 0.75f, false); + this.function = function; + } + + public V getOrThrow(K input) throws Exception { + SoftReference> localRef; + synchronized (cachedMap) { + localRef = cachedMap.get(input); + } + Result result = localRef != null ? localRef.get() : null; + if (result != null && result.isValid()) { + return result.getOrThrow(); + } + + try { + V content = function.applyWithException(input); + putCache(input, new Result<>(cacheTime, content, null)); + return content; + } catch (Exception ex) { + synchronized (cachedMap) { + SoftReference> exRef = cachedMap.get(input); + Result exResult = exRef != null ? exRef.get() : null; + if (exResult == null || exResult.isBadResult()) { + putCache(input, new Result<>(retryTime, null, ex)); + throw ex; + } else { + return exResult.getOrThrow(); + } + } + } + } + + private void putCache(K input, Result result) { + synchronized (cachedMap) { + cachedMap.put(input, new SoftReference<>(result)); + int toRemove = cachedMap.size() - maxSize; + if (toRemove > 0) { + Iterator iter = cachedMap.entrySet().iterator(); + for (int i = 0; i < toRemove; i++) { + iter.next(); + iter.remove(); + toRemove--; + } + } + } + } + + private static class Result { + private final Instant expiration; + private final T value; + + private final Exception exception; + + Result(Duration expiryDuration, T value, Exception exception) { + expiration = Instant.now().plus(expiryDuration); + this.value = value; + this.exception = exception; + } + + T getOrThrow() throws Exception { + if (exception != null) { + throw exception; + } else { + return value; + } + } + + boolean isBadResult() { + return exception != null || !isValid(); + } + + boolean isValid() { + return Instant.now().isBefore(expiration); + } + } +} From ee821883b046808c2af6a4097d4c8655f4a96fb1 Mon Sep 17 00:00:00 2001 From: Joris Borgdorff Date: Thu, 15 Jun 2023 14:02:53 +0200 Subject: [PATCH 2/7] Renamed cache values --- .../java/org/radarbase/appserver/service/GithubService.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/radarbase/appserver/service/GithubService.java b/src/main/java/org/radarbase/appserver/service/GithubService.java index 2063bb2a..da44dc19 100644 --- a/src/main/java/org/radarbase/appserver/service/GithubService.java +++ b/src/main/java/org/radarbase/appserver/service/GithubService.java @@ -18,11 +18,11 @@ public class GithubService { @Autowired public GithubService( GithubClient githubClient, - @Value("${security.github.service.cacheDuration:PT1H}") + @Value("${security.github.cache.duration:PT1H}") Duration cacheTime, - @Value("${security.github.service.retryDuration:PT1M}") + @Value("${security.github.cache.retryDuration:PT1M}") Duration retryTime, - @Value("${security.github.service.cacheSize:10000}") + @Value("${security.github.cache.size:10000}") int maxSize) { this.cache = new FunctionCache<>(githubClient::getGithubContent, cacheTime, retryTime, maxSize); } From 940bd08a712ca02f12fd4e25967e91e2c772b75f Mon Sep 17 00:00:00 2001 From: Joris Borgdorff Date: Thu, 15 Jun 2023 14:23:06 +0200 Subject: [PATCH 3/7] Renamed FunctionCache to CachedFunction --- .../appserver/service/GithubService.java | 8 ++++---- .../protocol/DefaultProtocolGenerator.java | 16 +++++++--------- .../protocol/GithubProtocolFetcherStrategy.java | 2 +- .../{FunctionCache.java => CachedFunction.java} | 16 +++++++++------- 4 files changed, 21 insertions(+), 21 deletions(-) rename src/main/java/org/radarbase/appserver/util/{FunctionCache.java => CachedFunction.java} (86%) diff --git a/src/main/java/org/radarbase/appserver/service/GithubService.java b/src/main/java/org/radarbase/appserver/service/GithubService.java index da44dc19..9d0ab392 100644 --- a/src/main/java/org/radarbase/appserver/service/GithubService.java +++ b/src/main/java/org/radarbase/appserver/service/GithubService.java @@ -1,6 +1,6 @@ package org.radarbase.appserver.service; -import org.radarbase.appserver.util.FunctionCache; +import org.radarbase.appserver.util.CachedFunction; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Value; import org.springframework.beans.factory.config.ConfigurableBeanFactory; @@ -13,7 +13,7 @@ @Scope(value = ConfigurableBeanFactory.SCOPE_SINGLETON) public class GithubService { - private final transient FunctionCache cache; + private final transient CachedFunction cachedGetContent; @Autowired public GithubService( @@ -24,10 +24,10 @@ public GithubService( Duration retryTime, @Value("${security.github.cache.size:10000}") int maxSize) { - this.cache = new FunctionCache<>(githubClient::getGithubContent, cacheTime, retryTime, maxSize); + this.cachedGetContent = new CachedFunction<>(githubClient::getGithubContent, cacheTime, retryTime, maxSize); } public String getGithubContent(String url) throws Exception { - return this.cache.getOrThrow(url); + return this.cachedGetContent.applyWithException(url); } } diff --git a/src/main/java/org/radarbase/appserver/service/questionnaire/protocol/DefaultProtocolGenerator.java b/src/main/java/org/radarbase/appserver/service/questionnaire/protocol/DefaultProtocolGenerator.java index 4aefaae8..57584cb0 100644 --- a/src/main/java/org/radarbase/appserver/service/questionnaire/protocol/DefaultProtocolGenerator.java +++ b/src/main/java/org/radarbase/appserver/service/questionnaire/protocol/DefaultProtocolGenerator.java @@ -21,22 +21,20 @@ package org.radarbase.appserver.service.questionnaire.protocol; -import java.io.IOException; -import java.time.Duration; -import java.util.List; -import java.util.Map; -import java.util.NoSuchElementException; - import lombok.NonNull; import lombok.extern.slf4j.Slf4j; import org.radarbase.appserver.dto.protocol.Protocol; -import org.radarbase.appserver.entity.User; import org.radarbase.appserver.util.CachedMap; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.config.ConfigurableBeanFactory; import org.springframework.context.annotation.Scope; import org.springframework.stereotype.Service; +import java.io.IOException; +import java.time.Duration; +import java.util.Map; +import java.util.NoSuchElementException; + /** * @author yatharthranjan * @see aRMT Protocols @@ -90,7 +88,7 @@ public Protocol getProtocol(String projectId) throws IOException { return cachedProjectProtocolMap.get(projectId); } catch (IOException ex) { log.warn( - "Cannot retrieve Protocols for project {} : {}, Using cached values.", projectId, ex); + "Cannot retrieve Protocols for project {} : {}, Using cached values.", projectId, ex.toString()); return cachedProjectProtocolMap.get(true).get(projectId); } } @@ -115,7 +113,7 @@ public Protocol getProtocolForSubject(String subjectId) { return protocol; } catch (IOException ex) { log.warn( - "Cannot retrieve Protocols for subject {} : {}, Using cached values.", subjectId, ex); + "Cannot retrieve Protocols for subject {} : {}, Using cached values.", subjectId, ex.toString()); return cachedProtocolMap.getCache().get(subjectId); } catch(NoSuchElementException ex) { log.warn("Subject does not exist in map. Fetching.."); diff --git a/src/main/java/org/radarbase/appserver/service/questionnaire/protocol/GithubProtocolFetcherStrategy.java b/src/main/java/org/radarbase/appserver/service/questionnaire/protocol/GithubProtocolFetcherStrategy.java index 1a476b63..42030b58 100644 --- a/src/main/java/org/radarbase/appserver/service/questionnaire/protocol/GithubProtocolFetcherStrategy.java +++ b/src/main/java/org/radarbase/appserver/service/questionnaire/protocol/GithubProtocolFetcherStrategy.java @@ -95,7 +95,7 @@ public GithubProtocolFetcherStrategy( this.protocolFileName = protocolFileName; this.protocolBranch = protocolBranch; projectProtocolUriMap = - new CachedMap<>(this::getProtocolDirectories, Duration.ofHours(3), Duration.ofHours(4)); + new CachedMap<>(this::getProtocolDirectories, Duration.ofHours(3), Duration.ofMinutes(4)); this.objectMapper = objectMapper; this.localMapper = this.objectMapper.copy(); this.localMapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false); diff --git a/src/main/java/org/radarbase/appserver/util/FunctionCache.java b/src/main/java/org/radarbase/appserver/util/CachedFunction.java similarity index 86% rename from src/main/java/org/radarbase/appserver/util/FunctionCache.java rename to src/main/java/org/radarbase/appserver/util/CachedFunction.java index fd8bfd5d..5848ef04 100644 --- a/src/main/java/org/radarbase/appserver/util/FunctionCache.java +++ b/src/main/java/org/radarbase/appserver/util/CachedFunction.java @@ -1,5 +1,6 @@ package org.radarbase.appserver.util; +import org.jetbrains.annotations.NotNull; import org.springframework.util.function.ThrowingFunction; import java.lang.ref.SoftReference; @@ -9,7 +10,7 @@ import java.util.LinkedHashMap; import java.util.Map; -public class FunctionCache { +public class CachedFunction implements ThrowingFunction { private final Duration cacheTime; private final Duration retryTime; @@ -19,7 +20,7 @@ public class FunctionCache { private final Map>> cachedMap; private final ThrowingFunction function; - public FunctionCache(ThrowingFunction function, + public CachedFunction(ThrowingFunction function, Duration cacheTime, Duration retryTime, int maxSize) { @@ -30,13 +31,14 @@ public FunctionCache(ThrowingFunction function, this.function = function; } - public V getOrThrow(K input) throws Exception { + @NotNull + public V applyWithException(@NotNull K input) throws Exception { SoftReference> localRef; synchronized (cachedMap) { localRef = cachedMap.get(input); } Result result = localRef != null ? localRef.get() : null; - if (result != null && result.isValid()) { + if (result != null && !result.isExpired()) { return result.getOrThrow(); } @@ -94,11 +96,11 @@ T getOrThrow() throws Exception { } boolean isBadResult() { - return exception != null || !isValid(); + return exception != null || isExpired(); } - boolean isValid() { - return Instant.now().isBefore(expiration); + boolean isExpired() { + return Instant.now().isAfter(expiration); } } } From 6729986358eba078bce822154bd9b0e8aeae4ada Mon Sep 17 00:00:00 2001 From: Joris Borgdorff Date: Thu, 15 Jun 2023 14:33:28 +0200 Subject: [PATCH 4/7] Add configuration properties to file --- src/main/resources/application-dev.properties | 6 ++++++ src/main/resources/application-prod.properties | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/src/main/resources/application-dev.properties b/src/main/resources/application-dev.properties index 4264489c..f2c81f10 100644 --- a/src/main/resources/application-dev.properties +++ b/src/main/resources/application-dev.properties @@ -100,3 +100,9 @@ security.radar.managementportal.url=http://localhost:8081 #security.oauth2.client.userAuthorizationUri= # Github Authentication security.github.client.token= +security.github.client.timeout=PT10s +# max content size 1 MB +security.github.client.maxContentLength=1000000 +security.github.cache.size=10000 +security.github.cache.duration=PT1H +security.github.cache.retryDuration=PT1M diff --git a/src/main/resources/application-prod.properties b/src/main/resources/application-prod.properties index ec57b82f..c093b066 100644 --- a/src/main/resources/application-prod.properties +++ b/src/main/resources/application-prod.properties @@ -70,3 +70,9 @@ radar.admin.user=radar radar.admin.password=radar # Github Authentication security.github.client.token= +security.github.client.timeout=PT10s +# max content size 1 MB +security.github.client.maxContentLength=1000000 +security.github.cache.size=10000 +security.github.cache.duration=PT1H +security.github.cache.retryDuration=PT1M From a4505f8a17dc8af58474c1285482b545500ec797 Mon Sep 17 00:00:00 2001 From: Joris Borgdorff Date: Thu, 15 Jun 2023 15:00:55 +0200 Subject: [PATCH 5/7] Fix code checks --- .github/workflows/codeql-analysis.yml | 11 +++++++- .gitignore | 3 ++- .../resources/application.properties | 6 +++++ .../appserver/service/GithubClient.java | 26 +++++++++++-------- .../appserver/service/GithubService.java | 13 ++++++---- .../appserver/util/CachedFunction.java | 18 ++++++------- src/main/resources/application-dev.properties | 4 +-- .../resources/application-prod.properties | 6 ++--- 8 files changed, 55 insertions(+), 32 deletions(-) diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index 80ed2464..a229930f 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -41,7 +41,16 @@ jobs: - name: Checkout repository uses: actions/checkout@v3 - # Initializes the CodeQL tools for scanning. + - uses: actions/setup-java@v3 + if: matrix.language == 'java' + with: + java-version: 17 + distribution: temurin + + - uses: gradle/gradle-build-action@v2 + if: matrix.language == 'java' + + # Initializes the CodeQL tools for scanning. - name: Initialize CodeQL uses: github/codeql-action/init@v2 with: diff --git a/.gitignore b/.gitignore index 1ce04ed0..2b4d22c2 100644 --- a/.gitignore +++ b/.gitignore @@ -45,4 +45,5 @@ bin/ docker/prod -*__pycache__ \ No newline at end of file +*__pycache__ +google-credentials.json diff --git a/src/integrationTest/resources/application.properties b/src/integrationTest/resources/application.properties index 639f9b9e..bcd3bef2 100644 --- a/src/integrationTest/resources/application.properties +++ b/src/integrationTest/resources/application.properties @@ -96,3 +96,9 @@ security.radar.managementportal.url=http://localhost:8081 # Github Authentication security.github.client.token= +security.github.client.timeout=10 +# max content size 1 MB +security.github.client.maxContentLength=1000000 +security.github.cache.size=10000 +security.github.cache.duration=3600 +security.github.cache.retryDuration=60 diff --git a/src/main/java/org/radarbase/appserver/service/GithubClient.java b/src/main/java/org/radarbase/appserver/service/GithubClient.java index 2db2a4fe..6bd97333 100644 --- a/src/main/java/org/radarbase/appserver/service/GithubClient.java +++ b/src/main/java/org/radarbase/appserver/service/GithubClient.java @@ -51,18 +51,19 @@ public class GithubClient { @Value("${security.github.client.token}") private transient String githubToken; - @Value("${security.github.client.timeout:PT10s}") - private transient Duration httpTimeout; + private transient final Duration httpTimeout; @Value("${security.github.client.maxContentLength:1000000}") private transient int maxContentLength; @SneakyThrows @Autowired - public GithubClient() { + public GithubClient( + @Value("${security.github.client.timeout:10}") int httpTimeout) { + this.httpTimeout = Duration.ofSeconds(httpTimeout); client = HttpClient.newBuilder() .followRedirects(HttpClient.Redirect.NORMAL) - .connectTimeout(httpTimeout) + .connectTimeout(this.httpTimeout) .build(); } @@ -71,13 +72,7 @@ public String getGithubContent(String url) throws IOException, InterruptedExcept if (!this.isValidGithubUri(uri)) { throw new MalformedURLException("Invalid Github url."); } - HttpResponse response; - try { - response = client.send(getRequest(uri), HttpResponse.BodyHandlers.ofInputStream()); - } catch (IOException ex) { - log.error("Failed to retrieve data from github: {}", ex.toString()); - throw new ResponseStatusException(HttpStatus.BAD_GATEWAY, "Github responded with an error."); - } + HttpResponse response = makeRequest(uri); if (response.statusCode() >= 200 && response.statusCode() < 300) { checkContentLengthHeader(response); @@ -94,6 +89,15 @@ public String getGithubContent(String url) throws IOException, InterruptedExcept } } + private HttpResponse makeRequest(URI uri) throws InterruptedException { + try { + return client.send(getRequest(uri), HttpResponse.BodyHandlers.ofInputStream()); + } catch (IOException ex) { + log.error("Failed to retrieve data from github: {}", ex.toString()); + throw new ResponseStatusException(HttpStatus.BAD_GATEWAY, "Github responded with an error."); + } + } + private void checkContentLengthHeader(HttpResponse response) { response.headers().firstValue("Content-Length") .map((l) -> { diff --git a/src/main/java/org/radarbase/appserver/service/GithubService.java b/src/main/java/org/radarbase/appserver/service/GithubService.java index 9d0ab392..644e9496 100644 --- a/src/main/java/org/radarbase/appserver/service/GithubService.java +++ b/src/main/java/org/radarbase/appserver/service/GithubService.java @@ -18,13 +18,16 @@ public class GithubService { @Autowired public GithubService( GithubClient githubClient, - @Value("${security.github.cache.duration:PT1H}") - Duration cacheTime, - @Value("${security.github.cache.retryDuration:PT1M}") - Duration retryTime, + @Value("${security.github.cache.duration:3600}") + int cacheTime, + @Value("${security.github.cache.retryDuration:60}") + int retryTime, @Value("${security.github.cache.size:10000}") int maxSize) { - this.cachedGetContent = new CachedFunction<>(githubClient::getGithubContent, cacheTime, retryTime, maxSize); + this.cachedGetContent = new CachedFunction<>(githubClient::getGithubContent, + Duration.ofSeconds(cacheTime), + Duration.ofSeconds(retryTime), + maxSize); } public String getGithubContent(String url) throws Exception { diff --git a/src/main/java/org/radarbase/appserver/util/CachedFunction.java b/src/main/java/org/radarbase/appserver/util/CachedFunction.java index 5848ef04..943b9cf1 100644 --- a/src/main/java/org/radarbase/appserver/util/CachedFunction.java +++ b/src/main/java/org/radarbase/appserver/util/CachedFunction.java @@ -11,14 +11,14 @@ import java.util.Map; public class CachedFunction implements ThrowingFunction { - private final Duration cacheTime; + private transient final Duration cacheTime; - private final Duration retryTime; + private transient final Duration retryTime; - private final int maxSize; + private transient final int maxSize; - private final Map>> cachedMap; - private final ThrowingFunction function; + private transient final Map>> cachedMap; + private transient final ThrowingFunction function; public CachedFunction(ThrowingFunction function, Duration cacheTime, @@ -60,6 +60,7 @@ public V applyWithException(@NotNull K input) throws Exception { } } + @SuppressWarnings("PMD.DataflowAnomalyAnalysis") private void putCache(K input, Result result) { synchronized (cachedMap) { cachedMap.put(input, new SoftReference<>(result)); @@ -69,17 +70,16 @@ private void putCache(K input, Result result) { for (int i = 0; i < toRemove; i++) { iter.next(); iter.remove(); - toRemove--; } } } } private static class Result { - private final Instant expiration; - private final T value; + private transient final Instant expiration; + private transient final T value; - private final Exception exception; + private transient final Exception exception; Result(Duration expiryDuration, T value, Exception exception) { expiration = Instant.now().plus(expiryDuration); diff --git a/src/main/resources/application-dev.properties b/src/main/resources/application-dev.properties index f2c81f10..694c1546 100644 --- a/src/main/resources/application-dev.properties +++ b/src/main/resources/application-dev.properties @@ -104,5 +104,5 @@ security.github.client.timeout=PT10s # max content size 1 MB security.github.client.maxContentLength=1000000 security.github.cache.size=10000 -security.github.cache.duration=PT1H -security.github.cache.retryDuration=PT1M +security.github.cache.duration=3600 +security.github.cache.retryDuration=60 diff --git a/src/main/resources/application-prod.properties b/src/main/resources/application-prod.properties index c093b066..01d20eee 100644 --- a/src/main/resources/application-prod.properties +++ b/src/main/resources/application-prod.properties @@ -70,9 +70,9 @@ radar.admin.user=radar radar.admin.password=radar # Github Authentication security.github.client.token= -security.github.client.timeout=PT10s +security.github.client.timeout=10 # max content size 1 MB security.github.client.maxContentLength=1000000 security.github.cache.size=10000 -security.github.cache.duration=PT1H -security.github.cache.retryDuration=PT1M +security.github.cache.duration=3600 +security.github.cache.retryDuration=60 From c23c11f42740612a909918047787bd9548e0a48b Mon Sep 17 00:00:00 2001 From: Joris Borgdorff Date: Thu, 15 Jun 2023 16:02:48 +0200 Subject: [PATCH 6/7] Cleanup variable in constructor --- .../org/radarbase/appserver/service/GithubClient.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/radarbase/appserver/service/GithubClient.java b/src/main/java/org/radarbase/appserver/service/GithubClient.java index 6bd97333..6a2d50e4 100644 --- a/src/main/java/org/radarbase/appserver/service/GithubClient.java +++ b/src/main/java/org/radarbase/appserver/service/GithubClient.java @@ -39,6 +39,7 @@ import java.net.http.HttpRequest; import java.net.http.HttpResponse; import java.time.Duration; +import java.util.Optional; @Slf4j @Component @@ -48,8 +49,7 @@ public class GithubClient { private static final String GITHUB_API_ACCEPT_HEADER = "application/vnd.github.v3+json"; private final transient HttpClient client; - @Value("${security.github.client.token}") - private transient String githubToken; + private final transient String githubToken; private transient final Duration httpTimeout; @@ -59,7 +59,9 @@ public class GithubClient { @SneakyThrows @Autowired public GithubClient( - @Value("${security.github.client.timeout:10}") int httpTimeout) { + @Value("${security.github.client.timeout:10}") int httpTimeout, + @Value("${security.github.client.token:}") String githubToken) { + this.githubToken = githubToken != null && !githubToken.isBlank() ? githubToken.trim() : null; this.httpTimeout = Duration.ofSeconds(httpTimeout); client = HttpClient.newBuilder() .followRedirects(HttpClient.Redirect.NORMAL) @@ -128,7 +130,7 @@ private HttpRequest getRequest(URI uri) { .header("Accept", GITHUB_API_ACCEPT_HEADER) .GET() .timeout(httpTimeout); - if (githubToken != null && !githubToken.isEmpty()) { + if (githubToken != null) { request.header("Authorization", "Bearer " + githubToken); } return request.build(); From f80413314fc6dc9d79a2d0de8d426025bae5d60b Mon Sep 17 00:00:00 2001 From: Joris Borgdorff Date: Mon, 19 Jun 2023 10:04:11 +0200 Subject: [PATCH 7/7] Fix pmd --- .../radarbase/appserver/service/GithubClient.java | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/radarbase/appserver/service/GithubClient.java b/src/main/java/org/radarbase/appserver/service/GithubClient.java index 6a2d50e4..925151fa 100644 --- a/src/main/java/org/radarbase/appserver/service/GithubClient.java +++ b/src/main/java/org/radarbase/appserver/service/GithubClient.java @@ -21,6 +21,7 @@ package org.radarbase.appserver.service; +import jakarta.annotation.Nonnull; import lombok.SneakyThrows; import lombok.extern.slf4j.Slf4j; import org.springframework.beans.factory.annotation.Autowired; @@ -39,7 +40,6 @@ import java.net.http.HttpRequest; import java.net.http.HttpResponse; import java.time.Duration; -import java.util.Optional; @Slf4j @Component @@ -49,7 +49,8 @@ public class GithubClient { private static final String GITHUB_API_ACCEPT_HEADER = "application/vnd.github.v3+json"; private final transient HttpClient client; - private final transient String githubToken; + @Nonnull + private final transient String authorizationHeader; private transient final Duration httpTimeout; @@ -61,9 +62,9 @@ public class GithubClient { public GithubClient( @Value("${security.github.client.timeout:10}") int httpTimeout, @Value("${security.github.client.token:}") String githubToken) { - this.githubToken = githubToken != null && !githubToken.isBlank() ? githubToken.trim() : null; + this.authorizationHeader = githubToken != null ? "Bearer " + githubToken.trim() : ""; this.httpTimeout = Duration.ofSeconds(httpTimeout); - client = HttpClient.newBuilder() + this.client = HttpClient.newBuilder() .followRedirects(HttpClient.Redirect.NORMAL) .connectTimeout(this.httpTimeout) .build(); @@ -130,8 +131,8 @@ private HttpRequest getRequest(URI uri) { .header("Accept", GITHUB_API_ACCEPT_HEADER) .GET() .timeout(httpTimeout); - if (githubToken != null) { - request.header("Authorization", "Bearer " + githubToken); + if (!authorizationHeader.isEmpty()) { + request.header("Authorization", authorizationHeader); } return request.build(); }