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

Returning NotFoundException in InputServiceImpl when id is invalid. #1767

Merged
merged 1 commit into from Apr 12, 2016

Conversation

Projects
None yet
3 participants
@dennisoelkers
Member

dennisoelkers commented Feb 4, 2016

Closes #1718.

@@ -142,6 +142,9 @@ public Input create(Map<String, Object> fields) {

@Override
public Input find(String id) throws NotFoundException {
if (!ObjectId.isValid(id)) {
throw new NotFoundException("Input id <" + id + "> is invalid!");

This comment has been minimized.

@joschi

joschi Feb 4, 2016

Contributor

IMHO this should be an IllegalArgumentException.

This comment has been minimized.

@dennisoelkers

dennisoelkers Feb 5, 2016

Member

I don't think so. This should be a pretty high-level abstraction of the Input persistence. From what the consumer of it needs to know is that there is no entity with that id, not that the id is misconstructed, which does not help the consumer of this method in any way. The id which is invalid for this implementation could be perfectly valid for a different implementation anyhow.

This comment has been minimized.

@joschi

joschi Feb 5, 2016

Contributor

InputService and its implementation are actually quite technical. The high-level access is channeled through the classes calling the methods of InputService, e. g. InputsResource.

It would even be better to have a proper JAX-RS and Jackson serializer and deserializer for ObjectId which would respond with an appropriate HTTP status code (e. g. 400 Bad Request) if no valid ObjectId was provided. This way it wouldn't only be "point repair" in InputService but could be used everywhere.

This comment has been minimized.

@dennisoelkers

dennisoelkers Feb 5, 2016

Member

I agree to the second point, but not to the first. From a separation of responsibility, InputsResource shouldn't bother with the id format, but if the entity is present or not. Therefore the way the code is currently structured throwing this exception is perfectly fine. Having a proper deserializer for ObjectId later on - sure. But not in the scope of this fix.

This comment has been minimized.

@joschi

joschi Feb 5, 2016

Contributor

But even then I don't like the mix of different exception semantics here. An invalid ID is definitively not "not found" but simply an invalid ID for which there is the IllegalArgumentException in Java. Even the message of the NotFoundException in your changeset says that.

I still think this should be an IAE (which then could be automatically mapped to an HTTP 400 response using a JAX-RS exception mapper).

I agree to the second point, but not to the first. From a separation of responsibility, InputsResource shouldn't bother with the id format, but if the entity is present or not.

It should, naturally, validate inputs and reject invalid inputs with an appropriate response code (in this case 400 and not 404).

@dennisoelkers dennisoelkers added this to the 2.0.0 milestone Feb 5, 2016

@dennisoelkers dennisoelkers deleted the issue-1718 branch Feb 5, 2016

@bernd bernd self-assigned this Apr 12, 2016

@bernd bernd restored the issue-1718 branch Apr 12, 2016

@bernd bernd reopened this Apr 12, 2016

@bernd bernd merged commit ceff8a7 into master Apr 12, 2016

0 of 4 checks passed

ci-server-integration Jenkins build graylog2-server-integration-pr 815 is running
Details
ci-web-linter Jenkins build is being scheduled
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@bernd bernd deleted the issue-1718 branch Apr 12, 2016

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