You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@directory.apache.org by er...@apache.org on 2006/05/16 11:58:44 UTC
svn commit: r406888 -
/directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java
Author: ersiner
Date: Tue May 16 02:58:43 2006
New Revision: 406888
URL: http://svn.apache.org/viewcvs?rev=406888&view=rev
Log:
Removed 1.5 dep by replacing Boolean.parseBoolean with custom implementation.
It's better to move this simple method to a common place though.
Modified:
directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java
Modified: directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java
URL: http://svn.apache.org/viewcvs/directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java?rev=406888&r1=406887&r2=406888&view=diff
==============================================================================
--- directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java (original)
+++ directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java Tue May 16 02:58:43 2006
@@ -126,13 +126,13 @@
if ( attrs.get( KerberosAttribute.ACCOUNT_DISABLED ) != null )
{
String val = ( String ) attrs.get( KerberosAttribute.ACCOUNT_DISABLED ).get();
- modifier.setDisabled( Boolean.parseBoolean( val.toLowerCase() ) );
+ modifier.setDisabled( parseBoolean( val.toLowerCase() ) );
}
if ( attrs.get( KerberosAttribute.ACCOUNT_LOCKEDOUT ) != null )
{
String val = ( String ) attrs.get( KerberosAttribute.ACCOUNT_LOCKEDOUT ).get();
- modifier.setLockedOut( Boolean.parseBoolean( val.toLowerCase() ) );
+ modifier.setLockedOut( parseBoolean( val.toLowerCase() ) );
}
if ( attrs.get( KerberosAttribute.ACCOUNT_EXPIRATION_TIME ) != null )
@@ -173,4 +173,18 @@
modifier.setKeyVersionNumber( Integer.parseInt( keyVersionNumber ) );
return modifier.getEntry();
}
+
+ /**
+ * TODO: It's better to move this method to a common place.
+ */
+ private static boolean parseBoolean( String bool )
+ {
+ if ( bool.equals( "true" ) )
+ {
+ return true;
+ }
+
+ return false;
+ }
+
}
Re: svn commit: r406888 - /directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java
Posted by Emmanuel Lecharny <el...@gmail.com>.
ah, sorry ! I thought it was permanent.
Thanks for the fix !
On 5/16/06, Ersin Er <er...@gmail.com> wrote:
> Emmanuel Lecharny wrote:
> > Thanks ersin !
> >
> > btw, could you replace
> > if ( bool.equals( "true" ) )
> >
> > by
> > if ( "true".equalsIgnorecase( bool ) )
> >
> > ?
> I had added it just as a quick fix for the compilation problem. I would
> like to use shared/ldap/utils/BooleanUtils instead of the hack but
> shared-ldap is not yet a dependeny for kerberos-shared.
>
> Anyways I've recommitted.
>
> Cheers,
>
> --
> Ersin
> >
> > On 5/16/06, ersiner@apache.org <er...@apache.org> wrote:
> >> Author: ersiner
> >> Date: Tue May 16 02:58:43 2006
> >> New Revision: 406888
> >>
> >> URL: http://svn.apache.org/viewcvs?rev=406888&view=rev
> >> Log:
> >> Removed 1.5 dep by replacing Boolean.parseBoolean with custom
> >> implementation.
> >> It's better to move this simple method to a common place though.
> >>
> >> Modified:
> >>
> >> directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java
> >>
> >>
> >> Modified:
> >> directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java
> >>
> >> URL:
> >> http://svn.apache.org/viewcvs/directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java?rev=406888&r1=406887&r2=406888&view=diff
> >>
> >> ==============================================================================
> >>
> >> ---
> >> directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java
> >> (original)
> >> +++
> >> directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java
> >> Tue May 16 02:58:43 2006
> >> @@ -126,13 +126,13 @@
> >> if ( attrs.get( KerberosAttribute.ACCOUNT_DISABLED ) != null )
> >> {
> >> String val = ( String ) attrs.get(
> >> KerberosAttribute.ACCOUNT_DISABLED ).get();
> >> - modifier.setDisabled( Boolean.parseBoolean(
> >> val.toLowerCase() ) );
> >> + modifier.setDisabled( parseBoolean( val.toLowerCase() ) );
> >> }
> >>
> >> if ( attrs.get( KerberosAttribute.ACCOUNT_LOCKEDOUT ) != null )
> >> {
> >> String val = ( String ) attrs.get(
> >> KerberosAttribute.ACCOUNT_LOCKEDOUT ).get();
> >> - modifier.setLockedOut( Boolean.parseBoolean(
> >> val.toLowerCase() ) );
> >> + modifier.setLockedOut( parseBoolean( val.toLowerCase() ) );
> >> }
> >>
> >> if ( attrs.get( KerberosAttribute.ACCOUNT_EXPIRATION_TIME )
> >> != null )
> >> @@ -173,4 +173,18 @@
> >> modifier.setKeyVersionNumber( Integer.parseInt(
> >> keyVersionNumber ) );
> >> return modifier.getEntry();
> >> }
> >> +
> >> + /**
> >> + * TODO: It's better to move this method to a common place.
> >> + */
> >> + private static boolean parseBoolean( String bool )
> >> + {
> >> + if ( bool.equals( "true" ) )
> >> + {
> >> + return true;
> >> + }
> >> +
> >> + return false;
> >> + }
> >> +
> >> }
> >>
> >>
> >>
> >
> >
>
>
--
Cordialement,
Emmanuel Lécharny
Re: svn commit: r406888 - /directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java
Posted by Ersin Er <er...@gmail.com>.
Emmanuel Lecharny wrote:
> Thanks ersin !
>
> btw, could you replace
> if ( bool.equals( "true" ) )
>
> by
> if ( "true".equalsIgnorecase( bool ) )
>
> ?
I had added it just as a quick fix for the compilation problem. I would
like to use shared/ldap/utils/BooleanUtils instead of the hack but
shared-ldap is not yet a dependeny for kerberos-shared.
Anyways I've recommitted.
Cheers,
--
Ersin
>
> On 5/16/06, ersiner@apache.org <er...@apache.org> wrote:
>> Author: ersiner
>> Date: Tue May 16 02:58:43 2006
>> New Revision: 406888
>>
>> URL: http://svn.apache.org/viewcvs?rev=406888&view=rev
>> Log:
>> Removed 1.5 dep by replacing Boolean.parseBoolean with custom
>> implementation.
>> It's better to move this simple method to a common place though.
>>
>> Modified:
>>
>> directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java
>>
>>
>> Modified:
>> directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java
>>
>> URL:
>> http://svn.apache.org/viewcvs/directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java?rev=406888&r1=406887&r2=406888&view=diff
>>
>> ==============================================================================
>>
>> ---
>> directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java
>> (original)
>> +++
>> directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java
>> Tue May 16 02:58:43 2006
>> @@ -126,13 +126,13 @@
>> if ( attrs.get( KerberosAttribute.ACCOUNT_DISABLED ) != null )
>> {
>> String val = ( String ) attrs.get(
>> KerberosAttribute.ACCOUNT_DISABLED ).get();
>> - modifier.setDisabled( Boolean.parseBoolean(
>> val.toLowerCase() ) );
>> + modifier.setDisabled( parseBoolean( val.toLowerCase() ) );
>> }
>>
>> if ( attrs.get( KerberosAttribute.ACCOUNT_LOCKEDOUT ) != null )
>> {
>> String val = ( String ) attrs.get(
>> KerberosAttribute.ACCOUNT_LOCKEDOUT ).get();
>> - modifier.setLockedOut( Boolean.parseBoolean(
>> val.toLowerCase() ) );
>> + modifier.setLockedOut( parseBoolean( val.toLowerCase() ) );
>> }
>>
>> if ( attrs.get( KerberosAttribute.ACCOUNT_EXPIRATION_TIME )
>> != null )
>> @@ -173,4 +173,18 @@
>> modifier.setKeyVersionNumber( Integer.parseInt(
>> keyVersionNumber ) );
>> return modifier.getEntry();
>> }
>> +
>> + /**
>> + * TODO: It's better to move this method to a common place.
>> + */
>> + private static boolean parseBoolean( String bool )
>> + {
>> + if ( bool.equals( "true" ) )
>> + {
>> + return true;
>> + }
>> +
>> + return false;
>> + }
>> +
>> }
>>
>>
>>
>
>
Re: svn commit: r406888 - /directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java
Posted by Emmanuel Lecharny <el...@gmail.com>.
Thanks ersin !
btw, could you replace
if ( bool.equals( "true" ) )
by
if ( "true".equalsIgnorecase( bool ) )
?
On 5/16/06, ersiner@apache.org <er...@apache.org> wrote:
> Author: ersiner
> Date: Tue May 16 02:58:43 2006
> New Revision: 406888
>
> URL: http://svn.apache.org/viewcvs?rev=406888&view=rev
> Log:
> Removed 1.5 dep by replacing Boolean.parseBoolean with custom implementation.
> It's better to move this simple method to a common place though.
>
> Modified:
> directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java
>
> Modified: directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java
> URL: http://svn.apache.org/viewcvs/directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java?rev=406888&r1=406887&r2=406888&view=diff
> ==============================================================================
> --- directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java (original)
> +++ directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java Tue May 16 02:58:43 2006
> @@ -126,13 +126,13 @@
> if ( attrs.get( KerberosAttribute.ACCOUNT_DISABLED ) != null )
> {
> String val = ( String ) attrs.get( KerberosAttribute.ACCOUNT_DISABLED ).get();
> - modifier.setDisabled( Boolean.parseBoolean( val.toLowerCase() ) );
> + modifier.setDisabled( parseBoolean( val.toLowerCase() ) );
> }
>
> if ( attrs.get( KerberosAttribute.ACCOUNT_LOCKEDOUT ) != null )
> {
> String val = ( String ) attrs.get( KerberosAttribute.ACCOUNT_LOCKEDOUT ).get();
> - modifier.setLockedOut( Boolean.parseBoolean( val.toLowerCase() ) );
> + modifier.setLockedOut( parseBoolean( val.toLowerCase() ) );
> }
>
> if ( attrs.get( KerberosAttribute.ACCOUNT_EXPIRATION_TIME ) != null )
> @@ -173,4 +173,18 @@
> modifier.setKeyVersionNumber( Integer.parseInt( keyVersionNumber ) );
> return modifier.getEntry();
> }
> +
> + /**
> + * TODO: It's better to move this method to a common place.
> + */
> + private static boolean parseBoolean( String bool )
> + {
> + if ( bool.equals( "true" ) )
> + {
> + return true;
> + }
> +
> + return false;
> + }
> +
> }
>
>
>
--
Cordialement,
Emmanuel Lécharny
Re: svn commit: r406888 - /directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java
Posted by Trustin Lee <tr...@gmail.com>.
Ersin,
You did a right job to fix a problem. A side effect was introduced, but we
can fix it easily via a good discussion like that Enrique raised a good
point.
Please keep up the good work! That's what all of us have to do to make this
project a reall killer. ;)
Regards,
Trustin
--
what we call human nature is actually human habit
--
http://gleamynode.net/
--
PGP key fingerprints:
* E167 E6AF E73A CBCE EE41 4A29 544D DE48 FE95 4E7E
* B693 628E 6047 4F8F CFA4 455E 1C62 A7DC 0255 ECA6
Re: svn commit: r406888 - /directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java
Posted by Ersin Er <er...@gmail.com>.
Enrique Rodriguez wrote:
> ersiner@apache.org wrote:
> ...
>> + + /**
>> + * TODO: It's better to move this method to a common place.
>> + */
>> + private static boolean parseBoolean( String bool )
>> + {
>> + if ( bool.equals( "true" ) )
>> + {
>> + return true;
>> + }
>> +
>> + return false;
>> + }
>> + }
I would really not commit this peace of code if knew it would cause this
much trouble :) While I was reading the user's email, there was just a
code open in front of me (Safehaus Triplesec code) and I just copied the
code from there, to fix the build issue in trunks (we do not want this
to happen, right?), thinking that Enrique would put the method to
another better place (or inline as he suggested) later. So the main
point was fixing the trunk build issue. Of course I also looked if
apacheds-kerberos-shared had a dep to shared-ldap. If it was the
situation, I would use the BooleanUtils code there (_especially, if I
need that much flexibility as provided in that Utils code_).
> This entire method is the same as saying:
>
> return bool.equals( "true" );
>
> In which case, you could inline it:
>
> modifier.setLockedOut( val.toLowerCase().equals( "true" ) );
>
> Or, Emmanuel's suggestion:
>
> modifier.setLockedOut( "true".equalsIgnoreCase( val ) );
Agree with inlining. But as I said it was just a temporal copy paste :)
> Now, you have a negligible amount of code that almost added a
> dependency on shared-ldap. Besides bloating the kerberos code by 3 X
> there is also no conceptual link between kerberos and shared-ldap
> code. The problem here is not necessary code size, which in this day
> and age isn't that important, but rather the conceptual integrity of
> the artifacts we provide and a drastic increase in POM maintenance in
> product configurations as these sorts of deps trickle down. I'm sorry
> to use Ersin's commit as an example, but this sort of "eliminate
> duplication at all costs" mindset occurs quite a bit on this project
> and it makes POM maintenance in downstream product assemblies a lot
> harder.
>
> Moreover, the thing to recognize here is that we have configuration
> code in apacheds-core and protocol-common and helper methods like the
> above in many of the protocols and shared-ldap. In which case, the
> thing to do is form a configuration package. What I mean by
> "conceptual integrity" is that we could then point to a configuration
> artifact as the place for configuration library and utility code, as
> opposed to pointing to shared-ldap or apacheds-core or
> protocol-common, which have nothing to do, conceptually, with boolean
> text parsing or configuration.
Most of the utilities under shared-ldap has nothing to do with ldap. So
we may have another lightweight "commons" project.
> Enrique
Cheers,
-- Ersin
Re: svn commit: r406888 - /directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java
Posted by Norbet Reilly <nr...@gmail.com>.
Thanks ... and sorry for the incorrect original posting (I "replied
to" but forgot to change the subject line and "to:" addresses)
Re: svn commit: r406888 - /directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java
Posted by Emmanuel Lecharny <el...@gmail.com>.
Norbet Reilly a écrit :
> Hi ApacheDS gurus...
Hi Norbet !
>
> I raised DIRSERVER-619 on JIRA a few days back and just wanted to
> petition for it to be considered in inclusion before 1.0 is released.
Don't worry ! 1.0 final is not for tomorrow ...
>
> My patch includes a small number of trivial fixes which provide the
> benefit of being able to report much more meaningful failure messages
> to end-users (via naming exception message), whereas currently most of
> the useful information is simply thrown away (by accident - I think).
Yeah, this is a good point. I met the same problem yesturday while
playing with LdifReader, I got an error which was not meaningful.
We will apply your suggested patch soon!
>
> Any opinions?
That's my opinion ;)
>
> Thanks
Thanks very much for the proposal !
Re: svn commit: r406888 - /directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java
Posted by Norbet Reilly <nr...@gmail.com>.
Hi ApacheDS gurus...
I raised DIRSERVER-619 on JIRA a few days back and just wanted to
petition for it to be considered in inclusion before 1.0 is released.
My patch includes a small number of trivial fixes which provide the
benefit of being able to report much more meaningful failure messages
to end-users (via naming exception message), whereas currently most of
the useful information is simply thrown away (by accident - I think).
Any opinions?
Thanks
Re: svn commit: r406888 - /directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java
Posted by Enrique Rodriguez <en...@gmail.com>.
Emmanuel Lecharny wrote:
> ah ! Ok
>
> this was my second bet, so : a new point.
>
> I gonna think twice about what you wrote, it's a little bit
> complicated for my french brain, which is not english fluent ;)
Again, sorry for putting you and Ersin on the spot with this one. I
know you were just fixing a build issue.
The problem is that I've had to deal with this a lot lately with
updating the OSGi build to M2 and Felix. OSGi will make it obvious when
you have excessive dependencies and when you are causing tight coupling
between jars, which is the case with apacheds-core and shared-ldap.
Those two artifacts are the source of our worst coupling.
Also, I'm a bit frustrated because I just went through this with a
similar issue. There is some Calendar manipulation code in DateUtils
that was causing pointless deps on shared-ldap elsewhere. In that case,
the Calendar manipulation is a simple one-liner with a DateFormat.
Enrique
Re: svn commit: r406888 - /directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java
Posted by Enrique Rodriguez <en...@gmail.com>.
Emmanuel Lecharny wrote:
> Hi Enrique !
>
> Not sure I understand your point, and not sure it's important... Ersin
> just fixed a building problem, I just suggested to invert the test and
> change it to allow "TRUE" to be accepted, and you suggested to inline
> the method (which is a good idea).
>
> Is there something I missed, or did you just raised a new point from a
> commit that worth being discussed ?
Ersin's commit to fix the build problem was legitimate. I jumped in
with a new point.
Enrique
>
> Sorry if I didn't get your point... I'm just trying to understand :)
>
> Emmanuel
>
Re: svn commit: r406888 - /directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java
Posted by Emmanuel Lecharny <el...@gmail.com>.
Hi Enrique !
Not sure I understand your point, and not sure it's important... Ersin
just fixed a building problem, I just suggested to invert the test and
change it to allow "TRUE" to be accepted, and you suggested to inline
the method (which is a good idea).
Is there something I missed, or did you just raised a new point from a
commit that worth being discussed ?
Sorry if I didn't get your point... I'm just trying to understand :)
Emmanuel
Re: svn commit: r406888 - /directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java
Posted by Enrique Rodriguez <en...@gmail.com>.
ersiner@apache.org wrote:
...
> +
> + /**
> + * TODO: It's better to move this method to a common place.
> + */
> + private static boolean parseBoolean( String bool )
> + {
> + if ( bool.equals( "true" ) )
> + {
> + return true;
> + }
> +
> + return false;
> + }
> +
> }
This entire method is the same as saying:
return bool.equals( "true" );
In which case, you could inline it:
modifier.setLockedOut( val.toLowerCase().equals( "true" ) );
Or, Emmanuel's suggestion:
modifier.setLockedOut( "true".equalsIgnoreCase( val ) );
Now, you have a negligible amount of code that almost added a dependency
on shared-ldap. Besides bloating the kerberos code by 3 X there is also
no conceptual link between kerberos and shared-ldap code. The problem
here is not necessary code size, which in this day and age isn't that
important, but rather the conceptual integrity of the artifacts we
provide and a drastic increase in POM maintenance in product
configurations as these sorts of deps trickle down. I'm sorry to use
Ersin's commit as an example, but this sort of "eliminate duplication at
all costs" mindset occurs quite a bit on this project and it makes POM
maintenance in downstream product assemblies a lot harder.
Moreover, the thing to recognize here is that we have configuration code
in apacheds-core and protocol-common and helper methods like the above
in many of the protocols and shared-ldap. In which case, the thing to
do is form a configuration package. What I mean by "conceptual
integrity" is that we could then point to a configuration artifact as
the place for configuration library and utility code, as opposed to
pointing to shared-ldap or apacheds-core or protocol-common, which have
nothing to do, conceptually, with boolean text parsing or configuration.
Enrique