Skip to content

Commit

Permalink
CDR-1391 Reject invalid fetch/offset with HTTP 422
Browse files Browse the repository at this point in the history
  • Loading branch information
alexlehn committed May 24, 2024
1 parent 300b591 commit b82afd9
Show file tree
Hide file tree
Showing 8 changed files with 233 additions and 27 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
### Changed
* The field `q` of AQL query responses now contain the requested, and not the executed, query string ([#1296](https://github.com/ehrbase/ehrbase/pull/1296))
* The field `meta._schema_version` of AQL query responses has been changed to `1.0.3` ([#1296](https://github.com/ehrbase/ehrbase/pull/1296))
* Return HTTP 422 Unprocessable Content in case fetch or offset is defined inside the aql query and as parameter ([#1325](https://github.com/ehrbase/ehrbase/pull/1325)).
### Fixed

## [2.1.0]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ public AqlQueryRepository(
* @param selects to obtain {@link AqlSqlResultPostprocessor} for.
*
* @see #executeQuery(PreparedQuery)
* @see #printQuery(PreparedQuery)
* @see #explainQuery(PreparedQuery)
* @see #getQuerySql(PreparedQuery)
* @see #explainQuery(boolean, PreparedQuery)
*/
public PreparedQuery prepareQuery(AslRootQuery aslQuery, List<SelectWrapper> selects) {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,15 @@
* Represents a prepared but not executed SQL query for the {@link AqlQueryRepository} that is constructed by
* {@link AqlQueryRepository#prepareQuery(AslRootQuery, List)}. This prepared query can be executed by
* {@link AqlQueryRepository#executeQuery(PreparedQuery)} or can be used to obtain the raw SQL query using
* {@link AqlQueryRepository#printQuery(PreparedQuery)} or the query planer output
* {@link AqlQueryRepository#explainQuery(PreparedQuery)}.
* {@link AqlQueryRepository#getQuerySql(PreparedQuery)}} or the query planer output
* {@link AqlQueryRepository#explainQuery(boolean, PreparedQuery)}.
*/
public final class PreparedQuery {

final SelectQuery<Record> selectQuery;
final Map<Integer, AqlSqlResultPostprocessor> postProcessors;

PreparedQuery(SelectQuery<Record> selectQuery, Map<Integer, AqlSqlResultPostprocessor> postProcessors) {
public PreparedQuery(SelectQuery<Record> selectQuery, Map<Integer, AqlSqlResultPostprocessor> postProcessors) {
this.selectQuery = selectQuery;
this.postProcessors = postProcessors;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
import org.ehrbase.api.exception.BadGatewayException;
import org.ehrbase.api.exception.IllegalAqlException;
import org.ehrbase.api.exception.InternalServerException;
import org.ehrbase.api.exception.InvalidApiParameterException;
import org.ehrbase.api.exception.UnprocessableEntityException;
import org.ehrbase.api.service.AqlQueryService;
import org.ehrbase.openehr.aqlengine.AqlQueryUtils;
import org.ehrbase.openehr.aqlengine.asl.AqlSqlLayer;
Expand Down Expand Up @@ -188,29 +188,30 @@ private static AqlQuery buildAqlQuery(AqlQueryRequest aqlQueryRequest) {
AqlQuery aqlQuery = AqlQueryParser.parse(aqlQueryRequest.queryString());

// apply limit and offset - where the definitions from the aql are the precedence
Long fetch = aqlQueryRequest.fetch();
if (fetch != null) {
if (aqlQuery.getLimit() != null) {
throw new InvalidApiParameterException(
"Invalid AQL query: fetch is defined on query %s and as parameter %s"
.formatted(aqlQuery.getLimit(), fetch));
}
aqlQuery.setLimit(fetch);
}
Long offset = aqlQueryRequest.offset();
if (offset != null) {
if (aqlQuery.getOffset() != null) {
throw new InvalidApiParameterException(
"Invalid AQL query: fetch is defined on query %s and as parameter %s"
.formatted(aqlQuery.getOffset(), offset));
}
aqlQuery.setOffset(offset);
Long fetchParam = aqlQueryRequest.fetch();
Long offsetParam = aqlQueryRequest.offset();
Long limitQuery = aqlQuery.getLimit();

// verify not parameter fetch offset are defined when query contains a LIMIT or assign fetch parameter
Optional.ofNullable(limitQuery)
.ifPresentOrElse(
limit -> {
if (fetchParam != null || offsetParam != null) {
throw new UnprocessableEntityException(
"Query contains a LIMIT clause, fetch and offset parameters must not be used");
}
},
() -> Optional.ofNullable(fetchParam).ifPresent(aqlQuery::setLimit));

// assign offset parameter
if (aqlQuery.getOffset() == null) {
Optional.ofNullable(offsetParam).ifPresent(aqlQuery::setOffset);
}

// sanity check - In AQL there is no offset without limit.
// sanity check parameter
if (aqlQuery.getOffset() != null && aqlQuery.getLimit() == null) {
throw new InvalidApiParameterException(
"Invalid AQL query: provided offset %s without a limit".formatted(aqlQuery.getOffset()));
throw new UnprocessableEntityException(
"Query parameter for offset %s provided without a fetch limit".formatted(aqlQuery.getOffset()));
}

// postprocess
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,60 @@
package org.ehrbase.openehr.aqlengine.service;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.mock;

import com.fasterxml.jackson.databind.ObjectMapper;
import java.util.Map;
import java.util.Optional;
import org.apache.commons.lang3.StringUtils;
import org.ehrbase.api.dto.AqlQueryContext;
import org.ehrbase.api.dto.AqlQueryRequest;
import org.ehrbase.api.exception.UnprocessableEntityException;
import org.ehrbase.openehr.aqlengine.asl.AqlSqlLayer;
import org.ehrbase.openehr.aqlengine.featurecheck.AqlQueryFeatureCheck;
import org.ehrbase.openehr.aqlengine.repository.AqlQueryRepository;
import org.ehrbase.openehr.aqlengine.repository.PreparedQuery;
import org.ehrbase.openehr.sdk.aql.dto.AqlQuery;
import org.ehrbase.openehr.sdk.aql.parser.AqlQueryParser;
import org.ehrbase.openehr.test.Fixture;
import org.ehrbase.openehr.test.TestAqlQueryContext;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;
import org.mockito.Mockito;

class AqlQueryServiceImpTest {

private static final ObjectMapper objectMapper = new ObjectMapper();

// returns null for each call
private final AqlQueryRepository mockQueryRepository = mock();

@BeforeEach
void setUp() {
Mockito.reset(mockQueryRepository);
PreparedQuery preparedQuery = Fixture.preparedQuery("SELECT * FROM AqlQueryServiceImpTest");
doReturn(preparedQuery).when(mockQueryRepository).prepareQuery(any(), any());
}

private AqlQueryServiceImp service() {
return service(new TestAqlQueryContext());
}

private AqlQueryServiceImp service(AqlQueryContext queryContext) {

return new AqlQueryServiceImp(
mockQueryRepository,
null,
new AqlSqlLayer(null, Fixture.systemService()),
new AqlQueryFeatureCheck(Fixture.systemService()),
objectMapper,
queryContext);
}

@ParameterizedTest
@CsvSource(
textBlock =
Expand Down Expand Up @@ -55,4 +101,51 @@ void resolveEhrCompositions(String srcAql, String expectedAql) {
AqlQueryServiceImp.replaceEhrPaths(aqlQuery);
assertThat(aqlQuery.render()).isEqualTo(expectedAql.replaceAll(" +", " "));
}

@ParameterizedTest
@CsvSource(
textBlock =
"""
5||10||Query contains a LIMIT clause, fetch and offset parameters must not be used
5|20||40|Query contains a LIMIT clause, fetch and offset parameters must not be used
5|||30|Query contains a LIMIT clause, fetch and offset parameters must not be used
|||42|Query parameter for offset 42 provided without a fetch limit
""",
delimiterString = "|")
void queryOffsetLimitRejected(
String aqlLimit, String aqlOffset, String paramLimit, String paramOffset, String message) {

assertThatThrownBy(() -> runQueryTest(aqlLimit, aqlOffset, paramLimit, paramOffset))
.isInstanceOf(UnprocessableEntityException.class)
.hasMessage(message);
}

@ParameterizedTest
@CsvSource(
textBlock =
"""
5|||
5|15||
||20|
||20|25
""",
delimiterString = "|")
void queryOffsetLimitAccepted(String aqlLimit, String aqlOffset, String paramLimit, String paramOffset) {
runQueryTest(aqlLimit, aqlOffset, paramLimit, paramOffset);
}

private void runQueryTest(String aqlLimit, String aqlOffset, String paramLimit, String paramOffset) {
// @format:off
String query = "SELECT s FROM EHR_STATUS s %s %s".formatted(
Optional.ofNullable(aqlLimit).filter(s -> !s.isEmpty()).map(s -> "LIMIT " + s).orElse(""),
Optional.ofNullable(aqlOffset).filter(s -> !s.isEmpty()).map(s -> "OFFSET " + s).orElse("")
);
service().query(new AqlQueryRequest(
query,
Map.of(),
Optional.ofNullable(paramLimit).filter(StringUtils::isNotEmpty).map(Long::parseLong).orElse(null),
Optional.ofNullable(paramOffset).filter(s -> !s.isEmpty()).map(Long::parseLong).orElse(null))
);
// @format:on
}
}
34 changes: 34 additions & 0 deletions aql-engine/src/test/java/org/ehrbase/openehr/test/Fixture.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* Copyright (c) 2019-2024 vitasystems GmbH.
*
* This file is part of project EHRbase
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.ehrbase.openehr.test;

import java.util.Map;
import org.ehrbase.api.service.SystemService;
import org.ehrbase.openehr.aqlengine.repository.PreparedQuery;
import org.jooq.impl.DSL;

public class Fixture {

public static SystemService systemService() {
return () -> "test-system-id";
}

public static PreparedQuery preparedQuery(String query) {
return new PreparedQuery(DSL.selectFrom(query).getQuery(), Map.of());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
/*
* Copyright (c) 2019-2024 vitasystems GmbH.
*
* This file is part of project EHRbase
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.ehrbase.openehr.test;

import java.net.URI;
import java.util.HashMap;
import java.util.Map;
import java.util.function.Consumer;
import org.ehrbase.api.dto.AqlQueryContext;
import org.ehrbase.openehr.sdk.response.dto.MetaData;

public class TestAqlQueryContext implements AqlQueryContext {

public String executedAql;
public boolean dryRun;
public boolean showExecutedAql;
public boolean showExecutedSql;
public boolean showQueryPlan;

public Map<MetaProperty, Object> metaProperties = new HashMap<>();

public static TestAqlQueryContext create(Consumer<TestAqlQueryContext> block) {
TestAqlQueryContext queryContext = new TestAqlQueryContext();
block.accept(queryContext);
return queryContext;
}

@Override
public MetaData createMetaData(URI location) {
return null;
}

@Override
public boolean showExecutedAql() {
return showExecutedAql;
}

@Override
public boolean isDryRun() {
return dryRun;
}

@Override
public boolean showExecutedSql() {
return showExecutedSql;
}

@Override
public boolean showQueryPlan() {
return showQueryPlan;
}

@Override
public void setExecutedAql(String executedAql) {
this.executedAql = executedAql;
}

@Override
public void setMetaProperty(MetaProperty property, Object value) {
metaProperties.put(property, value);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
public class StatusServiceImp implements StatusService {

@SuppressWarnings("SpringJavaAutowiredFieldsWarningInspection")
@Autowired
@Autowired(required = false)
private BuildProperties buildProperties;

private final DSLContext dslContext;
Expand Down

0 comments on commit b82afd9

Please sign in to comment.