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

added WRP validator middleware functionality #306

Merged
merged 2 commits into from
Oct 11, 2023
Merged

Conversation

maurafortino
Copy link
Contributor

@maurafortino maurafortino commented Oct 10, 2023

What's Included:

Scytale using the standard WRP validators (SpecValidators which validates the message type, the source and destination, and that message is UTF8) from wrp-go. Validators were created in this PR

@maurafortino maurafortino added the wrp validator issues related to wrp validator label Oct 10, 2023
@maurafortino maurafortino added this to In progress in XMiDT via automation Oct 10, 2023
@maurafortino maurafortino self-assigned this Oct 10, 2023
@maurafortino maurafortino marked this pull request as draft October 10, 2023 18:20
@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Merging #306 (187c79c) into main (19128a6) will decrease coverage by 0.68%.
Report is 2 commits behind head on main.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #306      +/-   ##
==========================================
- Coverage   21.61%   20.93%   -0.68%     
==========================================
  Files           7        7              
  Lines         708      731      +23     
==========================================
  Hits          153      153              
- Misses        553      576      +23     
  Partials        2        2              
Flag Coverage Δ
unittests 20.93% <0.00%> (-0.68%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
primaryHandler.go 0.00% <0.00%> (ø)

@maurafortino maurafortino marked this pull request as ready for review October 10, 2023 20:42
Copy link
Contributor

@denopink denopink left a comment

Choose a reason for hiding this comment

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

some suggestions, otherwise looks great!

primaryHandler.go Outdated Show resolved Hide resolved
err := v.Validate(*msg)
if err != nil {
w.WriteHeader(http.StatusBadRequest)
fmt.Fprintf(
Copy link
Contributor

Choose a reason for hiding this comment

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

u probably forgot to remove this print statement

primaryHandler.go Outdated Show resolved Hide resolved
for _, v := range validators {
err := v.Validate(*msg)
if err != nil {
w.WriteHeader(http.StatusBadRequest)
Copy link
Contributor

Choose a reason for hiding this comment

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

we can also use http.Error()

something like this maybe

if err != nil {
  bodyMsg := fmt.Sprintf(`{"code": %d, "message": "%s"}`,
	  http.StatusBadRequest,
	  fmt.Sprintf("failed to validate wrp message: %s", err))
  http.Error(w, bodyMsg, http.StatusBadRequest)
  return
  }

XMiDT automation moved this from In progress to PRs: Pending approval Oct 11, 2023
XMiDT automation moved this from PRs: Pending approval to PRs: Approved Oct 11, 2023
@maurafortino maurafortino merged commit 498d124 into main Oct 11, 2023
14 of 16 checks passed
XMiDT automation moved this from PRs: Approved to Done Oct 11, 2023
@maurafortino maurafortino deleted the wrp-validator branch October 11, 2023 15:52
Comment on lines +570 to +571
ctx := r.Context()
if msg, ok := wrpcontext.Get[*wrp.Message](ctx); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ctx := r.Context()
if msg, ok := wrpcontext.Get[*wrp.Message](ctx); ok {
if msg, ok := wrpcontext.Get[*wrp.Message](r.Context()); ok {

Comment on lines +588 to +589
}
delegate.ServeHTTP(w, r)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}
delegate.ServeHTTP(w, r)
}
delegate.ServeHTTP(w, r)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wrp validator issues related to wrp validator
Projects
XMiDT
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants