Validate the scheme in a separate method of URLValidator

[jleclanche]

This allows subclassing URLValidator to do fancier types of validation
such as regex matching, etc.

---

Use case: in django-oauth-toolkit, the list of allowed schemes is dynamic. This trivial change allows DOT to subclass URLValidator rather than having to reimplement it entirely.

[jleclanche]

I'm hoping this is trivial enough to land in Django 2.1?

[timgraham]

The feature freeze for 2.1 is past. A Trac ticket, test, and documentation are also required.

[jleclanche]

@timgraham I was following the lead of EmailValidator.validate_domain_part(). Not trying to implement a new public API. IMO this is a trivial refactoring, not a new feature.

Although I would use it in django-oauth-toolkit, that would be with full understanding that it's a private API. I can lead it with an underscore if it helps?

The method is already tested as well.

[timgraham]

If it's not a public API, then I don't see the advantage of put a simple condition in a separate method. We wouldn't refactor this if not for the use case. A test would involve overriding the method to ensure that the API isn't removed.

[jleclanche]

@timgraham Here's the use case basically:

jazzband/django-oauth-toolkit@d3400ed

I am overriding URLValidator because in oauth2, I need something that can take it more types of URIs (such as https://localdomain-no-tld/foo and mymobileapp://foo/bar).

Scheme validation is done outside the validator as it requires a call to the overridable model.get_allowed_schemes() in order to get the list:
https://github.com/jazzband/django-oauth-toolkit/blob/c018f5028b199eb4df61bcefd5b8963e8650d805/oauth2_provider/models.py#L138-L143

URLValidator makes it very hard to override. In EmailValidator, I don't believe validate_domain_part() is a public, official API (it's not documented), so I was refactoring to give more flexibility to potential subclasses, following that lead.

[timgraham]

Right, validate_domain_part() isn't a public API. It was added because it's called in two places (98f1376).

The proposed change doesn't strike me as a simplification or a refactoring that would be done if not for your use case. Another contributor could just as easily come along and say "Remove unneeded validate_schema() method" and call that a refactoring.

[jleclanche]

@timgraham Fair point. In that case, how to proceed with this PR? It feels heavy-handed to make this a public API.

[jleclanche]

@timgraham Digging a bit I found a similar ticket:

https://code.djangoproject.com/ticket/26424

I think the approach here in this PR is better. What do you think?

[claudep]

Jerome, could you add a test and refer to #26424 in your commit message?

[jleclanche]

Yes certainly. Do you think this ticket closes it?

[jleclanche]

Done

[claudep]

@jleclanche Working on test failures?

[jleclanche]

Didn't even notice. Working on it.

[felixxm]

Closing per ticket.