Better webhooks validation

[nmaupu]

• MessageCard can now be validated with a custom validation func
• send API now supports custom webhook patterns for URL validation (added to the ability to deactivating it)

[atc0005]

@nmaupu Thank you for submitting this Pull Request. I hope to look at the changes soon and provide useful feedback.

I glanced at the CI failures and they're golint related:

messagecard.go:200:1: exported method `MessageCard.Validate` should have comment or be unexported (golint)

If you're comfortable with me doing so, I can push to your branch to resolve that after I review the changes.

[nmaupu]

CI fixed.

[atc0005]

@nmaupu Thanks again for submitting this PR.

• MessageCard can now be validated with a custom validation func

I like the changes made here. I believe that the changes fit well and are clear and easy to understand. This feels like it really opens up options without making the library harder to use.

For send_test.go, I see one change that may need correction. I'll add that note directly on the line shortly.

For send.go, I have a few questions. First, I'll note that while I'm no longer a Go newbie, I still have a lot to learn. Thus, my questions may feel like they have obvious answers. I'll ask anyway in the hope that they will not cause offense.

I see that the overall theme is to move standalone functions/functionality out of standalone functions into methods and deprecate the exported validation functionality. For better or worse, I intentionally exported the validation functionality in dasrick#6 so that client code could directly use it. Looking back, while I can see that this might violate the A little copying is better than a little dependency proverb, the idea was to centralize the validation functionality in this library for use across multiple client applications.

Do you feel that exposing this validation functionality directly is a poor fit?

I see that you exported the default prefix regex (now named DefaultWebhookURLValidationPattern). What are your thoughts there?

I originally did not export it thinking that it might not be a stable validation pattern, but maybe it is. Renaming it, both to add a "Default" prefix to the name and add "ValidationPattern" as part of the name makes sense. It is a much friendlier name and fits well with the idea that there is support for multiple validation patterns. With that thinking, it now makes sense why "Prefix" was dropped from other identifiers. This helps keep things consistent.

That said, the ErrWebhookURLUnexpectedPrefix error type was exported in a prior stable release. I couldn't say for sure, but wouldn't we need to alias the old name somehow for backwards compatibility?

I did not initially understand why API was returned from SkipWebhookURLValidationOnSend and AddWebhookURLValidationPatterns, but I see now that this is likely intended to help support method chaining. Does changing (not just extending) the API for this library (even in a compatible way) require a major version bump? I'll have to research that further to learn what the consensus is. If it is only breakage that determines that requirement I suspect we're good here.

I'm still looking over the other changes, will respond further when I can.

[nmaupu]

Hi,

For send.go, I have a few questions. First, I'll note that while I'm no longer a Go newbie, I still have a lot to learn. Thus, my questions may feel like they have obvious answers. I'll ask anyway in the hope that they will not cause offense.

All questions and feedbacks are always welcome 👍 No one can know everything, I am also and always still learning!

I see that the overall theme is to move standalone functions/functionality out of standalone functions into methods and deprecate the exported validation functionality. For better or worse, I intentionally exported the validation functionality in dasrick#6 so that client code could directly use it. Looking back, while I can see that this might violate the A little copying is better than a little dependency proverb, the idea was to centralize the validation functionality in this library for use across multiple client applications.

I see what you mean, nevertheless, webhookURLValidationPatterns being tied to the object, I felt legit to also include the validation func "inside" the scope of the object.
I see a possible improvement though: it would be a great idea to have a singleton that one can use directly (other libraries do that as well, see viper for example: https://github.com/spf13/viper/blob/master/viper.go#L63)
So no need to even create a NewClient() if your usage is the default one 👍 Calls could look like the following:

import teams github.com/atc0005/go-teams-notify
[...]
func main() {
  _ := teams.SendWithContext(context.TODO(), webhook, msg)
}

Do you feel that exposing this validation functionality directly is a poor fit?

I think API should provide basic components only and should try to keep things easy to read and to maintain as much as possible. Exposing helper funcs like this somewhat stutters features of the API and can make things confusing.
Moreover, one can write a trivial helper function in case he/she needs it that does that, for instance:

func myValidateInput(webhookMessage MessageCard, webhooks... string) error {
  err := messageCard.Validate()
  if err != nil {
    return err
  }

  dummyClient := NewClient().AddWebhookURLValidationPatterns(myPatterns...)
  for _, w := range webhooks {
    if err := dummyClient.ValidateWebhook(w); err != nil {
      return err
    }
  }

  return nil
}

I see that you exported the default prefix regex (now named DefaultWebhookURLValidationPattern). What are your thoughts there?

This is a const, I don't see why not to 🤷‍♂️
This helps being transparent to the users and it also permits users to add their own patterns in addition to the default one without having to dig into the source code to copy the private regex...

client := NewClient().AddWebhookURLValidationPatterns(DefaultWebhookURLValidationPattern, myPatterns...)

That said, the ErrWebhookURLUnexpectedPrefix error type was exported in a prior stable release. I couldn't say for sure, but wouldn't we need to alias the old name somehow for backwards compatibility?

Yes sure, but as it's not really a prefix, I would assume naming it xxxPrefix would be confusing.

I did not initially understand why API was returned from SkipWebhookURLValidationOnSend and AddWebhookURLValidationPatterns, but I see now that this is likely intended to help support method chaining. Does changing (not just extending) the API for this library (even in a compatible way) require a major version bump? I'll have to research that further to learn what the consensus is. If it is only breakage that determines that requirement I suspect we're good here.

I don't have a good answer to this, I would say no because existing code will compile and it won't really change the behavior of the API.
However, validations' stuff might justify a major bump 🤔

EDIT: bumping go version would also be a good idea ;)

[atc0005]

However, validations' stuff might justify a major bump

Validation was included in the v1 series. We expanded the validation first to support one additional "known" FQDN pattern, then later as you've probably seen as a hotfix to support other domains (or disable it entirely). As you're aware from submitting this PR, that experience has room for further improvement.

Re the version bump, I think if we end up removing/renaming what is exposed/exported (what I meant by "API" earlier), then that might require us to alias it or do a major version bump as you said.

Will respond further when I have more time to do so.

[atc0005]

I see that you exported the default prefix regex (now named DefaultWebhookURLValidationPattern). What are your thoughts there?

This is a const, I don't see why not to 🤷‍♂️
This helps being transparent to the users and it also permits users to add their own patterns in addition to the default one without having to dig into the source code to copy the private regex...

The original line of thought was that by not exporting it we could modify the regex if needed to adjust the pattern. The regex has worked well thus far, so it should be fine to export.

That said, the ErrWebhookURLUnexpectedPrefix error type was exported in a prior stable release. I couldn't say for sure, but wouldn't we need to alias the old name somehow for backwards compatibility?

Yes sure, but as it's not really a prefix, I would assume naming it xxxPrefix would be confusing.

I don't know where I picked it up, but out of https://fqdn/suburi I was referring to https://fqdn as the "prefix". Admittedly it may not be the best wording, but that is where the prefix naming comes from.

After I ignored individual changes and looked at your code changes as a whole (before I responded) I agree that the new name fits better, but I'm focused on backwards compatibility at this point. Since the ErrWebhookURLUnexpectedPrefix name was published, I think we'll have to keep it around. We could mark it as deprecated and alias it, but I think we'll still have to keep it there.

I did not initially understand why API was returned from SkipWebhookURLValidationOnSend and AddWebhookURLValidationPatterns, but I see now that this is likely intended to help support method chaining. Does changing (not just extending) the API for this library (even in a compatible way) require a major version bump? I'll have to research that further to learn what the consensus is. If it is only breakage that determines that requirement I suspect we're good here.

I don't have a good answer to this, I would say no because existing code will compile and it won't really change the behavior of the API.

To clarify, when I said this:

Does changing (not just extending) the API for this library (even in a compatible way) require a major version bump?

I was referring to "API" in the larger sense of the exported types/functions/constants and so forth and not the specific API type. To answer my own question (alongside your answer), I think we're good if we retain 1:1 functionality.

Semantic Versioning 2.0.0

Summary

Given a version number MAJOR.MINOR.PATCH, increment the:

1. MAJOR version when you make incompatible API changes,
2. MINOR version when you add functionality in a backwards compatible manner, and
3. PATCH version when you make backwards compatible bug fixes.

Additional labels for pre-release and build metadata are available as extensions to the MAJOR.MINOR.PATCH format.

I had to go look it up just to make sure I wasn't remembering wrong.

At the risk of repeating myself, I think the API type changes are compliant, but dropping the exported ErrWebhookURLUnexpectedPrefix variable would not be.

However, validations' stuff might justify a major bump 🤔

I responded to this with my last post, but since the functionality was in the 1.x series I would consider the changes made after that to be a modification or enhancement to the original single FQDN validation behavior. Admittedly not as flexible or elegant as this approach, but not a breaking change in itself.

EDIT: bumping go version would also be a good idea ;)

Are you referring to the go.mod file? I admit that I did not have a solid understanding of the meaning of the version listed within. I often would bump it to reflect the current "old stable" version, but only recently began to see it as the minimum Go version supported by a project. For this project I don't know if we've made changes to require a newer Go version.

From https://golang.org/ref/mod#go-mod-file-go:

• For packages within the module, the compiler rejects use of language features introduced after the version specified by the go directive. For example, if a module has the directive go 1.12, its packages may not use numeric literals like 1_000_000, which were introduced in Go 1.13.
• If an older Go version builds one of the module's packages and encounters a compile error, the error notes that the module was written for a newer Go version. For example, suppose a module has go 1.13 and a package uses the numeric literal 1_000_000. If that package is built with Go 1.12, the compiler notes that the code is written for Go 1.13.

Are you aware of any changes made to this library that would require a newer Go version?

On a different note, I see that you've pushed further changes to your branch. I'll pull those and take another look.

[atc0005]

Since the ErrWebhookURLUnexpectedPrefix name was published, I think we'll have to keep it around. We could mark it as deprecated and alias it, but I think we'll still have to keep it there.

Maybe something like the following (to mirror the approach used in the standard library):

// ErrWebhookURLUnexpected is returned when a provided webhook URL does
// not match a set of confirmed webhook URL patterns.
var ErrWebhookURLUnexpected = errors.New("webhook URL does not match one of expected patterns")

// ErrWebhookURLUnexpectedPrefix is returned when a provided webhook URL does
// not match a set of confirmed webhook URL prefixes.
//
// Deprecated: Use ErrWebhookURLUnexpected instead.
var ErrWebhookURLUnexpectedPrefix = ErrWebhookURLUnexpected

[atc0005]

@atc0005: Since the ErrWebhookURLUnexpectedPrefix name was published, I think we'll have to keep it around.

Unfortunately this might end up applying to other values that were exported also, even DisableWebhookURLValidation which was only around for a short time.

[atc0005]

I see a possible improvement though: it would be a great idea to have a singleton that one can use directly (other libraries do that as well, see viper for example: https://github.com/spf13/viper/blob/master/viper.go#L63)
So no need to even create a NewClient() if your usage is the default one 👍 Calls could look like the following:

import teams github.com/atc0005/go-teams-notify
[...]
func main() {
  _ := teams.SendWithContext(context.TODO(), webhook, msg)
}

This makes sense. I've also seen similar approaches with some logging packages I've used. In most (all?) of those cases I've seen standalone functions and methods mirroring functionality. I don't know whether that means SendWithContext is added first then others later as needed, or if Send is also added to simplify further.

This sounds useful as a separate Pull Request. I'll go ahead and file an issue for it so further discussion can take place there (if needed).

[atc0005]

Do you feel that exposing this validation functionality directly is a poor fit?

I think API should provide basic components only and should try to keep things easy to read and to maintain as much as possible. Exposing helper funcs like this somewhat stutters features of the API and can make things confusing.

Say you have three different dependent projects that you maintain and you'd like to provide validation to each. One approach is to just let this library handle the task as-is without exposing validation functionality, handling it internally. The client application would simply accept the result/error code and deal with that.

In another case, client code could apply validation when parsing flags or config files and reject known invalid URLs leveraging the validation functionality provided by this package. I've done this with at least one case and was adopting it by the others (at some point). It felt like a good fit to expose validation by this library for shared (related) use elsewhere. Admittedly this mindset was building off of the existing validation behavior already present. Had it not been present to begin with I may have just kept the validation functionality in a shared package elsewhere as a layer on top of this library. I can't say for sure.

Just trying to reason this through. Moving the validation to internal-use only might be a good fit for a v3 design, if that is how things settle.

Moreover, one can write a trivial helper function in case he/she needs it that does that, for instance:

func myValidateInput(webhookMessage MessageCard, webhooks... string) error {
  err := messageCard.Validate()
  if err != nil {
    return err
  }

  dummyClient := NewClient().AddWebhookURLValidationPatterns(myPatterns...)
  for _, w := range webhooks {
    if err := dummyClient.ValidateWebhook(w); err != nil {
      return err
    }
  }

  return nil
}

For this:

err := messageCard.Validate()

Is this the new (mc *MessageCard) Validate() error method that you added for this PR? If so, I think I get what you're saying. Hide the helpers, expose validation functionality only through the API interface type (which I think of as the "client") and through the MessageCard concrete type.

Since both use cases are covered, the helper functions can be deprecated.

I think I better understand where you are coming from.

[atc0005]

@atc0005: Since both use cases are covered, the helper functions can be deprecated.

I think I better understand where you are coming from.

Thinking out loud: if GH-96 goes in, would providing exported validation functions (which internally use a singleton for method access) be a concern? If so, would moving validation functionality into a subpackage in any way change this?

[nmaupu]

Since the ErrWebhookURLUnexpectedPrefix name was published, I think we'll have to keep it around. We could mark it as deprecated and alias it, but I think we'll still have to keep it there.

Maybe something like the following (to mirror the approach used in the standard library):

// ErrWebhookURLUnexpected is returned when a provided webhook URL does
// not match a set of confirmed webhook URL patterns.
var ErrWebhookURLUnexpected = errors.New("webhook URL does not match one of expected patterns")

// ErrWebhookURLUnexpectedPrefix is returned when a provided webhook URL does
// not match a set of confirmed webhook URL prefixes.
//
// Deprecated: Use ErrWebhookURLUnexpected instead.
var ErrWebhookURLUnexpectedPrefix = ErrWebhookURLUnexpected

Sure, I added that in the PR!

[nmaupu]

Unfortunately this might end up applying to other values that were exported also, even DisableWebhookURLValidation which was only around for a short time.

I agree but this is not used anymore (making a string comparison to enable / disable something is so-so :/)

[atc0005]

@nmaupu

Thanks for all of your work on this. Overall I think the changes are really useful and make for a more cohesive package.

Unless you want to work it in, I'll plan to update the README coverage to illustrate the changes applied here, omitting use of functionality that is being deprecated by this PR.

[nmaupu]

@nmaupu

Thanks for all of your work on this. Overall I think the changes are really useful and make for a more cohesive package.

Unless you want to work it in, I'll plan to update the README coverage to illustrate the changes applied here, omitting use of functionality that is being deprecated by this PR.

I updated the README. Feel free to commit if you feel changes are inappropriate.

[atc0005]

Thank you for your hard work on this. This library is much improved due to your efforts.

[atc0005]

@nmaupu fwiw: staticcheck flags use of Deprecated code. I saw this in the CI output for some other projects that depend on this one (dependabot update PRs).