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

ConstraintViolationException HTTP status should be configurable #313

Merged
merged 2 commits into from
Oct 29, 2023
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
24 changes: 20 additions & 4 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -181,9 +181,8 @@ You may also want to check [this article](https://dzone.com/articles/when-http-s
More on throwing problems: [zalando/problem usage](https://github.com/zalando/problem#usage)

## Configuration options
All configuration options are build-time properties, meaning that you cannot override them in the runtime (i.e via environment variables).

- Include MDC properties in the API response (you have to provide those properties to MDC using `MDC.put`)
- (Build time) Include MDC properties in the API response. You have to provide those properties to MDC using `MDC.put`
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's worth to mention that this extension uses org.slf4j instead of jboss logging. Thus a user has to pick the appropriated MDC ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. On the other hand, quick tests show that setting properties via org.jboss.logging.MDC just works, jboss probably propagates everything to slf4j (or is a service provider etc). I'll double check that in real app though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah cool, wasn't aware of that. Thanks for pointing this out.

```
quarkus.resteasy.problem.include-mdc-properties=uuid,application,version
```
Expand All @@ -198,7 +197,24 @@ Result:
}
```

- Enable Smallrye (Microprofile) metrics for http error counters. Requires `quarkus-smallrye-metrics` in the classpath.
- (Runtime) Changes default `400 Bad request` response status when `ConstraintViolationException` is thrown (e.g. by Hibernate Validator)
```
quarkus.resteasy.problem.constraint-violation.status=422
quarkus.resteasy.problem.constraint-violation.title=Constraint violation
```
Result:
```json
HTTP/1.1 422 Unprocessable Entity
Content-Type: application/problem+json

{
"status": 422,
"title": "Constraint violation",
(...)
}
```

- (Build time) Enable Smallrye (Microprofile) metrics for http error counters. Requires `quarkus-smallrye-metrics` in the classpath.

Please note that if you use `quarkus-micrometer-registry-prometheus` you don't need this feature - http error metrics will be produced regardless of this setting or presence of this extension.

Expand All @@ -212,7 +228,7 @@ application_http_error_total{status="401"} 3.0
application_http_error_total{status="500"} 5.0
```

- Tuning logging
- (Runtime) Tuning logging
```
quarkus.log.category.http-problem.level=INFO # default: all problems are logged
quarkus.log.category.http-problem.level=ERROR # only HTTP 5XX problems are logged
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import static io.quarkus.deployment.annotations.ExecutionTime.RUNTIME_INIT;
import static io.quarkus.deployment.annotations.ExecutionTime.STATIC_INIT;

import com.tietoevry.quarkus.resteasy.problem.ProblemRuntimeConfig;
import com.tietoevry.quarkus.resteasy.problem.postprocessing.ProblemRecorder;
import io.quarkus.arc.deployment.AdditionalBeanBuildItem;
import io.quarkus.deployment.Capabilities;
Expand Down Expand Up @@ -168,6 +169,12 @@ void registerCustomPostProcessors(ProblemRecorder recorder) {
recorder.registerCustomPostProcessors();
}

@Record(RUNTIME_INIT)
@BuildStep
void applyRuntimeConfig(ProblemRecorder recorder, ProblemRuntimeConfig config) {
recorder.applyRuntimeConfig(config);
}

protected Logger logger() {
return LoggerFactory.getLogger(FEATURE_NAME);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package com.tietoevry.quarkus.resteasy.problem;

import io.quarkus.test.junit.QuarkusTest;
import io.quarkus.test.junit.QuarkusTestProfile;
import io.quarkus.test.junit.TestProfile;
import io.restassured.RestAssured;
import org.junit.jupiter.api.Test;

import java.util.Map;

import static io.restassured.RestAssured.given;
import static jakarta.ws.rs.core.MediaType.APPLICATION_JSON;
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.Matchers.hasSize;

@QuarkusTest
@TestProfile(ConstraintViolationMapperConfigIT.CustomHttpStatus.class)
class ConstraintViolationMapperConfigIT {

static {
RestAssured.enableLoggingOfRequestAndResponseIfValidationFails();
}

@Test
void constraintViolationShouldProvideErrorDetails() {
given()
.body("{\"phraseName\": 10 }")
.contentType(APPLICATION_JSON)
.post("/throw/validation/constraint-violation-exception")
.then()
.statusCode(422)
.body("title", equalTo("Constraint violation"))
.body("status", equalTo(422))
.body("violations", hasSize(1));
}

public static class CustomHttpStatus implements QuarkusTestProfile {

@Override
public Map<String, String> getConfigOverrides() {
return Map.of(
"quarkus.resteasy.problem.constraint-violation.status", "422",
"quarkus.resteasy.problem.constraint-violation.title", "Constraint violation"
);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package com.tietoevry.quarkus.resteasy.problem;

import io.quarkus.test.junit.QuarkusIntegrationTest;

@QuarkusIntegrationTest
class ConstraintViolationMapperConfigNativeIT extends ConstraintViolationMapperConfigIT {
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package com.tietoevry.quarkus.resteasy.problem;

import io.quarkus.runtime.annotations.ConfigPhase;
import io.quarkus.runtime.annotations.ConfigRoot;
import io.smallrye.config.ConfigMapping;
import io.smallrye.config.WithDefault;
import io.smallrye.config.WithName;

@ConfigMapping(prefix = "quarkus.resteasy.problem")
@ConfigRoot(phase = ConfigPhase.RUN_TIME)
public interface ProblemRuntimeConfig {

/**
* Config for ConstraintViolationException mapper
*/
@WithName("constraint-violation")
ConstraintViolationMapperConfig constraintViolation();

interface ConstraintViolationMapperConfig {
static ConstraintViolationMapperConfig defaults() {
return new ConstraintViolationMapperConfig() {
@Override
public int status() {
return 400;
}

@Override
public String title() {
return "Bad Request";
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's worth to define a constant to keep the default values aligned.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I didn't like this defaults() method that much from the very beginning, will try to simplify this.

}
};
}

/**
* Response status code when ConstraintViolationException is thrown.
*/
@WithDefault("400")
int status();

/**
* Response title when ConstraintViolationException is thrown.
*/
@WithDefault("Bad Request")
String title();
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package com.tietoevry.quarkus.resteasy.problem.postprocessing;

import com.tietoevry.quarkus.resteasy.problem.ExceptionMapperBase;
import com.tietoevry.quarkus.resteasy.problem.ProblemRuntimeConfig;
import com.tietoevry.quarkus.resteasy.problem.validation.ConstraintViolationExceptionMapper;
import io.quarkus.runtime.annotations.Recorder;
import jakarta.enterprise.inject.spi.CDI;
import java.util.Set;
Expand Down Expand Up @@ -30,4 +32,7 @@ public void registerCustomPostProcessors() {
.forEach(ExceptionMapperBase.postProcessorsRegistry::register);
}

public void applyRuntimeConfig(ProblemRuntimeConfig config) {
ConstraintViolationExceptionMapper.configure(config.constraintViolation());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import com.tietoevry.quarkus.resteasy.problem.ExceptionMapperBase;
import com.tietoevry.quarkus.resteasy.problem.HttpProblem;
import com.tietoevry.quarkus.resteasy.problem.ProblemRuntimeConfig.ConstraintViolationMapperConfig;
import jakarta.annotation.Priority;
import jakarta.validation.ConstraintViolation;
import jakarta.validation.ConstraintViolationException;
Expand All @@ -13,7 +14,6 @@
import jakarta.ws.rs.QueryParam;
import jakarta.ws.rs.container.ResourceInfo;
import jakarta.ws.rs.core.Context;
import jakarta.ws.rs.core.Response;
import java.lang.reflect.Method;
import java.lang.reflect.Parameter;
import java.util.ArrayList;
Expand All @@ -31,14 +31,20 @@
@Priority(Priorities.USER)
lwitkowski marked this conversation as resolved.
Show resolved Hide resolved
public final class ConstraintViolationExceptionMapper extends ExceptionMapperBase<ConstraintViolationException> {

private static ConstraintViolationMapperConfig config = ConstraintViolationMapperConfig.defaults();

@Context
ResourceInfo resourceInfo;

public static void configure(ConstraintViolationMapperConfig config) {
ConstraintViolationExceptionMapper.config = config;
}

@Override
protected HttpProblem toProblem(ConstraintViolationException exception) {
return HttpProblem.builder()
.withStatus(Response.Status.BAD_REQUEST)
.withTitle(Response.Status.BAD_REQUEST.getReasonPhrase())
.withStatus(config.status())
.withTitle(config.title())
.with("violations", toViolations(exception.getConstraintViolations()))
.build();
}
Expand Down