Skip to content

Issue #177 malformed urls inside json response #178

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

Closed
wants to merge 3 commits into from
Closed

Conversation

skiarn
Copy link

@skiarn skiarn commented Mar 17, 2019

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

@codecov-io
Copy link

codecov-io commented Mar 17, 2019

Codecov Report

Merging #178 into master will increase coverage by 0.18%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #178      +/-   ##
==========================================
+ Coverage    77.2%   77.38%   +0.18%     
==========================================
  Files          18       18              
  Lines         636      650      +14     
==========================================
+ Hits          491      503      +12     
- Misses        104      105       +1     
- Partials       41       42       +1
Impacted Files Coverage Δ
lambda/handler.go 95.18% <85.71%> (-1.93%) ⬇️

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 c84016c...49fa0d3. Read the comment docs.

// SetResponseNonEscapeHTML will NOT coerced JSON strings to valid UTF-8,
// The angle brackets "<" and ">" will NOT be escaped to "\u003c" and "\u003e"
// Ampersand "&" is also NOT be escaped to "\u0026".
func SetResponseNonEscapeHTML(flag bool) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of a package global setter, I'd prefer optional values to somehow be configured on the type returned by NewHandler. I think this could be done in a backwards compatible way in one of these two approaches:

  1. adding a HandlerOptions type to pass as varargs to NewHandler. eg:
  • NewHandler(handler interface{}, opts... HandlerOptions) lambda.Handler.
  1. update NewHandler to return a new struct *JSONHandler still implementing lambda.Handler, and add this add this configuration as a method for that struct. eg:
  • func NewHandler(handler interface{}) *JSONHandler
  • func (handler *JSONHandler) SetEscapeHTML(on bool)

I think I like option 2 better, since it would setup a pattern for mirroring the other json encode/decode options like https://golang.org/pkg/encoding/json/#Encoder.SetEscapeHTML

In-use, this'd look like

func main() {
        jsonHandler := NewHandler(func () (string, error) {
                return "<html><body>html in json string!</body></html>", nil
        });
        jsonHandler.SetEscapeHTML(false);
        lambda.StartHandler(jsonHandler)
}

Copy link
Collaborator

@bmoffatt bmoffatt left a comment

Choose a reason for hiding this comment

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

@skiarn
Copy link
Author

skiarn commented Jul 31, 2019

Didnt have to much time lately, took a look at again, do you mean the Handler interface can be modified like ex: https://github.com/skiarn/aws-lambda-go/tree/aws-lambda-encoder
After thinking about it more adding a .SetEscapeHTML(false); method is most likely not very good since users may want to use different encoding/json packages? Therefore I added SetMarshal / SetUnmarshal functions to the Handler interface, but then kind of felt as if using the Invoke function is a better way to go?

Maybe only having Invoke(ctx context.Context, request []byte) ([]byte, error) is good enough?

For me the work around proposed in #177, is good enough for me, I have the ability to implement desired serializers. Maybe instead of having more methods on the Handler interface documentation may be better? highlighting how invoke can be used when serializing req and resp?
`func (h *Handler) Invoke(ctx context.Context, request []byte) ([]byte, error) {
// ...
}

func main() {
lambda.StartHandler(&Handler{})
}`

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.

4 participants