Skip to content
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.

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

data-management-api: validate query parameters #1258

Merged
merged 3 commits into from
May 10, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ in the detailed section referring to by linking pull requests or issues.
* Check to avoid duplicated module names (#1190)
* Implement Catalog service for Data Management API (#1195)
* Add strict body validation for REST endpoints (#1128)
* Add validation on query endpoints (#1258)
* Dependency injection using factory/provider methods (#1056)
* Provisioned resource information in Data Management API (#1221)
* Add custom Jackson (de)serializer for `XMLGregorianCalendar` (#1226)
Expand Down
2 changes: 1 addition & 1 deletion build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ plugins {
jacoco
id("com.rameshkp.openapi-merger-gradle-plugin") version "1.0.4"
id("org.eclipse.dataspaceconnector.module-names")
id("com.autonomousapps.dependency-analysis") version "1.0.0-rc05" apply (false)
id("com.autonomousapps.dependency-analysis") version "1.1.0" apply (false)
}

repositories {
Expand Down
10 changes: 6 additions & 4 deletions extensions/api/api-core/build.gradle.kts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2021 Daimler TSS GmbH
* Copyright (c) 2021 - 2022 Daimler TSS GmbH
*
* This program and the accompanying materials are made available under the
* terms of the Apache License, Version 2.0 which is available at
Expand All @@ -9,13 +9,13 @@
*
* Contributors:
* Daimler TSS GmbH - Initial API and Implementation
* Bayerische Motoren Werke Aktiengesellschaft (BMW AG) - improvements
*
*/


val infoModelVersion: String by project
val rsApi: String by project
val jakartaValidationApi: String by project
val jerseyVersion: String by project
val rsApi: String by project

plugins {
`java-library`
Expand All @@ -27,11 +27,13 @@ dependencies {

implementation(project(":common:util"))
implementation("jakarta.ws.rs:jakarta.ws.rs-api:${rsApi}")
implementation("jakarta.validation:jakarta.validation-api:${jakartaValidationApi}")

testImplementation("org.glassfish.jersey.core:jersey-common:${jerseyVersion}")
testImplementation("org.glassfish.jersey.core:jersey-server:${jerseyVersion}")

testImplementation(testFixtures(project(":launchers:junit")))
testRuntimeOnly("org.glassfish.jersey.ext:jersey-bean-validation:${jerseyVersion}") //for validation
}

publishing {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import org.eclipse.dataspaceconnector.api.transformer.DtoTransformerRegistry;
import org.eclipse.dataspaceconnector.api.transformer.DtoTransformerRegistryImpl;
import org.eclipse.dataspaceconnector.api.transformer.QuerySpecDtoToQuerySpecTransformer;
import org.eclipse.dataspaceconnector.spi.system.Provides;
import org.eclipse.dataspaceconnector.spi.system.ServiceExtension;
import org.eclipse.dataspaceconnector.spi.system.ServiceExtensionContext;
Expand All @@ -30,6 +31,8 @@ public String name() {

@Override
public void initialize(ServiceExtensionContext context) {
context.registerService(DtoTransformerRegistry.class, new DtoTransformerRegistryImpl());
var transformerRegistry = new DtoTransformerRegistryImpl();
transformerRegistry.register(new QuerySpecDtoToQuerySpecTransformer());
context.registerService(DtoTransformerRegistry.class, transformerRegistry);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
/*
* Copyright (c) 2022 Bayerische Motoren Werke Aktiengesellschaft (BMW AG)
*
* This program and the accompanying materials are made available under the
* terms of the Apache License, Version 2.0 which is available at
* https://www.apache.org/licenses/LICENSE-2.0
*
* SPDX-License-Identifier: Apache-2.0
*
* Contributors:
* Bayerische Motoren Werke Aktiengesellschaft (BMW AG) - initial API and implementation
*
*/

package org.eclipse.dataspaceconnector.api.query;

import jakarta.validation.constraints.AssertTrue;
import jakarta.validation.constraints.Positive;
import jakarta.validation.constraints.PositiveOrZero;
import jakarta.ws.rs.QueryParam;
import org.eclipse.dataspaceconnector.spi.query.SortOrder;

public class QuerySpecDto {

@QueryParam("offset")
@PositiveOrZero
private Integer offset = 0;

@QueryParam("limit")
@Positive
private Integer limit = 50;

@QueryParam("filter")
private String filter;

@QueryParam("sort")
private SortOrder sortOrder = SortOrder.ASC;

@QueryParam("sortField")
private String sortField;

public QuerySpecDto() {

}

public Integer getOffset() {
return offset;
}

public Integer getLimit() {
return limit;
}

public String getFilter() {
return filter;
}

public SortOrder getSortOrder() {
return sortOrder;
}

public String getSortField() {
return sortField;
}

@AssertTrue
Copy link
Member

@paullatzelsperger paullatzelsperger May 5, 2022

Choose a reason for hiding this comment

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

This needs to have the @JsonIgnore annotation, otherwise Jackson will look for a valid field. A serialization test would have surfaced that. Mind adding one?

For example:

    @Test
    void verifySerialization() throws JsonProcessingException {

        var dto = QuerySpecDto.Builder.newInstance()
                .filter("some filter")
                .limit(10)
                .offset(2)
                .build();

        var mapper = new ObjectMapper();
        var json = mapper.writeValueAsString(dto);

        assertThat(json).isNotNull();
        var d = mapper.readValue(json, QuerySpecDto.class);
        assertThat(d).usingRecursiveComparison().isEqualTo(dto);
    }

Copy link
Member Author

Choose a reason for hiding this comment

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

This class is never serialized to/deserialized from json, so it's not needed

Copy link
Member

Choose a reason for hiding this comment

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

I haven't checked, but what about tests (integration, e2e)?

Copy link
Member Author

@ndr-brt ndr-brt May 9, 2022

Choose a reason for hiding this comment

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

tests are working just fine without touching them, I just add a "failing query" test for every controller

Copy link
Member

Choose a reason for hiding this comment

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

sure, that'll work, but as far as I'm aware @algattik has plans to generate a java API client from our own OpenAPI spec and use that in tests. If that is the case we'd indeed have to serialize the QuerySpecDto.

Copy link
Member Author

Choose a reason for hiding this comment

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

@paullatzelsperger I'm not sure about that, however I would leave this for that issue, if it's needed it will be done

Copy link
Member

Choose a reason for hiding this comment

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

ok, fine for me.

public boolean isValid() {
if (filter != null && filter.isBlank()) {
return false;
}

if (sortField != null && sortField.isBlank()) {
return false;
}

return true;
}

public static final class Builder {
private final QuerySpecDto querySpec;

private Builder() {
querySpec = new QuerySpecDto();
}

public static Builder newInstance() {
return new Builder();
}

public Builder offset(Integer offset) {
querySpec.offset = offset;
return this;
}

public Builder limit(Integer limit) {
querySpec.limit = limit;
return this;
}

public Builder sortOrder(SortOrder sortOrder) {
querySpec.sortOrder = sortOrder;
return this;
}

public Builder sortField(String sortField) {
querySpec.sortField = sortField;
return this;
}

public Builder filter(String filter) {
querySpec.filter = filter;
return this;
}

public QuerySpecDto build() {
return querySpec;
}

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* Copyright (c) 2022 Bayerische Motoren Werke Aktiengesellschaft (BMW AG)
*
* This program and the accompanying materials are made available under the
* terms of the Apache License, Version 2.0 which is available at
* https://www.apache.org/licenses/LICENSE-2.0
*
* SPDX-License-Identifier: Apache-2.0
*
* Contributors:
* Bayerische Motoren Werke Aktiengesellschaft (BMW AG) - initial API and implementation
*
*/

package org.eclipse.dataspaceconnector.api.transformer;

import org.eclipse.dataspaceconnector.api.query.QuerySpecDto;
import org.eclipse.dataspaceconnector.spi.query.QuerySpec;
import org.eclipse.dataspaceconnector.spi.transformer.TransformerContext;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

public class QuerySpecDtoToQuerySpecTransformer implements DtoTransformer<QuerySpecDto, QuerySpec> {

@Override
public Class<QuerySpecDto> getInputType() {
return QuerySpecDto.class;
}

@Override
public Class<QuerySpec> getOutputType() {
return QuerySpec.class;
}

@Override
public @Nullable QuerySpec transform(@Nullable QuerySpecDto object, @NotNull TransformerContext context) {
return QuerySpec.Builder.newInstance()
.limit(object.getLimit())
.offset(object.getOffset())
.filter(object.getFilter())
.sortField(object.getSortField())
.sortOrder(object.getSortOrder())
.build();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
/*
* Copyright (c) 2022 Bayerische Motoren Werke Aktiengesellschaft (BMW AG)
*
* This program and the accompanying materials are made available under the
* terms of the Apache License, Version 2.0 which is available at
* https://www.apache.org/licenses/LICENSE-2.0
*
* SPDX-License-Identifier: Apache-2.0
*
* Contributors:
* Bayerische Motoren Werke Aktiengesellschaft (BMW AG) - initial API and implementation
*
*/

package org.eclipse.dataspaceconnector.api.query;

import jakarta.validation.Validation;
import jakarta.validation.Validator;
import jakarta.validation.ValidatorFactory;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import static org.assertj.core.api.Assertions.assertThat;

class QuerySpecDtoValidationTest {

private Validator validator;

@BeforeEach
void setUp() {
try (ValidatorFactory factory = Validation.buildDefaultValidatorFactory()) {
validator = factory.getValidator();
}
}

@Test
void defaultQuerySpecIsValid() {
var querySpec = QuerySpecDto.Builder.newInstance().build();

var result = validator.validate(querySpec);

assertThat(result).isEmpty();
}

@Test
void limitShouldBeGreaterThanZero() {
var querySpec = QuerySpecDto.Builder.newInstance().limit(0).build();

var result = validator.validate(querySpec);

assertThat(result).isNotEmpty();
}

@Test
void offsetShouldBeGreaterOrEqualThanZero() {
var querySpec = QuerySpecDto.Builder.newInstance().offset(-1).build();

var result = validator.validate(querySpec);

assertThat(result).isNotEmpty();
}

@Test
void filterShouldNotBeBlank() {
var querySpec = QuerySpecDto.Builder.newInstance().filter(" ").build();

var result = validator.validate(querySpec);

assertThat(result).isNotEmpty();
}

@Test
void sortFieldShouldNotBeBlank() {
var querySpec = QuerySpecDto.Builder.newInstance().sortField(" ").build();

var result = validator.validate(querySpec);

assertThat(result).isNotEmpty();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/*
* Copyright (c) 2022 Bayerische Motoren Werke Aktiengesellschaft (BMW AG)
*
* This program and the accompanying materials are made available under the
* terms of the Apache License, Version 2.0 which is available at
* https://www.apache.org/licenses/LICENSE-2.0
*
* SPDX-License-Identifier: Apache-2.0
*
* Contributors:
* Bayerische Motoren Werke Aktiengesellschaft (BMW AG) - initial API and implementation
*
*/

package org.eclipse.dataspaceconnector.api.transformer;

import org.eclipse.dataspaceconnector.api.query.QuerySpecDto;
import org.eclipse.dataspaceconnector.spi.query.Criterion;
import org.eclipse.dataspaceconnector.spi.query.SortOrder;
import org.eclipse.dataspaceconnector.spi.transformer.TransformerContext;
import org.junit.jupiter.api.Test;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.mock;

class QuerySpecDtoToQuerySpecTransformerTest {

private final QuerySpecDtoToQuerySpecTransformer transformer = new QuerySpecDtoToQuerySpecTransformer();

@Test
void inputOutputType() {
assertThat(transformer.getInputType()).isNotNull();
assertThat(transformer.getOutputType()).isNotNull();
}

@Test
void transform() {
var context = mock(TransformerContext.class);
var querySpecDto = QuerySpecDto.Builder.newInstance()
.offset(10)
.limit(20)
.filter("field=value")
.sortOrder(SortOrder.DESC)
.sortField("field")
.build();

var spec = transformer.transform(querySpecDto, context);

assertThat(spec.getOffset()).isEqualTo(10);
assertThat(spec.getLimit()).isEqualTo(20);
assertThat(spec.getFilterExpression()).hasSize(1)
.containsExactly(new Criterion("field", "=", "value"));
assertThat(spec.getSortOrder()).isEqualTo(SortOrder.DESC);
assertThat(spec.getSortField()).isEqualTo("field");
}

@Test
void transform_defaultValues() {
var context = mock(TransformerContext.class);
var querySpecDto = QuerySpecDto.Builder.newInstance().build();

var spec = transformer.transform(querySpecDto, context);

assertThat(spec.getOffset()).isEqualTo(0);
assertThat(spec.getLimit()).isEqualTo(50);
assertThat(spec.getFilterExpression()).hasSize(0);
assertThat(spec.getSortOrder()).isEqualTo(SortOrder.ASC);
assertThat(spec.getSortField()).isNull();
}

}
Loading