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.

Sign up for GitHub

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

Jump to bottom

Fix race condition in doesProjectExist #601

Merged
merged 2 commits into from
Feb 29, 2024
Merged

Fix race condition in doesProjectExist #601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a2dc4ab
Fix race condition in `doesProjectExist`
nscuro Feb 28, 2024
e8b8158
Fix failing tests due to new `UNIQUE` constraints
nscuro Feb 28, 2024
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
77 changes: 43 additions & 34 deletions src/main/java/org/dependencytrack/resources/v1/ProjectResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@
import java.util.function.BiConsumer;
import java.util.function.Function;

import static org.dependencytrack.util.PersistenceUtil.isUniqueConstraintViolation;

/**
* JAX-RS resources for processing projects.
*
Expand Down Expand Up @@ -248,21 +250,24 @@ public Response createProject(Project jsonProject) {
Project parent = qm.getObjectByUuid(Project.class, jsonProject.getParent().getUuid());
jsonProject.setParent(parent);
}
if (!qm.doesProjectExist(StringUtils.trimToNull(jsonProject.getName()), StringUtils.trimToNull(jsonProject.getVersion()))) {
final Project project;
try {
project = qm.createProject(jsonProject, jsonProject.getTags(), true);
} catch (IllegalArgumentException e) {
LOGGER.debug(e.getMessage());
return Response.status(Response.Status.CONFLICT).entity("An inactive Parent cannot be selected as parent").build();
final Project project;
try {
project = qm.createProject(jsonProject, jsonProject.getTags(), true);
} catch (IllegalArgumentException e) {
LOGGER.debug("Failed to create project %s".formatted(jsonProject), e);
return Response.status(Response.Status.CONFLICT).entity("An inactive Parent cannot be selected as parent").build();
} catch (RuntimeException e) {
if (isUniqueConstraintViolation(e)) {
return Response.status(Response.Status.CONFLICT).entity("A project with the specified name already exists.").build();
}
Principal principal = getPrincipal();
qm.updateNewProjectACL(project, principal);
LOGGER.info("Project " + project.toString() + " created by " + super.getPrincipal().getName());
return Response.status(Response.Status.CREATED).entity(project).build();
} else {
return Response.status(Response.Status.CONFLICT).entity("A project with the specified name already exists.").build();

LOGGER.error("Failed to create project %s".formatted(jsonProject), e);
return Response.status(Response.Status.INTERNAL_SERVER_ERROR).build();
}
Principal principal = getPrincipal();
qm.updateNewProjectACL(project, principal);
LOGGER.info("Project " + project.toString() + " created by " + super.getPrincipal().getName());
return Response.status(Response.Status.CREATED).entity(project).build();
}
}

Expand Down Expand Up @@ -303,24 +308,25 @@ public Response updateProject(Project jsonProject) {
return Response.status(Response.Status.FORBIDDEN).entity("Access to the specified project is forbidden").build();
}
final String name = StringUtils.trimToNull(jsonProject.getName());
final String version = StringUtils.trimToNull(jsonProject.getVersion());
final Project tmpProject = qm.getProject(name, version);
if (tmpProject == null || (tmpProject.getUuid().equals(project.getUuid()))) {
// Name cannot be empty or null - prevent it
if (name == null) {
jsonProject.setName(project.getName());
}
try {
project = qm.updateProject(jsonProject, true);
} catch (IllegalArgumentException e) {
LOGGER.debug(e.getMessage());
return Response.status(Response.Status.CONFLICT).entity(e.getMessage()).build();
// Name cannot be empty or null - prevent it
if (name == null) {
jsonProject.setName(project.getName());
}
try {
project = qm.updateProject(jsonProject, true);
} catch (IllegalArgumentException e) {
LOGGER.debug("Failed to update project %s".formatted(jsonProject.getUuid()), e);
return Response.status(Response.Status.CONFLICT).entity(e.getMessage()).build();
} catch (RuntimeException e) {
if (isUniqueConstraintViolation(e)) {
return Response.status(Response.Status.CONFLICT).entity("A project with the specified name and version already exists.").build();
}
LOGGER.info("Project " + project.toString() + " updated by " + super.getPrincipal().getName());
return Response.ok(project).build();
} else {
return Response.status(Response.Status.CONFLICT).entity("A project with the specified name and version already exists.").build();

LOGGER.error("Failed to update project %s".formatted(jsonProject.getUuid()), e);
return Response.status(Response.Status.INTERNAL_SERVER_ERROR).build();
}
LOGGER.info("Project " + project.toString() + " updated by " + super.getPrincipal().getName());
return Response.ok(project).build();
} else {
return Response.status(Response.Status.NOT_FOUND).entity("The UUID of the project could not be found.").build();
}
Expand Down Expand Up @@ -369,10 +375,6 @@ public Response patchProject(
project = qm.detachWithGroups(project, List.of(FetchGroup.DEFAULT, Project.FetchGroup.PARENT.name()));
modified |= setIfDifferent(jsonProject, project, Project::getName, Project::setName);
modified |= setIfDifferent(jsonProject, project, Project::getVersion, Project::setVersion);
// if either name or version has been changed, verify that this new combination does not already exist
if (modified && qm.doesProjectExist(project.getName(), project.getVersion())) {
return Response.status(Response.Status.CONFLICT).entity("A project with the specified name and version already exists.").build();
}
modified |= setIfDifferent(jsonProject, project, Project::getAuthor, Project::setAuthor);
modified |= setIfDifferent(jsonProject, project, Project::getPublisher, Project::setPublisher);
modified |= setIfDifferent(jsonProject, project, Project::getGroup, Project::setGroup);
Expand Down Expand Up @@ -407,8 +409,15 @@ public Response patchProject(
try {
project = qm.updateProject(project, true);
} catch (IllegalArgumentException e) {
LOGGER.debug(e.getMessage());
LOGGER.debug("Failed to patch project %s".formatted(uuid));
return Response.status(Response.Status.CONFLICT).entity(e.getMessage()).build();
} catch (RuntimeException e) {
if (isUniqueConstraintViolation(e)) {
return Response.status(Response.Status.CONFLICT).entity("A project with the specified name and version already exists.").build();
}

LOGGER.error("Failed to patch project %s".formatted(uuid), e);
return Response.status(Response.Status.INTERNAL_SERVER_ERROR).build();
}
LOGGER.info("Project " + project.toString() + " updated by " + super.getPrincipal().getName());
return Response.ok(project).build();
Expand Down
7 changes: 3 additions & 4 deletions src/main/java/org/dependencytrack/util/PersistenceUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,9 @@ public static <T, V> boolean applyIfChanged(final T existingObject, final T newO
}

public static boolean isUniqueConstraintViolation(final Throwable throwable) {
// TODO: DataNucleus doesn't map constraint violation exceptions very well,
// so we have to depend on the exception of the underlying JDBC driver to
// tell us what happened. We currently only handle PostgreSQL, but we'll have
// to do the same for at least H2 and MSSQL.
// NB: DataNucleus doesn't map constraint violation exceptions very well,
// so we have to depend on the exception of the underlying JDBC driver to
// tell us what happened.
return ExceptionUtils.getRootCause(throwable) instanceof final SQLException se
&& PSQLState.UNIQUE_VIOLATION.getState().equals(se.getSQLState());
}
Expand Down
15 changes: 15 additions & 0 deletions src/main/resources/migration/changelog-v5.4.0.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,19 @@
<column name="TOOLS" type="TEXT"/>
</addColumn>
</changeSet>

<changeSet id="v5.4.0-2" author="[email protected]">
<!--
Create two partial unique indexes to cover cases where VERSION is NULL.
Per default, NULLs are considered to be unique values.
PostgreSQL 15 supports NULLS NOT DISTINCT: https://stackoverflow.com/a/8289253
But we are far away from being able to raise the baseline version to 15.
-->
<sql splitStatements="true">
CREATE UNIQUE INDEX "PROJECT_NAME_VERSION_IDX" ON "PROJECT" ("NAME", "VERSION")
WHERE "VERSION" IS NOT NULL;
CREATE UNIQUE INDEX "PROJECT_NAME_VERSION_NULL_IDX" ON "PROJECT" ("NAME")
WHERE "VERSION" IS NULL;
</sql>
</changeSet>
</databaseChangeLog>
22 changes: 0 additions & 22 deletions src/test/java/org/dependencytrack/persistence/jdbi/JdbiTestUtil.java
View file
Open in desktop

This file was deleted.

Loading
Loading