Add the ability to validate and update a message using a collection o…

[schmidtw]

…f Options.

[codecov]

Codecov Report

Attention: Patch coverage is 98.06452% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 52.88%. Comparing base (727d830) to head (7f19f1b).

Files	Patch %	Lines
normify.go	97.97%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #167      +/-   ##
==========================================
+ Coverage   51.40%   52.88%   +1.47%     
==========================================
  Files          31       32       +1     
  Lines        4739     4894     +155     
==========================================
+ Hits         2436     2588     +152     
- Misses       2111     2113       +2     
- Partials      192      193       +1     

Flag	Coverage Δ
unittests	52.88% <98.06%> (+1.47%)	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[denopink]

lgtm, just 1 note

I saw we're adding validators. I don't see an issue with clients having more than 1 place where wrp validation occurs. But should we consider reusing the wrpvalidation code where we can in order to keep things aligned?

for example, changing func ValidateMessageType() NormifyOption to

// ValidateMessageType ensures that the message type is valid.
func ValidateMessageType() NormifyOption {
	return optionFunc(wrpvalidator.MessageType)
}

[denopink]

style suggestion if we wanted to keep things arbitrarily consistent

Suggested change

	func (n *Normify) Process(m *Message) error {
	func (n *Normify) Normify(m *Message) error {

or leave that along and change func (f optionFunc) normify(m *Message) error to

func (f optionFunc) process(m *Message) error {
	return f(m)
}

[denopink]

I'm fine with leaving it as is, but I wanted to point it out in case that was the intention

[schmidtw]

Good question. I was worried this might feel like a stuttering API if I called it Normify. I'm fine either way. @johnabass what do you think?

[johnabass]

The general convention with a single-method interface is essentially:

type Xer interface {
    X()
}

So, in this case:

type Normifier interface {
    Normify()
}

The stuttering principal doesn't apply because you don't refer to the method in the same line as the type in normal flow:

var n wrp.Normifier = wrp.NewNormifier(...)
n.Normify() // different line from above

[denopink]

On a different note, where validation is taking place in this package should we consider reusing the wrpvalidation code where we can in order to keep things aligned?

[johnabass]

Yes, as much as possible we should use this library for validation. There are some server-specific rules, like what scytale does. But for the most part, all our services should apply the same validation.

[schmidtw]

@denopink I think we should have wrpvalidation use this code when possible as wrpvalidation is quite involved regarding metrics. I decided to punt on variants of these options that support metrics for now as I don't immediately need them nor do I have the cycles to refactor wrpvalidation to use this yet. My current thinking on the pattern for how to do them is like this:

// EnsureTransactionUUID ensures that the message has a transaction UUID.  If
// the message does not have a transaction UUID, a new one is generated and
// added to the message.
func EnsureTransactionUUID(onadded ...func()) NormifierOption {
	return optionFunc(func(m *Message) error {
		if m.TransactionUUID == "" {
			id, err := uuid.NewRandom()
			if err != nil {
				return err
			}

			m.TransactionUUID = id.String()
			for _, fn := range onadded {
				if fn != nil {
					fn()
				}
			}
		}
		return nil
	})
}