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

Add capability to define optional handler behavior #444

Merged
merged 4 commits into from May 19, 2022

Conversation

bmoffatt
Copy link
Collaborator

Issue #, if available:

#177
#178 (comment)

Description of changes:

  • add Option WithSetEscapeHTML to set this same named option on the underlying json encoder
  • refactor implementations of ***WithContext functions to use the Option WithContext
  • mark a bunch of not useful related stuff as deprecated so that they are hidden in the godocs

This adds an options pattern for allowing a function author to pick from one or more customizations to the default runtime behavior. The pattern can later be followed to implement other requests for changes to the default runtime behavior. Example feature request: #433

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

* add Option WithSetEscapeHTML to set this same named option on the underlying json encoder
* refactor implementations of ***WithContext functions to use the Option WithContext
* mark a bunch of not useful related stuff as deprecated so that they are hidden in the godocs
@bmoffatt bmoffatt requested a review from carlzogh May 18, 2022 01:02
@codecov-commenter
Copy link

codecov-commenter commented May 18, 2022

Codecov Report

Merging #444 (03d7be3) into main (ca99aa0) will increase coverage by 0.82%.
The diff coverage is 91.35%.

@@            Coverage Diff             @@
##             main     #444      +/-   ##
==========================================
+ Coverage   72.01%   72.83%   +0.82%     
==========================================
  Files          19       19              
  Lines        1054     1086      +32     
==========================================
+ Hits          759      791      +32     
- Misses        225      226       +1     
+ Partials       70       69       -1     
Impacted Files Coverage Δ
lambda/entry.go 72.00% <66.66%> (-4.20%) ⬇️
lambda/function.go 88.88% <83.33%> (+1.61%) ⬆️
lambda/handler.go 97.70% <95.16%> (+0.93%) ⬆️
lambda/invoke_loop.go 73.68% <100.00%> (ø)
lambda/rpc.go 62.50% <100.00%> (ø)

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 ca99aa0...03d7be3. Read the comment docs.

@carlzogh
Copy link
Contributor

Thanks for this Bryan! It may be worth adding a little Usage section to describe how a user of the library would set some of these options

carlzogh
carlzogh previously approved these changes May 18, 2022
type handlerOptions struct {
Handler
baseContext context.Context
escapeHTML bool
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is only relevant to response serialization, maybe we can make it explicit in the property name (eg. escapeResponseHTML or something along those lines)

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. I would even go a bit further and prefix/suffix these with something like ResponseJSON so it would be something like:

responseJSONEscapeHTML
responseJSONPrefix
responseJSONIndex

Alternatively you could have customers pass in a newJSONEncoder function which they could configure however they like (or use a different JSON library). It would have the following signature:

newJSONEncoder func(out io.Writer) error

That might be the most flexible. Could even throw in a newJSONDecoder so users could have complete control over the JSON part of the process.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did play around with the custom encoder idea before pushing this PR! However didn't come up with something that still elegantly worked with the stdlib json NewEncoder. My failed ideas looked something like this: https://play.golang.com/p/To4q1sKBbjm

Something like this would work:

func WithEncoder(encoderFactory func(io.Writer) func(any) error) Option {
  return Option(func(h *handlerOption) {
    h.encoderFactory = encoderFactory
  })
}
// use example with stdlib json
encoderOption := WithEncoder(func(w io.Writer) func(any) error {
  return json.NewEncoder(w).Encode
})

But i'd want to mull on it a little more, and look at other popular encoders, to see if there's not something more ergonomic that can be figured out

Copy link
Contributor

Choose a reason for hiding this comment

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

yea, I personally like the WithEncoder pattern over having three new json-specific variables but either way ultimately works.

@bmoffatt bmoffatt merged commit 0dae564 into aws:main May 19, 2022
@bmoffatt bmoffatt deleted the start-with-options branch May 19, 2022 00:19
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.

None yet

4 participants