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

lambda: check if error type is InvokeResponse_Error #312

Merged
merged 2 commits into from Sep 22, 2020

Conversation

smasher164
Copy link
Contributor

Currently, we cannot set the errorType field without creating a custom
named error type. This causes a proliferation of dummy named error types
whose only purpose is to set a text field, and limits the errorType
field to what can be expressed as an identifier.

This change checks that the panicked or returned error is if type
InvokeResponse_Error, and if so, uses it directly in the invoker's
response. InvokeResponse_Error is now updated to implement error.

Note: This is fully backwards compatible, since InvokeResponse_Error
previously did not implement the error interface.

Fixes #308.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Currently, we cannot set the errorType field without creating a custom
named error type. This causes a proliferation of dummy named error types
whose only purpose is to set a text field, and limits the errorType
field to what can be expressed as an identifier.

This change checks that the panicked or returned error is if type
InvokeResponse_Error, and if so, uses it directly in the invoker's
response. InvokeResponse_Error is now updated to implement error.

Note: This is fully backwards compatible, since InvokeResponse_Error
previously did not implement the error interface.

Fixes aws#308.
@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2020

Codecov Report

Merging #312 into master will decrease coverage by 0.40%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #312      +/-   ##
==========================================
- Coverage   72.63%   72.23%   -0.41%     
==========================================
  Files          18       18              
  Lines         720      724       +4     
==========================================
  Hits          523      523              
- Misses        134      136       +2     
- Partials       63       65       +2     
Impacted Files Coverage Δ
lambda/errors.go 77.27% <0.00%> (-17.18%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4145757...c7abb62. Read the comment docs.

@bmoffatt
Copy link
Collaborator

Thanks for the PR @smasher164 - Until now, I've considered the types in the messages package to be an internal implementations detail, but that's not a strongly held opinion.

I've forwarded this to the team to check for alternative approaches to the problem.

@smasher164
Copy link
Contributor Author

Hey @bmoffatt, friendly ping to ask if there's any word on the issue/PR?
I have a couple of ideas on alternative approaches:

  • The exported type does not have to be from the messages package. You can introduce and detect another named type. For instance, you could introduce something like this in the lambda package:
type Error struct{
    Message string `json:"errorMessage"`		
    Type    string `json:"errorType"`
}
  • Introduce an interface that the user's desired error type could satisfy. This would be an optional interface that would require a type assertion, like:
type Error interface {
    error
    Type() string
}

The disadvantage is that the second option is a potentially breaking change, if the user's error implements the Type method.

@bmoffatt
Copy link
Collaborator

I talked with the team, and we figured that if we ever want to get rid of the messages library, we'd have to do a v2 release regardless of your initial proposal. I'll merge the change as-is

The lambda.Error change you proposed is cosmetically nicer, but I don't like that we'd just be introducing more types just to end up copying into InvokeResponse_Error anyways.

@bmoffatt bmoffatt merged commit c6b2e41 into aws:master Sep 22, 2020
@smasher164 smasher164 deleted the issue-308 branch September 22, 2020 19:23
@smasher164 smasher164 mentioned this pull request Oct 21, 2020
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.

detect if returned error is of type InvokeResponse_Error
3 participants