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

Fix the values passed to the callback function for fail(). This fixes #257 #258

Closed
wants to merge 1 commit into from

Conversation

hrak
Copy link

@hrak hrak commented Apr 13, 2021

Fix the values passed to the callback function for fail(). This fixes #257

Description

This PR makes the fail() function work the same way as success(), allowing custom status codes and return content.

Motivation and Context

  • I have raised an issue to propose this change (required)

See #257 for details

Which issue(s) this PR fixes

Fixes #257

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Version change (see: Impact to existing users)

Impact to existing users

Existing users that rely on the current behavior of fail() always returning 500 Internal Server Error will have to alter their code.

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@alexellis alexellis requested a review from viveksyngh April 14, 2021 16:21
@alexellis
Copy link
Member

When do you think you can do this review and test @viveksyngh?

@@ -85,9 +85,9 @@ class FunctionContext {
}

fail(value) {
Copy link
Member

Choose a reason for hiding this comment

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

Think of this example, it will return a HTTP 200, and I don't think that's what you wanted.

  return context
    .fail(result)

The code should probably stay the same as it is, but then change the middleware closure, with something like this:

            return res.status(fnContext.status() || 500)

Copy link
Member

Choose a reason for hiding this comment

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

Can you share a handler example that we can use for an end to end test?

return context
    .fail("Not OK")

Should give 500 and "Not OK"

return context
    .succeed("OK")

Gives 200 and "OK"

return context.status(401)
    .fail("Not authorized")

Gives 401 - Not authorized

Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

See notes please

@alexellis
Copy link
Member

@hrak please can you see the notes I posted last week?

@hrak
Copy link
Author

hrak commented May 5, 2021

@hrak please can you see the notes I posted last week?

Hi! Sorry, life happened in the meantime, in the process of buying a house. Will address this asap.

@alexellis
Copy link
Member

Not a problem. We appreciate the contribution and it will make things better.

@alexellis alexellis closed this May 12, 2021
@alexellis
Copy link
Member

I'm going to fix this since we've waited long enough to have it resolved and I have a few minutes to test my work at the same time.

Thanks again for your interest @hrak

If you value our work and project, consider becoming a GitHub Sponsor if you are not already.

alexellis added a commit that referenced this pull request May 12, 2021
Tested with a 401 status before failing the message and saw
the code as expected.
Also tested with just calling fail, and saw the 500 code.

Closes #258 and Fixes: #257

Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
alexellis added a commit that referenced this pull request May 12, 2021
Tested with a 401 status before failing the message and saw
the code as expected.
Also tested with just calling fail, and saw the 500 code.

Closes #258 and Fixes: #257

Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nodejs12 (and nodejs14) templates only return error 500 on context.fail()
2 participants