From ce1ea638ee59b200de9be1e8b851533d2385aa78 Mon Sep 17 00:00:00 2001 From: Thomas Meyer Date: Sat, 27 Jul 2019 15:36:31 +0200 Subject: [PATCH 1/3] SystemScopeService: Optimize SQL access The scopesMatch() method is called for each ApprovedSite in the approval process and for each ApprovedSite entry all SystemScopes are read by a single SQL statement. Optimise this a bit and switch to a List so only one SQL is issued per ApprovedSite. --- .../org/mitre/oauth2/model/SystemScope.java | 4 +- .../repository/SystemScopeRepository.java | 2 + .../impl/JpaSystemScopeRepository.java | 8 ++++ .../impl/DefaultSystemScopeService.java | 39 +++++++++---------- 4 files changed, 32 insertions(+), 21 deletions(-) diff --git a/openid-connect-common/src/main/java/org/mitre/oauth2/model/SystemScope.java b/openid-connect-common/src/main/java/org/mitre/oauth2/model/SystemScope.java index 0807b160e8..83e84dfd13 100644 --- a/openid-connect-common/src/main/java/org/mitre/oauth2/model/SystemScope.java +++ b/openid-connect-common/src/main/java/org/mitre/oauth2/model/SystemScope.java @@ -38,11 +38,13 @@ @Table(name = "system_scope") @NamedQueries({ @NamedQuery(name = SystemScope.QUERY_ALL, query = "select s from SystemScope s ORDER BY s.id"), - @NamedQuery(name = SystemScope.QUERY_BY_VALUE, query = "select s from SystemScope s WHERE s.value = :" + SystemScope.PARAM_VALUE) + @NamedQuery(name = SystemScope.QUERY_BY_VALUE, query = "select s from SystemScope s WHERE s.value = :" + SystemScope.PARAM_VALUE), + @NamedQuery(name = SystemScope.QUERY_BY_VALUES, query = "select s from SystemScope s WHERE s.value in :" + SystemScope.PARAM_VALUE) }) public class SystemScope { public static final String QUERY_BY_VALUE = "SystemScope.getByValue"; + public static final String QUERY_BY_VALUES = "SystemScope.getByValues"; public static final String QUERY_ALL = "SystemScope.findAll"; public static final String PARAM_VALUE = "value"; diff --git a/openid-connect-common/src/main/java/org/mitre/oauth2/repository/SystemScopeRepository.java b/openid-connect-common/src/main/java/org/mitre/oauth2/repository/SystemScopeRepository.java index 8c891d566d..dd41ace47a 100644 --- a/openid-connect-common/src/main/java/org/mitre/oauth2/repository/SystemScopeRepository.java +++ b/openid-connect-common/src/main/java/org/mitre/oauth2/repository/SystemScopeRepository.java @@ -36,6 +36,8 @@ public interface SystemScopeRepository { public SystemScope getByValue(String value); + public Set getByValues(Set values); + public void remove(SystemScope scope); public SystemScope save(SystemScope scope); diff --git a/openid-connect-server/src/main/java/org/mitre/oauth2/repository/impl/JpaSystemScopeRepository.java b/openid-connect-server/src/main/java/org/mitre/oauth2/repository/impl/JpaSystemScopeRepository.java index f646b57243..f2d575b197 100644 --- a/openid-connect-server/src/main/java/org/mitre/oauth2/repository/impl/JpaSystemScopeRepository.java +++ b/openid-connect-server/src/main/java/org/mitre/oauth2/repository/impl/JpaSystemScopeRepository.java @@ -23,6 +23,7 @@ import static org.mitre.util.jpa.JpaUtil.getSingleResult; import static org.mitre.util.jpa.JpaUtil.saveOrUpdate; +import java.util.HashSet; import java.util.LinkedHashSet; import java.util.Set; @@ -76,6 +77,13 @@ public SystemScope getByValue(String value) { return getSingleResult(query.getResultList()); } + @Override + public Set getByValues(Set values) { + TypedQuery query = em.createNamedQuery(SystemScope.QUERY_BY_VALUES, SystemScope.class); + query.setParameter(SystemScope.PARAM_VALUE, values); + return new HashSet<>(query.getResultList()); + } + /* (non-Javadoc) * @see org.mitre.oauth2.repository.SystemScopeRepository#remove(org.mitre.oauth2.model.SystemScope) */ diff --git a/openid-connect-server/src/main/java/org/mitre/oauth2/service/impl/DefaultSystemScopeService.java b/openid-connect-server/src/main/java/org/mitre/oauth2/service/impl/DefaultSystemScopeService.java index 21474fe6e0..61022a73c4 100644 --- a/openid-connect-server/src/main/java/org/mitre/oauth2/service/impl/DefaultSystemScopeService.java +++ b/openid-connect-server/src/main/java/org/mitre/oauth2/service/impl/DefaultSystemScopeService.java @@ -20,8 +20,10 @@ */ package org.mitre.oauth2.service.impl; +import java.util.HashSet; import java.util.LinkedHashSet; import java.util.Set; +import java.util.stream.Collectors; import org.mitre.oauth2.model.SystemScope; import org.mitre.oauth2.repository.SystemScopeRepository; @@ -32,7 +34,6 @@ import com.google.common.base.Function; import com.google.common.base.Predicate; import com.google.common.base.Predicates; -import com.google.common.base.Strings; import com.google.common.collect.Collections2; import com.google.common.collect.Sets; @@ -67,24 +68,6 @@ public boolean apply(SystemScope input) { } }; - private Function stringToSystemScope = new Function() { - @Override - public SystemScope apply(String input) { - if (Strings.isNullOrEmpty(input)) { - return null; - } else { - // get the real scope if it's available - SystemScope s = getByValue(input); - if (s == null) { - // make a fake one otherwise - s = new SystemScope(input); - } - - return s; - } - } - }; - private Function systemScopeToString = new Function() { @Override public String apply(SystemScope input) { @@ -120,6 +103,10 @@ public SystemScope getByValue(String value) { return repository.getByValue(value); } + private Set getByValues(Set values) { + return repository.getByValues(values); + } + /* (non-Javadoc) * @see org.mitre.oauth2.service.SystemScopeService#remove(org.mitre.oauth2.model.SystemScope) */ @@ -149,7 +136,19 @@ public Set fromStrings(Set scope) { if (scope == null) { return null; } else { - return new LinkedHashSet<>(Collections2.filter(Collections2.transform(scope, stringToSystemScope), Predicates.notNull())); + Set scopeValues = scope.stream().filter(s -> s != null).collect(Collectors.toSet()); + Set scopesFromDB = getByValues(scopeValues); + Set scopesFromDBValues = scopesFromDB.stream().map(SystemScope::getValue).collect(Collectors.toSet()); + Set missingScopesFromDB = scopesFromDB + .stream() + .map(SystemScope::getValue) + .filter(sv -> !scopesFromDBValues.contains(sv)) + .map(sv -> new SystemScope(sv)) + .collect(Collectors.toSet()); + Set allScopes = new HashSet(); + allScopes.addAll(scopesFromDB); + allScopes.addAll(missingScopesFromDB); + return allScopes; } } From 7dc3b8ec3ae3429c81e744f61f6b772666c7b6b3 Mon Sep 17 00:00:00 2001 From: Thomas Meyer Date: Sat, 3 Aug 2019 14:59:22 +0200 Subject: [PATCH 2/3] JpaSystemScopeRepository: Fix JPA expectations --- .../oauth2/repository/impl/JpaSystemScopeRepository.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/openid-connect-server/src/main/java/org/mitre/oauth2/repository/impl/JpaSystemScopeRepository.java b/openid-connect-server/src/main/java/org/mitre/oauth2/repository/impl/JpaSystemScopeRepository.java index f2d575b197..ad0d2da47e 100644 --- a/openid-connect-server/src/main/java/org/mitre/oauth2/repository/impl/JpaSystemScopeRepository.java +++ b/openid-connect-server/src/main/java/org/mitre/oauth2/repository/impl/JpaSystemScopeRepository.java @@ -23,6 +23,7 @@ import static org.mitre.util.jpa.JpaUtil.getSingleResult; import static org.mitre.util.jpa.JpaUtil.saveOrUpdate; +import java.util.Collections; import java.util.HashSet; import java.util.LinkedHashSet; import java.util.Set; @@ -79,6 +80,10 @@ public SystemScope getByValue(String value) { @Override public Set getByValues(Set values) { + if(values.isEmpty()) { + return Collections.emptySet(); + } + TypedQuery query = em.createNamedQuery(SystemScope.QUERY_BY_VALUES, SystemScope.class); query.setParameter(SystemScope.PARAM_VALUE, values); return new HashSet<>(query.getResultList()); From 742620a29ce386b412316ec1501a6d216562e644 Mon Sep 17 00:00:00 2001 From: Thomas Meyer Date: Sat, 3 Aug 2019 15:04:23 +0200 Subject: [PATCH 3/3] DefaultSystemScopeService: Fix new logic --- .../oauth2/service/impl/DefaultSystemScopeService.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/openid-connect-server/src/main/java/org/mitre/oauth2/service/impl/DefaultSystemScopeService.java b/openid-connect-server/src/main/java/org/mitre/oauth2/service/impl/DefaultSystemScopeService.java index 61022a73c4..d5182de93a 100644 --- a/openid-connect-server/src/main/java/org/mitre/oauth2/service/impl/DefaultSystemScopeService.java +++ b/openid-connect-server/src/main/java/org/mitre/oauth2/service/impl/DefaultSystemScopeService.java @@ -22,6 +22,7 @@ import java.util.HashSet; import java.util.LinkedHashSet; +import java.util.Objects; import java.util.Set; import java.util.stream.Collectors; @@ -136,15 +137,15 @@ public Set fromStrings(Set scope) { if (scope == null) { return null; } else { - Set scopeValues = scope.stream().filter(s -> s != null).collect(Collectors.toSet()); + Set scopeValues = scope.stream().filter(Objects::nonNull).collect(Collectors.toSet()); Set scopesFromDB = getByValues(scopeValues); Set scopesFromDBValues = scopesFromDB.stream().map(SystemScope::getValue).collect(Collectors.toSet()); - Set missingScopesFromDB = scopesFromDB + Set missingScopesFromDB = scopeValues .stream() - .map(SystemScope::getValue) .filter(sv -> !scopesFromDBValues.contains(sv)) .map(sv -> new SystemScope(sv)) .collect(Collectors.toSet()); + Set allScopes = new HashSet(); allScopes.addAll(scopesFromDB); allScopes.addAll(missingScopesFromDB);