cfpb / django-flags

Feature flags for Django projects
https://cfpb.github.io/django-flags/
Creative Commons Zero v1.0 Universal
260 stars 32 forks source link

Path condition validator warns on valid regular expressions #77

Closed chosak closed 3 years ago

chosak commented 3 years ago

The documentation for the path matches condition suggests it can be configured with a "regular expression value", but its use generates a warning if the ? character is used.

For example, with a flag configured like

FLAGS = {'MY_FLAG': [{'condition': 'path matches', 'value': r'^/flagged/(path)?$'}]}

Django will generate a warning like:

System check identified some issues:

WARNINGS: ?: (flags.E002) Flag MY_FLAG's 'path matches' condition has an invalid value. HINT: Enter a valid path without a URL scheme, query string, or fragment.

The warning (generated by this check) happens because validate_path fails if the specified value includes a ? character. It checks the specified path against ^[^\s:?#]+$, which excludes the ? character.

A simple fix would be to remove ? from the exclusion list. A broader fix might be to change the validator to do something like:

try:
    re.compile(value)
except re.error as e:
    raise ValidationError("Enter a valid regular expression") from e
willbarton commented 3 years ago

That's a good catch, and a good suggestion! I'll open a PR soon for this.

willbarton commented 3 years ago

Hmmm... the unit tests remind me, the reason ? causes a validation error is to ensure that someone doesn't try to match a querystring as part of a URL (to enforce the distinction between URL-matching and path-matching). I might ruminate a little more on this.

willbarton commented 3 years ago

As I've mused on this, the simplest thing is probably to fix the regex and then per #65 provide some means to adding help_text so that the condition can be described in the admin/etc as not working with querystrings.