Reject user texts matching a regex and provide the reason to the client-side

[davidegirardi]

Sometimes users send something they really should not in their rageshake descriptions. I propose here an approach to reject rageshakes whose description matches a regex. The rejection reason is reflected client-side so the user's client can show them want went wrong.

The sample config file provides a regex to reject user texts that could contain a recovery key.

TODO if we like this approach:

• Update docs/blocked_rageshake.md
• Update main_test.go
• Update submit_test.go
• Update changelog

[richvdh]

looks sensible in principle. A few thoughts.

[richvdh]

Maybe I've been writing Rust too long, but I don't love the fact that (false, "blah") is a meaningless response.

Could maybe just return *string here, with nil meaning "do not reject" and anything else being the reason to reject.

[davidegirardi]

Fixed in afd59fd.

[richvdh]

I don't think this is fixed. shouldReject and matchesRejectionCondition both still return a bool and a *string, meaning that they can return nonsensical things like (false, "blah").

Recommend making them return just *string.

[davidegirardi]

I think it's ready for review again. I abused the Data attribute to add an expected rejection reason in the tests. I don't know if that's OK.

[richvdh]

I don't think this is fixed. shouldReject and matchesRejectionCondition both still return a bool and a *string, meaning that they can return nonsensical things like (false, "blah").

Recommend making them return just *string.

[richvdh]

I think it woldl be cleaner to take a *payload rather than have this ever-growing list of parameters.

[davidegirardi]

Done, I also moved the version extraction logic into shoudReject from matchesRejectionCondition to do it cleanly.

[davidegirardi]

Gocyclo wants a comment //gocyclo:ignore on a line by itself to ignore a function. Unfortunately go fmt will add a space there // gocyclo:ignore. Can I raise the cyclomatic dependency in the linting script from 12 to 13?

[richvdh]

sorry, forgot to respond to this. (Feel free to "request review", it helps me remember there is something to come back to.)

I'm really not enthusiastic about increasing the cyclomatic complexity limit at all, either locally or globally. Whilst it can feel arbitrary, and to some extent the exact limit is arbitrary, it's there for a reason, and is intended to help remind us when it's time to break down a function into smaller parts rather than waiting for a function to get huge. If we increase the limit every time we hit it, we defeat the purpose.

In this case: sorry, but you've broken the camel's back. It's time to split up shouldReject. How about a set of functions like

func (c RejectionCondition) matchesApp(p *payload) bool {
	// Empty `RejectionCondition.App` is a wildcard which matches anything
	return c.App == "" || c.App == p.AppName
}

func (c RejectionCondition) matchesVersion(p *payload) bool {
	version := ""
	if p.Data != nil {
		version = p.Data["Version"]
	}
	// Empty `RejectionCondition.Version` is a wildcard which matches anything
	return c.Version == "" || c.Version == version
}

... and then put those together in the obvious manner in shouldReject ?

[davidegirardi]

I'll think about it but IMO it will make the code harder, not easier, to follow. Let's see where I land.