commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: svn commit: r1712699 - in /commons/proper/validator/trunk/src: changes/changes.xml main/java/org/apache/commons/validator/routines/DomainValidator.java test/java/org/apache/commons/validator/routines/DomainValidatorTest.java
Date Thu, 05 Nov 2015 17:23:36 GMT
On 5 November 2015 at 15:58, sebb <sebbaz@gmail.com> wrote:
> On 5 November 2015 at 11:38, Benedikt Ritter <britter@apache.org> wrote:
>> 2015-11-05 1:42 GMT+01:00 <sebb@apache.org>:
>>
>>> Author: sebb
>>> Date: Thu Nov  5 00:42:37 2015
>>> New Revision: 1712699
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1712699&view=rev
>>> Log:
>>> VALIDATOR-341 Make TLD list configurable
>>>

<snip/>

>>>
>> I don't think it is a good idea to implement this based on static fields.
>> It would be better to make DomainValidator instances configurable via the
>> constructor. Has this been implemented this way, because other validators
>> use DomainValidator via Singleton access?
>
> Yes, the existing code uses static methods on the class.

Sorry, that's not exactly incorrect.
The methods are not static, however the calling code uses one of two
fixed instances (depending on whether they support local URLs or not).

So effectively the methods are static at present.
(is that hemi-static ?)

I don't think there is any need to allow different domain lists for
different instances.
Indeed I think that would be confusing.

So I think the approach is still OK.

However if you feel that there is a use case for supporting multiple
threads with different overrides, this needs to be fixed before the
next release.
It would mean a minor change to the API for the DomainValidator, but
all the validators that use the DomainValidator would also need to be
enhanced to add an option to allow a DV instance to be provided.
I think that would make the feature unusable.

It just occurs to me that it would be possible to disallow use of the
override once getInstance has been called.
This would guarantee that every thread got the same overrides.

I think I will add that check, as an accidental later update to the
list might prove difficult to debug and cause unnecessary queries on
the mailing lists.
(It will need to be overridden for unit tests, but that can be done
with a package-protected method)

> The patch just allows the user to update the static data which is used
> for the validation.
>
> In any case, I don't think it makes sense for different threads to
> have different base data - either "xyz" is a valid domain or it is
> not, and this applies for all users of the code, until such time as a
> new domain is registered (or dropped).
>
> So I cannot see a use case for using multiple instances.
>
> As the Javadoc says, the intention is to allow the user to update the
> base data set at the start of an application.
> The alternative is to wait for the next release of the class, or
> rebuild/replace the class locally.
>
> [The code uses a pre-sorted database - rather than parsing a separate
> datafile - because that speeds up startup.
> But that is orthogonal to the question of whether the class should be
> a singleton.]
>
>> Benedikt
>>
>>
>>
>> --
>> http://people.apache.org/~britter/
>> http://www.systemoutprintln.de/
>> http://twitter.com/BenediktRitter
>> http://github.com/britter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message