Skip to content

Patching for Issue 143 #29

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Patching for Issue 143 #29

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
64f3470
Patching for Issue 143
Jan 5, 2016
8814372
Changed in response to the comments given in the pull request.
Jan 7, 2016
64886a5
Using default constructor
Jan 7, 2016
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
9 changes: 8 additions & 1 deletion core/src/main/java/io/confluent/rest/validation/JacksonMessageBodyProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,14 @@
*/
package io.confluent.rest.validation;

import com.fasterxml.jackson.databind.DeserializationFeature;
import com.fasterxml.jackson.databind.JsonMappingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
import com.fasterxml.jackson.jaxrs.json.JacksonJaxbJsonProvider;

import javax.validation.ConstraintViolationException;
import javax.ws.rs.WebApplicationException;
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.MultivaluedMap;
import javax.ws.rs.ext.Provider;
Expand All @@ -40,10 +42,13 @@
public class JacksonMessageBodyProvider extends JacksonJaxbJsonProvider {

public JacksonMessageBodyProvider() {
setMapper(new ObjectMapper());
ObjectMapper mapper = new ObjectMapper();
mapper.enable(DeserializationFeature.FAIL_ON_READING_DUP_TREE_KEY);
setMapper(mapper);
}

public JacksonMessageBodyProvider(ObjectMapper mapper) {
mapper.enable(DeserializationFeature.FAIL_ON_READING_DUP_TREE_KEY);
Copy link
Contributor

Choose a reason for hiding this comment

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

Forcibly overriding this seems to be counter to the goal of this constructor, which is to allow the user to get control over the exact settings of the ObjectMapper. Is there any reason this needs to be overridden here? If the caller chose to leave FAIL_ON_READING_DUP_TREE_KEY off, is there any reason not to respect that?

This might require adjustments to kafka-rest to address the issue that triggered this PR, but keep in mind this class is used across a few projects and some might not need to be as restrictive and might prefer being a bit more liberal with the input they accept.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry I was thinking having a single key per record to be produced is the requirement, as defined in the API reference page. Key should be uniquely identify a record. However, I think other REST function may also use this to parse their request, which may allow duplicated meta-data. I will change it back.

setMapper(mapper);
}

Expand Down Expand Up @@ -77,6 +82,8 @@ public Object readFrom(Class<Object> type,
Throwable cause = e.getCause();
if (cause instanceof ConstraintViolationException) {
throw (ConstraintViolationException) cause;
}else if(cause instanceof WebApplicationException){
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to extract this type of exception now? And do we . Without a test that triggers the specific error, I'm having trouble understanding why we would get a WebApplicationException as the cause of the JsonMappingException when a key is duplicated. I wouldn't have expected it since WebApplicationException is a Jersey error, not a Jackson error, but maybe I'm missing something about the exception handling in this case?

I know I mentioned WebApplicationException in the kafka-rest issue, but this wasn't quite what I had in mind. I just meant that somewhere in this catch block we might need to handle the specific exception thrown in response to this type of error and convert it to the right type (e.g. that's how invalid constraints found during bean validation are handled by extracting the ConstraintViolationException, which is in turn handled elsewhere by an ExceptionMapper to convert it to the appropriate WebApplicationException).

Copy link
Author

Choose a reason for hiding this comment

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

Sorry I mis-understood your comment. The online Jackson doc specified that FAIL_ON_READING_DUP_TREE_KEY may trigger the JsonMappingException only. They did not mention specific cause type.

throw (WebApplicationException) cause;
}
throw e;
}
Expand Down