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

ConstraintViolationException HTTP status should be configurable #313

Merged
merged 2 commits into from
Oct 29, 2023
Merged

ConstraintViolationException HTTP status should be configurable #313

merged 2 commits into from
Oct 29, 2023

Conversation

lwitkowski
Copy link
Collaborator

@lwitkowski lwitkowski commented Oct 27, 2023 •
edited
Loading

Turns out HttpProblem relied on Response.Status a bit too much, which is being addressed in #314

Closes #273

Base automatically changed from feat/post-processor-devex-improvements to master October 28, 2023 08:54
@lwitkowski lwitkowski force-pushed the feat/273-constraint-violation-status-config branch from 0c5e8c4 to 021d5de Compare October 28, 2023 08:56
@lwitkowski lwitkowski changed the base branch from master to feat/response-status-decoupling October 28, 2023 08:56
@lwitkowski lwitkowski requested a review from pazkooda October 28, 2023 08:59
@lwitkowski
Copy link
Collaborator Author

@pazkooda when you have time, you could have a look at this one after #314

@lwitkowski lwitkowski marked this pull request as ready for review October 28, 2023 09:09
@pazkooda
Copy link
Collaborator

Yup, but it will be later today (evening/night) at the earliest.

lwitkowski
lwitkowski commented Oct 28, 2023
View reviewed changes
README.md Outdated Show resolved Hide resolved
@lwitkowski lwitkowski mentioned this pull request Oct 28, 2023
Open
pazkooda
pazkooda approved these changes Oct 29, 2023
View reviewed changes
Copy link
Collaborator

@pazkooda pazkooda left a comment

Choose a reason for hiding this comment

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

LGTM

Base automatically changed from feat/response-status-decoupling to master October 29, 2023 15:27
@lwitkowski lwitkowski force-pushed the feat/273-constraint-violation-status-config branch from c2e7151 to ce0ffd2 Compare October 29, 2023 15:28
@lwitkowski lwitkowski merged commit 129efd2 into master Oct 29, 2023
6 checks passed
@lwitkowski lwitkowski deleted the feat/273-constraint-violation-status-config branch October 29, 2023 15:32
chberger
chberger reviewed Oct 30, 2023
View reviewed changes
Copy link
Contributor

@chberger chberger left a comment

Choose a reason for hiding this comment

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

I'm leaving my comments here as well, I hope that's ok 😊

README.md

- 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.

runtime/src/main/java/com/tietoevry/quarkus/resteasy/problem/ProblemRuntimeConfig.java

@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.

...va/com/tietoevry/quarkus/resteasy/problem/validation/ConstraintViolationExceptionMapper.java Show resolved Hide resolved
@chberger chberger mentioned this pull request Oct 30, 2023
Open
@lwitkowski
Copy link
Collaborator Author

I'm leaving my comments here as well, I hope that's ok 😊

That's more than ok, really appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chberger chberger chberger left review comments

@pazkooda pazkooda pazkooda approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ConstraintViolationException HTTP status should be configurable
3 participants
@lwitkowski @pazkooda @chberger