From d0e7b4938392e13315e33890964eea621212f706 Mon Sep 17 00:00:00 2001 From: Niklas Date: Thu, 8 Aug 2024 11:27:40 +0200 Subject: [PATCH 1/2] Port: Gracefully handle `NotSortableException`s (#832) --- .../persistence/QueryManager.java | 55 ++++++-- .../exception/NotSortableExceptionMapper.java | 47 +++++++ .../NotSortableExceptionMapperTest.java | 118 ++++++++++++++++++ 3 files changed, 211 insertions(+), 9 deletions(-) create mode 100644 src/main/java/org/dependencytrack/resources/v1/exception/NotSortableExceptionMapper.java create mode 100644 src/test/java/org/dependencytrack/resources/v1/exception/NotSortableExceptionMapperTest.java diff --git a/src/main/java/org/dependencytrack/persistence/QueryManager.java b/src/main/java/org/dependencytrack/persistence/QueryManager.java index 4beb70fb3..5a9592da1 100644 --- a/src/main/java/org/dependencytrack/persistence/QueryManager.java +++ b/src/main/java/org/dependencytrack/persistence/QueryManager.java @@ -27,7 +27,9 @@ import alpine.model.Team; import alpine.model.UserPrincipal; import alpine.notification.NotificationLevel; +import alpine.persistence.AbstractAlpineQueryManager; import alpine.persistence.AlpineQueryManager; +import alpine.persistence.NotSortableException; import alpine.persistence.OrderDirection; import alpine.persistence.PaginatedResult; import alpine.persistence.ScopedCustomization; @@ -105,13 +107,15 @@ import javax.jdo.Query; import javax.jdo.Transaction; import javax.jdo.datastore.JDOConnection; -import java.lang.reflect.Field; +import javax.jdo.metadata.MemberMetadata; +import javax.jdo.metadata.TypeMetadata; import java.security.Principal; import java.sql.Connection; import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.SQLException; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.Date; @@ -201,8 +205,18 @@ public QueryManager(final PersistenceManager pm, final AlpineRequest request) { this.request = request; } + /** + * Override of {@link AbstractAlpineQueryManager#decorate(Query)} to modify the + * method's behavior such that it always sorts by ID, in addition to whatever field + * is requested to be sorted by via {@link #orderBy}. + *

+ * This is to ensure stable ordering in case {@link #orderBy} refers to a field that + * allows duplicates. + * + * @since 5.2.0 + */ @Override - public Query decorate(final Query query) { + public Query decorate(final Query query) { // Clear the result to fetch if previously specified (i.e. by getting count) query.setResult(null); if (pagination != null && pagination.isPaginated()) { @@ -213,16 +227,39 @@ public Query decorate(final Query query) { if (orderBy != null && RegexSequence.Pattern.STRING_IDENTIFIER.matcher(orderBy).matches() && orderDirection != OrderDirection.UNSPECIFIED) { // Check to see if the specified orderBy field is defined in the class being queried. boolean found = false; - final org.datanucleus.store.query.Query iq = ((JDOQuery) query).getInternalQuery(); + // NB: Only persistent fields can be used as sorting subject. + final org.datanucleus.store.query.Query iq = ((JDOQuery) query).getInternalQuery(); final String candidateField = orderBy.contains(".") ? orderBy.substring(0, orderBy.indexOf('.')) : orderBy; - for (final Field field : iq.getCandidateClass().getDeclaredFields()) { - if (candidateField.equals(field.getName())) { - found = true; + final TypeMetadata candidateTypeMetadata = pm.getPersistenceManagerFactory().getMetadata(iq.getCandidateClassName()); + if (candidateTypeMetadata == null) { + // NB: If this happens then the entire query is broken and needs programmatic fixing. + // Throwing an exception here to make this painfully obvious. + throw new IllegalStateException(""" + Persistence type metadata for candidate class %s could not be found. \ + Querying for non-persistent types is not supported, correct your query.\ + """.formatted(iq.getCandidateClassName())); + } + boolean foundPersistentMember = false; + for (final MemberMetadata memberMetadata : candidateTypeMetadata.getMembers()) { + if (candidateField.equals(memberMetadata.getName())) { + foundPersistentMember = true; break; } } - if (found) { + if (foundPersistentMember) { + // NB: Changed from AbstractAlpineQueryManager#decorate to always sort by ID. query.setOrdering(orderBy + " " + orderDirection.name().toLowerCase() + ", id asc"); + } else { + // Is it a non-persistent (transient) field? + final boolean foundNonPersistentMember = Arrays.stream(iq.getCandidateClass().getDeclaredFields()) + .anyMatch(field -> field.getName().equals(candidateField)); + if (foundNonPersistentMember) { + throw new NotSortableException(iq.getCandidateClass().getSimpleName(), candidateField, + "The field is computed and can not be queried or sorted by"); + } + + throw new NotSortableException(iq.getCandidateClass().getSimpleName(), candidateField, + "The field does not exist"); } } return query; @@ -1470,7 +1507,7 @@ public List getObjectsById(final Class clazz, final Collection i * even the default one. If inclusion of the default fetch group is desired, it must be * included in {@code fetchGroups} explicitly. *

- * Eventually, this may be moved to {@link alpine.persistence.AbstractAlpineQueryManager}. + * Eventually, this may be moved to {@link AbstractAlpineQueryManager}. * * @param object The persistent object to detach * @param fetchGroups Fetch groups to use for this operation @@ -1529,7 +1566,7 @@ public Query getObjectsByUuidsQuery(final Class clazz, final List - * Eventually, this may be moved to {@link alpine.persistence.AbstractAlpineQueryManager}. + * Eventually, this may be moved to {@link AbstractAlpineQueryManager}. * * @param clazz Class of the object to fetch * @param uuid {@link UUID} of the object to fetch diff --git a/src/main/java/org/dependencytrack/resources/v1/exception/NotSortableExceptionMapper.java b/src/main/java/org/dependencytrack/resources/v1/exception/NotSortableExceptionMapper.java new file mode 100644 index 000000000..0f96e0c9b --- /dev/null +++ b/src/main/java/org/dependencytrack/resources/v1/exception/NotSortableExceptionMapper.java @@ -0,0 +1,47 @@ +/* + * This file is part of Dependency-Track. + * + * 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 + * + * http://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. + * + * SPDX-License-Identifier: Apache-2.0 + * Copyright (c) OWASP Foundation. All Rights Reserved. + */ +package org.dependencytrack.resources.v1.exception; + +import alpine.persistence.NotSortableException; +import jakarta.ws.rs.core.Response; +import jakarta.ws.rs.ext.ExceptionMapper; +import jakarta.ws.rs.ext.Provider; +import org.dependencytrack.resources.v1.problems.ProblemDetails; + +/** + * @since 4.12.0 + */ +@Provider +public class NotSortableExceptionMapper implements ExceptionMapper { + + @Override + public Response toResponse(final NotSortableException exception) { + final var problemDetails = new ProblemDetails(); + problemDetails.setStatus(400); + problemDetails.setTitle("Field not sortable"); + problemDetails.setDetail(exception.getMessage()); + + return Response + .status(Response.Status.BAD_REQUEST) + .type(ProblemDetails.MEDIA_TYPE_JSON) + .entity(problemDetails) + .build(); + } + +} \ No newline at end of file diff --git a/src/test/java/org/dependencytrack/resources/v1/exception/NotSortableExceptionMapperTest.java b/src/test/java/org/dependencytrack/resources/v1/exception/NotSortableExceptionMapperTest.java new file mode 100644 index 000000000..1e6630014 --- /dev/null +++ b/src/test/java/org/dependencytrack/resources/v1/exception/NotSortableExceptionMapperTest.java @@ -0,0 +1,118 @@ +/* + * This file is part of Dependency-Track. + * + * 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 + * + * http://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. + * + * SPDX-License-Identifier: Apache-2.0 + * Copyright (c) OWASP Foundation. All Rights Reserved. + */ +package org.dependencytrack.resources.v1.exception; + +import alpine.persistence.PaginatedResult; +import alpine.server.auth.AuthenticationNotRequired; +import alpine.server.filters.ApiFilter; +import alpine.server.resources.AlpineResource; +import jakarta.ws.rs.GET; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.Produces; +import jakarta.ws.rs.core.MediaType; +import jakarta.ws.rs.core.Response; +import org.dependencytrack.JerseyTestRule; +import org.dependencytrack.ResourceTest; +import org.dependencytrack.persistence.QueryManager; +import org.glassfish.jersey.server.ResourceConfig; +import org.junit.ClassRule; +import org.junit.Test; + +import static net.javacrumbs.jsonunit.assertj.JsonAssertions.assertThatJson; +import static org.assertj.core.api.Assertions.assertThat; + +public class NotSortableExceptionMapperTest extends ResourceTest { + + @ClassRule + public static JerseyTestRule jersey = new JerseyTestRule( + new ResourceConfig(TestResource.class) + .register(ApiFilter.class) + .register(NotSortableExceptionMapper.class)); + + @Test + public void testFieldDoesNotExist() { + final Response response = jersey.target("/") + .queryParam("sortName", "foo") + .queryParam("sortOrder", "asc") + .request() + .get(); + assertThat(response.getStatus()).isEqualTo(400); + assertThat(response.getHeaderString("Content-Type")).isEqualTo("application/problem+json"); + assertThatJson(getPlainTextBody(response)) + .isEqualTo(""" + { + "status": 400, + "title": "Field not sortable", + "detail": "Can not sort by Project#foo: The field does not exist" + } + """); + } + + @Test + public void testTransientField() { + final Response response = jersey.target("/") + .queryParam("sortName", "bomRef") + .queryParam("sortOrder", "asc") + .request() + .get(); + assertThat(response.getStatus()).isEqualTo(400); + assertThat(response.getHeaderString("Content-Type")).isEqualTo("application/problem+json"); + assertThatJson(getPlainTextBody(response)) + .isEqualTo(""" + { + "status": 400, + "title": "Field not sortable", + "detail": "Can not sort by Project#bomRef: The field is computed and can not be queried or sorted by" + } + """); + } + + @Test + public void testPersistentField() { + final Response response = jersey.target("/") + .queryParam("sortName", "name") + .queryParam("sortOrder", "asc") + .request() + .get(); + assertThat(response.getStatus()).isEqualTo(200); + assertThat(response.getHeaderString("Content-Type")).isEqualTo("application/json"); + assertThatJson(getPlainTextBody(response)) + .isEqualTo("[]"); + } + + @Path("/") + public static class TestResource extends AlpineResource { + + @GET + @Produces(MediaType.APPLICATION_JSON) + @AuthenticationNotRequired + public Response get() { + try (final var qm = new QueryManager(getAlpineRequest())) { + final PaginatedResult projects = qm.getProjects(); + return Response + .status(Response.Status.OK) + .header("X-Total-Count", projects.getTotal()) + .entity(projects.getObjects()) + .build(); + } + } + + } + +} \ No newline at end of file From a1b876dfd1786b3c7671aa3b4948624cea6f1566 Mon Sep 17 00:00:00 2001 From: Niklas Date: Thu, 8 Aug 2024 14:09:46 +0200 Subject: [PATCH 2/2] Bump Redpanda to v24.2.2 (#837) --- src/main/java/org/dependencytrack/common/ConfigKey.java | 2 +- src/main/resources/application.properties | 2 +- .../event/kafka/processor/api/ProcessorManagerTest.java | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/dependencytrack/common/ConfigKey.java b/src/main/java/org/dependencytrack/common/ConfigKey.java index 65e16acbf..0b7116812 100644 --- a/src/main/java/org/dependencytrack/common/ConfigKey.java +++ b/src/main/java/org/dependencytrack/common/ConfigKey.java @@ -102,7 +102,7 @@ public enum ConfigKey implements Config.Key { DEV_SERVICES_ENABLED("dev.services.enabled", false), DEV_SERVICES_IMAGE_FRONTEND("dev.services.image.frontend", "ghcr.io/dependencytrack/hyades-frontend:snapshot"), - DEV_SERVICES_IMAGE_KAFKA("dev.services.image.kafka", "docker.redpanda.com/vectorized/redpanda:v24.2.1"), + DEV_SERVICES_IMAGE_KAFKA("dev.services.image.kafka", "docker.redpanda.com/vectorized/redpanda:v24.2.2"), DEV_SERVICES_IMAGE_POSTGRES("dev.services.image.postgres", "postgres:16"); private final String propertyName; diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index b4d52fabf..57d90f4d8 100644 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -1302,7 +1302,7 @@ dev.services.image.frontend=ghcr.io/dependencytrack/hyades-frontend:snapshot # # @category: Development # @type: string -dev.services.image.kafka=docker.redpanda.com/vectorized/redpanda:v24.2.1 +dev.services.image.kafka=docker.redpanda.com/vectorized/redpanda:v24.2.2 # The image to use for the PostgreSQL dev services container. # diff --git a/src/test/java/org/dependencytrack/event/kafka/processor/api/ProcessorManagerTest.java b/src/test/java/org/dependencytrack/event/kafka/processor/api/ProcessorManagerTest.java index a8d2c50c7..210775a12 100644 --- a/src/test/java/org/dependencytrack/event/kafka/processor/api/ProcessorManagerTest.java +++ b/src/test/java/org/dependencytrack/event/kafka/processor/api/ProcessorManagerTest.java @@ -61,7 +61,7 @@ public class ProcessorManagerTest { @Rule public RedpandaContainer kafkaContainer = new RedpandaContainer(DockerImageName - .parse("docker.redpanda.com/vectorized/redpanda:v24.2.1")); + .parse("docker.redpanda.com/vectorized/redpanda:v24.2.2")); private AdminClient adminClient; private Producer producer;