Skip to content

Commit 755d41d

Browse files
LEDfanLEDfan
committed
Fix #28289: allow specifying default values for parameters
1 parent 76cb823 commit 755d41d

8 files changed

Lines changed: 219 additions & 9 deletions

File tree

‎src/main/java/eu/openanalytics/containerproxy/model/runtime/AllowedParametersForUser.java‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,6 @@
2020
*/
2121
package eu.openanalytics.containerproxy.model.runtime;
2222

23-
import org.apache.commons.lang3.tuple.Pair;
24-
2523
import java.util.HashSet;
2624
import java.util.List;
2725
import java.util.Map;
@@ -38,10 +36,12 @@ public class AllowedParametersForUser {
3836
*/
3937
private final HashSet<List<Integer>> allowedCombinations;
4038

39+
private final List<Integer> defaultValue;
4140

42-
public AllowedParametersForUser(Map<String, List<String>> values, HashSet<List<Integer>> allowedCombinations) {
41+
public AllowedParametersForUser(Map<String, List<String>> values, HashSet<List<Integer>> allowedCombinations, List<Integer> defaultValue) {
4342
this.values = values;
4443
this.allowedCombinations = allowedCombinations;
44+
this.defaultValue = defaultValue;
4545
}
4646

4747
public HashSet<List<Integer>> getAllowedCombinations() {
@@ -51,4 +51,8 @@ public HashSet<List<Integer>> getAllowedCombinations() {
5151
public Map<String, List<String>> getValues() {
5252
return values;
5353
}
54+
55+
public List<Integer> getDefaultValue() {
56+
return defaultValue;
57+
}
5458
}

‎src/main/java/eu/openanalytics/containerproxy/model/spec/ParameterDefinition.java‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,13 +33,16 @@ public class ParameterDefinition {
3333
private final String displayName;
3434
private final String description;
3535

36+
private final String defaultValue;
37+
3638
// a mapping of the raw value (used in the backend) to a human friendly name used in the front-end
3739
private final BidiMap<String, String> valueNames;
3840

39-
public ParameterDefinition(String id, String displayName, String description, List<ValueName> valueNames) {
41+
public ParameterDefinition(String id, String displayName, String description, List<ValueName> valueNames, String defaultValue) {
4042
this.id = id;
4143
this.displayName = displayName;
4244
this.description = description;
45+
this.defaultValue = defaultValue;
4346
this.valueNames = new DualHashBidiMap<>();
4447
if (valueNames != null) {
4548
for (ValueName valueName : valueNames) {
@@ -75,6 +78,10 @@ public String getId() {
7578
return id;
7679
}
7780

81+
public String getDefaultValue() {
82+
return defaultValue;
83+
}
84+
7885
/**
7986
* Given the (backend) value, return the human friendly name for the value.
8087
* @param value the backend-value

‎src/main/java/eu/openanalytics/containerproxy/service/ParametersService.java‎

Lines changed: 50 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,14 +35,17 @@
3535

3636
import javax.annotation.PostConstruct;
3737
import java.util.ArrayList;
38+
import java.util.Collections;
3839
import java.util.HashMap;
3940
import java.util.HashSet;
4041
import java.util.List;
4142
import java.util.Map;
43+
import java.util.Objects;
4244
import java.util.Optional;
4345
import java.util.Set;
4446
import java.util.regex.Pattern;
4547
import java.util.stream.Collectors;
48+
import java.util.stream.Stream;
4649

4750
@Service
4851
public class ParametersService {
@@ -53,12 +56,9 @@ public class ParametersService {
5356

5457
private static final Pattern PARAMETER_ID_PATTERN = Pattern.compile("[a-zA-Z\\d_-]*");
5558

56-
private final ObjectMapper objectMapper;
57-
5859
public ParametersService(IProxySpecProvider baseSpecProvider, AccessControlEvaluationService accessControlEvaluationService, ObjectMapper objectMapper) {
5960
this.baseSpecProvider = baseSpecProvider;
6061
this.accessControlEvaluationService = accessControlEvaluationService;
61-
this.objectMapper = objectMapper;
6262
}
6363

6464
@PostConstruct
@@ -102,6 +102,28 @@ private void validateSpec(ProxySpec spec) {
102102
}
103103
}
104104

105+
// Check that either no defaults are provided or that all parameters has a default value
106+
List<String> defaults = spec.getParameters().getDefinitions().stream().map(ParameterDefinition::getDefaultValue).collect(Collectors.toList());
107+
if (!defaults.stream().allMatch(Objects::isNull) && !defaults.stream().allMatch(Objects::nonNull)) {
108+
throw new IllegalStateException(String.format("Configuration error: error in parameters of spec '%s', error: not every parameter has a default value. Either define no defaults, or defaults for all parameters.", spec.getId()));
109+
}
110+
111+
// Check that every default value exists
112+
if (spec.getParameters().getDefinitions().get(0).getDefaultValue() != null) {
113+
for (ParameterDefinition definition : spec.getParameters().getDefinitions()) {
114+
boolean defaultValueExists = false;
115+
for (Parameters.ValueSet valueSet : spec.getParameters().getValueSets()) {
116+
if (valueSet.getParameterValues(definition.getId()).contains(definition.getDefaultValue())) {
117+
defaultValueExists = true;
118+
break;
119+
}
120+
}
121+
if (!defaultValueExists) {
122+
throw new IllegalStateException(String.format("Configuration error: error in parameters of spec '%s', error: default value for parameter with id '%s' is not defined in a value-set", spec.getId(), definition.getId()));
123+
}
124+
}
125+
}
126+
105127
// Validate Parameter Value Sets
106128
int valueSetIdx = 0;
107129
for (Parameters.ValueSet valueSet : spec.getParameters().getValueSets()) {
@@ -232,7 +254,7 @@ private List<ParameterNames.ParameterName> getParameterNames(List<ParameterDefin
232254
public AllowedParametersForUser calculateAllowedParametersForUser(Authentication auth, ProxySpec proxySpec) {
233255
Parameters parameters = proxySpec.getParameters();
234256
if (parameters == null) {
235-
return new AllowedParametersForUser(new HashMap<>(), new HashSet<>());
257+
return new AllowedParametersForUser(new HashMap<>(), new HashSet<>(), null);
236258
}
237259
List<String> parameterIds = parameters.getIds();
238260

@@ -270,7 +292,10 @@ public AllowedParametersForUser calculateAllowedParametersForUser(Authentication
270292
allowedCombinations.addAll(getAllowedCombinationsForSingleValueSet(parameterIds, valueSet, valuesToIndex));
271293
}
272294

273-
return new AllowedParametersForUser(values, allowedCombinations);
295+
// 4. compute default value
296+
List<Integer> defaultValue = getDefaultValue(parameters.getDefinitions(), allowedCombinations, valuesToIndex);
297+
298+
return new AllowedParametersForUser(values, allowedCombinations, defaultValue);
274299

275300
}
276301

@@ -302,4 +327,24 @@ private List<List<Integer>> getAllowedCombinationsForSingleValueSet(List<String>
302327
return newAllowedCombinations;
303328
}
304329

330+
private List<Integer> getDefaultValue(List<ParameterDefinition> definitions, HashSet<List<Integer>> allowedCombinations, Map<String, Map<String, Integer>> valuesToIndex) {
331+
List<Integer> noDefault = new ArrayList<>(Collections.nCopies(definitions.size(), 0));
332+
List<Integer> result = new ArrayList<>();
333+
334+
if (definitions.get(0).getDefaultValue() == null) {
335+
return noDefault; // no default values defined
336+
}
337+
for (ParameterDefinition definition: definitions) {
338+
Integer valueIndex = valuesToIndex.get(definition.getId()).get(definition.getDefaultValue());
339+
if (valueIndex == null) {
340+
return noDefault; // default value cannot be used by this user
341+
}
342+
result.add(valueIndex);
343+
}
344+
if (allowedCombinations.contains(result)) {
345+
return result;
346+
}
347+
return noDefault; // this combination cannot be used by the user
348+
}
349+
305350
}

‎src/test/java/eu/openanalytics/containerproxy/test/unit/TestParameterValidationService.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,12 @@ public void testValueSetErrors() {
7070
test("classpath:application-parameters-validation-7.yaml", "Configuration error: error in parameters of spec 'big-parameters', error: value set 0 contains values for more parameters than there are defined");
7171
}
7272

73+
@Test
74+
public void testDefaultValueErrors() {
75+
test("classpath:application-parameters-validation-9.yaml", "Configuration error: error in parameters of spec 'big-parameters', error: not every parameter has a default value. Either define no defaults, or defaults for all parameters");
76+
test("classpath:application-parameters-validation-10.yaml", "Configuration error: error in parameters of spec 'big-parameters', error: default value for parameter with id 'parameter2' is not defined in a value-set");
77+
}
78+
7379
public static class TestPropertyLoader implements ApplicationContextInitializer<ConfigurableApplicationContext> {
7480

7581
private final String location;

‎src/test/java/eu/openanalytics/containerproxy/test/unit/TestParametersService.java‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@
4646
import java.util.Optional;
4747

4848
import static org.mockito.Mockito.mock;
49+
import static org.mockito.Mockito.when;
4950

5051
@SpringBootTest(classes = {TestIntegrationOnKube.TestConfiguration.class, ContainerProxyApplication.class})
5152
@ContextConfiguration(initializers = PropertyOverrideContextInitializer.class)
@@ -149,6 +150,35 @@ public void testBigParameters() {
149150
"well"
150151
),
151152
allowedParametersForUser.getValues().get("parameter4"));
153+
154+
Assertions.assertEquals(Arrays.asList(0, 0, 0, 0), allowedParametersForUser.getDefaultValue());
155+
}
156+
157+
@Test
158+
public void testDefaultValues() {
159+
ProxySpec spec = proxyService.getProxySpec("default-values");
160+
161+
Authentication authJack = mock(Authentication.class);
162+
when(authJack.getName()).thenReturn("jack");
163+
164+
AllowedParametersForUser allowedParametersForUser = parametersService.calculateAllowedParametersForUser(authJack, spec);
165+
// jack does not have access to a value of this set
166+
Assertions.assertEquals(Arrays.asList(0, 0, 0), allowedParametersForUser.getDefaultValue());
167+
168+
Authentication authThomas = mock(Authentication.class);
169+
when(authThomas.getName()).thenReturn("thomas");
170+
171+
AllowedParametersForUser allowedParametersForUser2 = parametersService.calculateAllowedParametersForUser(authThomas, spec);
172+
// thomas does not have access to the combination of the default values
173+
Assertions.assertEquals(Arrays.asList(0, 0, 0), allowedParametersForUser2.getDefaultValue());
174+
175+
Authentication authJeff = mock(Authentication.class);
176+
when(authJeff.getName()).thenReturn("jeff");
177+
178+
AllowedParametersForUser allowedParametersForUser3 = parametersService.calculateAllowedParametersForUser(authJeff, spec);
179+
// thomas does not have access to the combination of the default values
180+
Assertions.assertEquals(Arrays.asList(1, 2, 3), allowedParametersForUser3.getDefaultValue());
181+
152182
}
153183

154184
private Pair<ParameterNames, ParameterValues> testAllowedValue(ProxySpec spec, String parameter1, String parameter2, String parameter3, String parameter4) throws InvalidParametersException {

‎src/test/resources/application-parameters-validation-10.yaml‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
spring:
2+
session:
3+
store-type: none
4+
data:
5+
redis:
6+
repositories:
7+
enabled: false
8+
proxy:
9+
authentication: simple
10+
container-backend: docker
11+
12+
13+
users:
14+
- name: demo
15+
password: demo
16+
- name: demo2
17+
password: demo2
18+
19+
docker:
20+
url: http://localhost:2375
21+
22+
specs:
23+
- id: big-parameters
24+
container-specs:
25+
- image: "openanalytics/shinyproxy-demo"
26+
cmd: [ "R", "-e", "shinyproxy::run_01_hello()" ]
27+
port-mapping:
28+
default: 3838
29+
parameters:
30+
definitions:
31+
- id: parameter1
32+
default-value: a
33+
- id: parameter2
34+
default-value: abc2
35+
value-sets:
36+
- values:
37+
parameter1:
38+
- a
39+
parameter2:
40+
- abc

‎src/test/resources/application-parameters-validation-9.yaml‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
spring:
2+
session:
3+
store-type: none
4+
data:
5+
redis:
6+
repositories:
7+
enabled: false
8+
proxy:
9+
authentication: simple
10+
container-backend: docker
11+
12+
13+
users:
14+
- name: demo
15+
password: demo
16+
- name: demo2
17+
password: demo2
18+
19+
docker:
20+
url: http://localhost:2375
21+
22+
specs:
23+
- id: big-parameters
24+
container-specs:
25+
- image: "openanalytics/shinyproxy-demo"
26+
cmd: [ "R", "-e", "shinyproxy::run_01_hello()" ]
27+
port-mapping:
28+
default: 3838
29+
parameters:
30+
definitions:
31+
- id: parameter1
32+
- id: parameter2
33+
default-value: abc
34+
value-sets:
35+
- values:
36+
parameter1:
37+
- a
38+
parameter2:
39+
- abc

‎src/test/resources/application-parameters.yaml‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -226,3 +226,42 @@ proxy:
226226
users:
227227
- jack
228228
- jeff
229+
- id: default-values
230+
container-specs:
231+
- image: "openanalytics/shinyproxy-demo"
232+
cmd: [ "R", "-e", "shinyproxy::run_01_hello()" ]
233+
port-mapping:
234+
default: 3838
235+
parameters:
236+
definitions:
237+
- id: parameter1
238+
default-value: value1
239+
- id: parameter2
240+
default-value: value2
241+
- id: parameter3
242+
default-value: value3
243+
value-sets:
244+
- values:
245+
parameter1:
246+
- value1
247+
- value2
248+
parameter2:
249+
- value1
250+
- value2
251+
parameter3:
252+
- value1
253+
- value2
254+
- values:
255+
parameter1: value1
256+
parameter2: value2
257+
parameter3: value3
258+
access-control:
259+
users:
260+
- jeff
261+
- values:
262+
parameter1: value1
263+
parameter2: value3
264+
parameter3: value3
265+
access-control:
266+
users:
267+
- thomas

0 commit comments

Comments
 (0)