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

feat: add config to make error messages configurable #4121

Merged

Conversation

stevenpyzhang
Copy link
Member

@stevenpyzhang stevenpyzhang commented Dec 12, 2019

Description

This PR adds a ksql.server.error.messages config which takes a class that implements the ErrorMessages interface. It uses the ErrorMessages class in certain places to return more targeted error messages (such as linking to documentation in the error message when the user runs into ACLs related errors)

In the future, more responses can be changed to use methods from the class that's plugged in through the server configs.

Testing done

Unit tests
Made my own ErrorMEssagesImpl and specified it in the config

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@stevenpyzhang stevenpyzhang requested a review from a team as a code owner December 12, 2019 00:48
Copy link
Contributor

@big-andy-coates big-andy-coates left a comment

Choose a reason for hiding this comment

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

Thanks @stevenpyzhang

I understand the motivation for this, however this introduces a new public API that we'll need to maintain compatibility with (ideally) moving forward. As such, we should consider is this the right interface to expose? . I would say not. I think this needs a little bit more thought.

At the moment the interface allows the actual HTTP status code to be customised, which could result in some strange behaviours if, for example, an implementation returned a success code where an error code was needed. Is there a need to be able to change the status code? I understand from the description its really only the error message we want to customize.

Might we not be better creating a new ErrorMessages interface or similar, more targeted towards what we actually want to achieve? Such an interface could then be accepted by an Errors class and used to customise the message in the response.

Let me know if the requirements or your plans cover more than just message text.

Add me back in as a reviewer if you want me to review again.

@big-andy-coates big-andy-coates self-assigned this Dec 12, 2019
@stevenpyzhang stevenpyzhang force-pushed the pluggable-error-messages branch 2 times, most recently from d786b54 to 3bc62ec Compare December 13, 2019 01:37
@stevenpyzhang
Copy link
Member Author

Might we not be better creating a new ErrorMessages interface or similar, more targeted towards what we actually want to achieve? Such an interface could then be accepted by an Errors class and used to customise the message in the response.

Went with this. The scope for this should be just the error message so I agree, the previous approach was a bit too heavy handed with potentially changing up error codes.

@stevenpyzhang stevenpyzhang force-pushed the pluggable-error-messages branch from 3bc62ec to 286a5f4 Compare December 13, 2019 01:52
Copy link
Contributor

@big-andy-coates big-andy-coates left a comment

Choose a reason for hiding this comment

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

Thanks @stevenpyzhang

Missing unit tests for the changes to Errors and the pattern of passing an instance of Errors to a static method of Errors is very questionable. Make it a member function!

Otherwise LGTM.

return constructAccessDeniedFromKafkaResponse(errorMessages.kafkaAuthorizationErrorMessage(e));
}

public String webSocketKafkaAuthorizationErrorMessage(final Exception e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing unit test for this method.

Copy link
Member Author

Choose a reason for hiding this comment

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

added

this.errorMessages = errorMessages;
}

public Response accessDeniedFromKafka(final Exception e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing unit test for this method.

Copy link
Member Author

Choose a reason for hiding this comment

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

added

@stevenpyzhang stevenpyzhang force-pushed the pluggable-error-messages branch 3 times, most recently from 8d61f2a to 565fa73 Compare December 17, 2019 00:17
@stevenpyzhang stevenpyzhang force-pushed the pluggable-error-messages branch from 565fa73 to 2ba7a26 Compare December 17, 2019 00:29
@stevenpyzhang stevenpyzhang merged commit cedf47e into confluentinc:master Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants