-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add message for FeatureUnavailableException #3402
Conversation
No New Or Fixed Issues Found |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can still move to NotFoundObjectResult
per our discussion if you'd like too. I see a few good ideas at https://stackoverflow.com/questions/20139621/how-do-i-return-notfound-ihttpactionresult-with-an-error-message-or-exception.
@@ -82,6 +82,11 @@ public override void OnException(ExceptionContext context) | |||
errorMessage = "Resource not found."; | |||
context.HttpContext.Response.StatusCode = 404; | |||
} | |||
else if (exception is FeatureUnavailableException) | |||
{ | |||
errorMessage = "This feature is not available."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎨 If you want a custom message you can just put this in the exception class below -- set its Message
via the base constructor.
This handler is to override messages, results, etc. for exceptions we don't own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this is right - we own NotFoundException, for example, and many others in this if...else if
block.
The message given to the constructor will still be included in the error response if it's running in dev mode: https://github.com/bitwarden/server/blob/feature-unavailable-exception-improvements/src/Api/Utilities/ExceptionHandlerFilterAttribute.cs#L133-L141. But this provides a guaranteed user-appropriate message for production use.
This does suggest that these properties should just be on the custom Exception object though; this long if...else if
block is basically polymorphic behaviour.
I am unsure how far to go in this PR in refactoring this class, or what direction it even needs to go in. (Part of a larger problem where we routinely throw in the service layer for control flow.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NotFoundException
is ours from inheritance yes, but I mean it has a way to set the message on the base. Why not keep FeatureUnavailableException
inheriting that and inside that class set the static "This feature is not available." message; FeatureUnavailableException
stays parameterless in its constructor.
The status code will still be set.
@@ -82,6 +82,11 @@ public override void OnException(ExceptionContext context) | |||
errorMessage = "Resource not found."; | |||
context.HttpContext.Response.StatusCode = 404; | |||
} | |||
else if (exception is FeatureUnavailableException) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ The current state of this PR has FeatureUnavailableException
unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still used in CollectionsController
and CollectionAuthorizationHandler
. We've used those as guards at the top of feature-flagged methods, e.g.
if (flagEnabled)
{
callnewMethod();
}
public void newMethod() {
// just to make sure noone calls this accidentally...
if (!flagEnabled)
{
throw FeatureUnavailableException();
}
// etc etc
}
That is maybe overkill and misuse of the exception. What do you think? Maybe we should just remove this use of it and rely on unit testing instead, then we don't need the exception at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If your approach to set the result is working well I think it's best to use it everywhere and drop any inconsistent usages.
@@ -30,7 +31,7 @@ public override void OnActionExecuting(ActionExecutingContext context) | |||
|
|||
if (!featureService.IsEnabled(_featureFlagKey, currentContext)) | |||
{ | |||
throw new FeatureUnavailableException(); | |||
context.Result = new NotFoundObjectResult("This feature is unavailable."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ If this setting of the result is acceptable to our middleware and everything works as expected we'd be complete with this change here, but per previous comments it isn't in our situation right? If we can make it move that way that's great, but I thought we need an exception to throw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested this and confirm that it returns a 404 error with the message. It also short-circuits any remaining action filters (docs).
I thought you were suggesting that we use a NotFoundObjectResult
instead of throwing an exception (for the reasons discussed in #architecture
, i.e. exceptions are expensive and maybe our current pattern is not a great one). This change would start moving us in this direction, although at the expense of mixing patterns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we mix patterns but this is the recommended way and we'd eventually want to move everything to it, I support the mixing.
// Assert | ||
Assert.IsType<NotFoundObjectResult>(context.Result); | ||
var result = (NotFoundObjectResult)context.Result; | ||
Assert.Contains("This feature is unavailable", (string)result.Value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Do we need to assert error messages in tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's our current practice. In this case (for example), it ensures that the NotFoundObjectResult
is the one I expect and not for some other reason (a false negative).
This is pretty old by this point and has never come up again, so I'm leaving it be. |
Type of change
Objective
Add a separate message for
FeatureUnavailableException
to help with debugging, both while developing and if a customer stumbles across it in prod.As noted in the xml comment, it is not intended that a user ever sees this; it indicates that we've messed up our client-side feature flagging logic. However, that is helpful to know!
Code changes
Before you submit
dotnet format --verify-no-changes
) (required)