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

Add message for FeatureUnavailableException #3402

Closed
wants to merge 5 commits into from
+28 −10
Closed

Add message for FeatureUnavailableException #3402

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f3c2f29
Add message for FeatureUnavailableException
eliykat Nov 2, 2023
a9025d8
Merge remote-tracking branch 'origin/master' into feature-unavailable…
eliykat Nov 22, 2023
8f76c70
Return NotFoundObjectResult instead of throwing exception
eliykat Nov 22, 2023
4cfebd9
Update tests
eliykat Nov 22, 2023
328b0c2
Make error wording consistent
eliykat Nov 22, 2023
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
5 changes: 5 additions & 0 deletions src/Api/Utilities/ExceptionHandlerFilterAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@ public override void OnException(ExceptionContext context)
errorMessage = "Resource not found.";
context.HttpContext.Response.StatusCode = 404;
}
else if (exception is FeatureUnavailableException)
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

{
errorMessage = "This feature is not available.";
Copy link
Contributor

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.

Copy link
Member Author

@eliykat eliykat Nov 22, 2023 •
edited
Loading

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

Copy link
Contributor

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.

context.HttpContext.Response.StatusCode = 404;
}
else if (exception is SecurityTokenValidationException)
{
errorMessage = "Invalid token.";
Expand Down
3 changes: 2 additions & 1 deletion src/Core/Exceptions/FeatureUnavailableException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@

/// <summary>
/// Exception to throw when a requested feature is not yet enabled/available for the requesting context.
/// The client should know what features are available and should not call disabled features.
/// </summary>
public class FeatureUnavailableException : NotFoundException
public class FeatureUnavailableException : Exception
{
public FeatureUnavailableException()
{ }
Expand Down
Loading