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

小提议:可否重新设计 auth.Validator? #35

Closed
cupen opened this issue Jul 12, 2021 · 7 comments
Closed

小提议:可否重新设计 auth.Validator? #35

cupen opened this issue Jul 12, 2021 · 7 comments
Assignees

Comments

@cupen
Copy link

cupen commented Jul 12, 2021

  • Go 版本:1.16
  • wechatpay-go 版本:0.2.2

我在修复 notify/handler 的过程中发现有些坏味道代码,可能当初的设计是非常好的,只是加上 notify 后变得不那么合适了。
我个人感觉可以更新一下 Validator 的定义。
目前的定义是这样:

// Validator 应答报文验证器
type Validator interface {
	Validate(ctx context.Context, response *http.Response) error // 对 HTTP 应答报文进行验证
}

可否改为类似下面的 :

// Validator 验证器
type Validator interface {
	Validate(v Validatable) error {
}

// 可验证对象(表示业务上下文)
type Validatable interface {
	Verify(verifier auth.Verifier)
	// or
	GetVerifyArgs() (serialNo, message, sign string)
}

// 默认的验证器实现
type DefaultValidator struct {
	verifier auth.Verifier
}

func (dv *DefaultValidator) Validate(v Validatable) error {
	return  v.Verify(dv.verifier)
	// or
	serialNo, msg, sign := v.GetVerifyArgs()
	return dv.verifier.Verify(serialNo, msg, sign)
}

然后不同的业务上下文各自实现自己的 Validatable ,比如 http.Response 和 notify 就可以拆到各自的模块,互不相干。
两者在业务上确实也没啥关联,仅仅只是重用了一套验证逻辑(当然,也能重用一些小代码)。

@cupen
Copy link
Author

cupen commented Jul 12, 2021

也许还能更简单些。

// Validator 验证器
type Validator interface {
	Validate(verifier auth.Verifier) error {
}

// HTTP 验证器
type HTTPValidator struct {
	headers ...
	body  ...
	parsedErr error
}

func NewHTTPValidator(headers http.Header, body []byte) Validator {
	// parsing http context
	...
	return HTTPValidator{parsedErr: err,  ...}
}

func (this *HTTPValidator) Validate(verifier auth.Verifier ) error {
	if this.parsedErr != nil {
		return this.parsedErr
	}
	serialNo, msg, sign := this.getVerifyArgs()
	return verifier.Verify(serialNo, msg, sign)
}

@cupen
Copy link
Author

cupen commented Jul 12, 2021

/cc @xy-peng @EmmetZC 敬请评估下。

@EmmetZC
Copy link
Contributor

EmmetZC commented Jul 12, 2021

感谢反馈。

的确一开始 Validator 的设计是专门为了 「API 应答」设计的,后来加入「通知请求」的检查后尽管检查逻辑依旧相同,但是输入不再是 http.Response。

我们评估下这种情况如何处理更合适。

@xy-peng
Copy link
Contributor

xy-peng commented Jul 12, 2021

Validatable的想法很好,等于针对输入数据再做了一次依赖反转,抽象出接口。不过就应答和回调两种场景,抽象出来可能过度设计了。正如你所说的“也许还能更简单”。

// Validator 验证器
type Validator interface {
	Validate(verifier auth.Verifier) error {
}

// HTTP 验证器
type HTTPValidator struct {
	headers ...
	body  ...
	parsedErr error
}

func NewHTTPValidator(headers http.Header, body []byte) Validator {
	// parsing http context
	...
	return HTTPValidator{parsedErr: err,  ...}
}

func (this *HTTPValidator) Validate(verifier auth.Verifier ) error {
	if this.parsedErr != nil {
		return this.parsedErr
	}
	serialNo, msg, sign := this.getVerifyArgs()
	return verifier.Verify(serialNo, msg, sign)
}

我觉得还是 Validate(headers http.Header, body []byte)好。validatorverifier应该是没有接口关联的,只是某些实现上需要。

@EmmetZC
Copy link
Contributor

EmmetZC commented Jul 12, 2021

关于第二种也许还能更简单的方案,我觉得是很有意思的一个抽象方式。不过在我们这里可能并不是特别适用。

首先是Validator 这个接口可能还是叫 Validatee 或者 Validatable 更合适,毕竟看HTTPValidator的实现更像是对数据以及其Validate的逻辑的封装,而 Validate(verifier auth.Verifier) 这个接口则是对 Verify 这个逻辑的依赖注入。

其次这种模式看上去更适合于「有多个 Verifier需要对同一个应答/通知进行检查」的场景。例如:

type SomeHTTPHandler struct {
	verifiers []auth.Verifier
	...
}

func (h *SomeHTTPHandler) verify(header http.Header, body []byte) (err error) {
	data := NewHTTPValidator(header, body)
	
	for (v := range h.verifiers) {
		if err = data.validate(v); err != nil {
			return fmt.Errorf("Verify failed: %w", err)
		}
	}
	return nil
}

而在我们SDK的场景中,目前看应该不会有多个 Verifier 需要同时执行,我们定义Verifier 这个接口的目的其实是为了方便开发者开发替代品。

@cupen
Copy link
Author

cupen commented Jul 12, 2021

@xy-peng @EmmetZC 收到,感谢回复。我也赞同只做必要抽象,晚点我再看看。

@xy-peng
Copy link
Contributor

xy-peng commented Sep 16, 2021

先关闭了,有问题再打开

@xy-peng xy-peng closed this as completed Sep 16, 2021
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

No branches or pull requests

3 participants