[Rt-commit] rt branch, 4.2/ip-address-canonicalization, created. rt-4.2.5rc1-9-g3e8172a

Alex Vandiver alexmv at bestpractical.com
Thu Jun 5 14:23:13 EDT 2014


The branch, 4.2/ip-address-canonicalization has been created
        at  3e8172a980fe4c30cd2db07804c24c09941e442b (commit)

- Log -----------------------------------------------------------------
commit 8bafa98007b3f5e7a9376df4c626030e1a40b3ce
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Wed Jun 4 16:09:58 2014 -0400

    ->Content is as formatted for the user, not the DB value
    
    When comparing the canonicalized values, one must bear in mind that
    ->Content is not just the Content column in the database; instead, it
    falls back to the LongContent, in addition to doing explicit formatting
    for IP range custom fields.
    
    Compare the 'Content' key to what is actually in the 'Content' column,
    using $self->_Value().  This does not fix IP custom field updates, as
    the input Content has still not been split into Content and LargeContent
    as the database stores it.

diff --git a/lib/RT/ObjectCustomFieldValues.pm b/lib/RT/ObjectCustomFieldValues.pm
index 11c797f..e02c4ad 100644
--- a/lib/RT/ObjectCustomFieldValues.pm
+++ b/lib/RT/ObjectCustomFieldValues.pm
@@ -149,7 +149,7 @@ sub HasEntry {
             return $item if lc $item->Content eq lc $args->{Content};
         }
         else {
-            if ( $item->Content eq $args->{Content} ) {
+            if ( $item->_Value('Content') eq $args->{Content} ) {
                 if ( defined $item->LargeContent ) {
                     return $item
                       if defined $args->{LargeContent}

commit c899c7d16d909ae94915c55437871a4b1192de9c
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Wed Jun 4 16:11:49 2014 -0400

    Add a generic ->_CanonicalizeValue
    
    Refactor the two places that use can() into a common method.

diff --git a/lib/RT/CustomField.pm b/lib/RT/CustomField.pm
index 5e2b90b..17271b3 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -1675,11 +1675,7 @@ sub AddValueForObject {
         }
     }
 
-    if (my $canonicalizer = $self->can('_CanonicalizeValue'.$self->Type)) {
-         $canonicalizer->($self, \%args);
-    }
-
-
+    $self->_CanonicalizeValue(\%args);
 
     my $newval = RT::ObjectCustomFieldValue->new( $self->CurrentUser );
     my ($val, $msg) = $newval->Create(
@@ -1702,6 +1698,16 @@ sub AddValueForObject {
 }
 
 
+sub _CanonicalizeValue {
+    my $self = shift;
+    my $args = shift;
+
+    return unless $self->Type;
+
+    my $method = '_CanonicalizeValue'.$self->Type;
+    return unless $self->can($method);
+    $self->$method($args);
+}
 
 sub _CanonicalizeValueDateTime {
     my $self    = shift;
diff --git a/lib/RT/ObjectCustomFieldValues.pm b/lib/RT/ObjectCustomFieldValues.pm
index e02c4ad..bec3e81 100644
--- a/lib/RT/ObjectCustomFieldValues.pm
+++ b/lib/RT/ObjectCustomFieldValues.pm
@@ -135,12 +135,7 @@ sub HasEntry {
         my $args = $canon_value{ $cf->Type };
         if ( !$args ) {
             $args = { Content => $value, LargeContent => $large_content };
-            if ( my $canonicalizer =
-                $cf->can( '_CanonicalizeValue' . $cf->Type ) )
-            {
-                $canonicalizer->( $cf, $args );
-            }
-
+            $cf->_CanonicalizeValue( $args );
             $canon_value{ $cf->Type } = $args;
         }
 

commit 3aabe6f133139b2a34f1d496d0b0da3a94483a26
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Wed Jun 4 16:13:35 2014 -0400

    Refactor IP canonicalizers in Create into standard methods
    
    The IP address and range code in ObjectCustomFieldValue's Create is
    moved into the more general canonicalization functions.  This also
    allows HasEntry can split IP ranges correctly to check their existance,
    and ensures that the standard form (padded to three digits per octet) is
    compared against the database.
    
    This additionally ensures that invalid Date and DateTime CF values
    cannot be created directly via ObjectCustomFieldValue->Create; they were
    previously cleaned up only at the ->AddValueForObject level.

diff --git a/lib/RT/CustomField.pm b/lib/RT/CustomField.pm
index 17271b3..0a4ae56 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -1675,8 +1675,6 @@ sub AddValueForObject {
         }
     }
 
-    $self->_CanonicalizeValue(\%args);
-
     my $newval = RT::ObjectCustomFieldValue->new( $self->CurrentUser );
     my ($val, $msg) = $newval->Create(
         ObjectType   => ref($obj),
@@ -1732,6 +1730,26 @@ sub _CanonicalizeValueDate {
     $args->{'Content'} = $DateObj->Date( Timezone => 'user' );
 }
 
+sub _CanonicalizeValueIPAddress {
+    my $self = shift;
+    my $args = shift;
+
+    $args->{Content} = RT::ObjectCustomFieldValue->ParseIP( $args->{Content} );
+}
+
+sub _CanonicalizeValueIPAddressRange {
+    my $self = shift;
+    my $args = shift;
+
+    my $content = $args->{Content};
+    $content .= "-".$args->{LargeContent} if $args->{LargeContent};
+
+    ($args->{Content}, $args->{LargeContent})
+        = RT::ObjectCustomFieldValue->ParseIPRange( $content );
+
+    $args->{ContentType} = 'text/plain';
+}
+
 =head2 MatchPattern STRING
 
 Tests the incoming string against the Pattern of this custom field object
diff --git a/lib/RT/ObjectCustomFieldValue.pm b/lib/RT/ObjectCustomFieldValue.pm
index 07f452d..b8654e5 100644
--- a/lib/RT/ObjectCustomFieldValue.pm
+++ b/lib/RT/ObjectCustomFieldValue.pm
@@ -82,36 +82,11 @@ sub Create {
         @_,
     );
 
+    my $cf = RT::CustomField->new( $self->CurrentUser );
+    $cf->Load( $args{CustomField} );
+    return (0, $self->loc("Invalid custom field")) unless $cf->id;
 
-    my $cf_as_sys = RT::CustomField->new(RT->SystemUser);
-    $cf_as_sys->Load($args{'CustomField'});
-
-    if($cf_as_sys->Type eq 'IPAddress') {
-        if ( $args{'Content'} ) {
-            $args{'Content'} = $self->ParseIP( $args{'Content'} );
-        }
-
-        unless ( defined $args{'Content'} ) {
-            return
-              wantarray
-              ? ( 0, $self->loc("Content is an invalid IP address") )
-              : 0;
-        }
-    }
-
-    if($cf_as_sys->Type eq 'IPAddressRange') {
-        if ($args{'Content'}) {
-            ($args{'Content'}, $args{'LargeContent'}) = $self->ParseIPRange( $args{'Content'} );
-        }
-        $args{'ContentType'} = 'text/plain';
-
-        unless ( defined $args{'Content'} ) {
-            return
-              wantarray
-              ? ( 0, $self->loc("Content is an invalid IP address range") )
-              : 0;
-        }
-    }
+    $cf->_CanonicalizeValue(\%args);
 
     if ( defined $args{'Content'} && length( Encode::encode_utf8($args{'Content'}) ) > 255 ) {
         if ( defined $args{'LargeContent'} && length $args{'LargeContent'} ) {

commit d8093d14ccf7495890c0af828d9f4e709b408dec
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Wed Jun 4 16:21:50 2014 -0400

    Let canonicalization return a failure message
    
    This allows CF creation to abort on invalid values, and HasEntry to give
    up and move on if passed a value that can never be found in the
    database.

diff --git a/lib/RT/CustomField.pm b/lib/RT/CustomField.pm
index 0a4ae56..22c5269 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -1700,10 +1700,10 @@ sub _CanonicalizeValue {
     my $self = shift;
     my $args = shift;
 
-    return unless $self->Type;
+    return 1 unless $self->Type;
 
     my $method = '_CanonicalizeValue'.$self->Type;
-    return unless $self->can($method);
+    return 1 unless $self->can($method);
     $self->$method($args);
 }
 
@@ -1714,6 +1714,7 @@ sub _CanonicalizeValueDateTime {
     $DateObj->Set( Format => 'unknown',
                    Value  => $args->{'Content'} );
     $args->{'Content'} = $DateObj->ISO;
+    return 1;
 }
 
 # For date, we need to store Content as ISO date
@@ -1728,6 +1729,7 @@ sub _CanonicalizeValueDate {
                    Value    => $args->{'Content'},
                  );
     $args->{'Content'} = $DateObj->Date( Timezone => 'user' );
+    return 1;
 }
 
 sub _CanonicalizeValueIPAddress {
@@ -1735,6 +1737,9 @@ sub _CanonicalizeValueIPAddress {
     my $args = shift;
 
     $args->{Content} = RT::ObjectCustomFieldValue->ParseIP( $args->{Content} );
+    return (0, $self->loc("Content is not a valid IP address"))
+        unless $args->{Content};
+    return 1;
 }
 
 sub _CanonicalizeValueIPAddressRange {
@@ -1748,6 +1753,9 @@ sub _CanonicalizeValueIPAddressRange {
         = RT::ObjectCustomFieldValue->ParseIPRange( $content );
 
     $args->{ContentType} = 'text/plain';
+    return (0, $self->loc("Content is not a valid IP address range"))
+        unless $args->{Content};
+    return 1;
 }
 
 =head2 MatchPattern STRING
diff --git a/lib/RT/ObjectCustomFieldValue.pm b/lib/RT/ObjectCustomFieldValue.pm
index b8654e5..3b8e0a3 100644
--- a/lib/RT/ObjectCustomFieldValue.pm
+++ b/lib/RT/ObjectCustomFieldValue.pm
@@ -86,7 +86,8 @@ sub Create {
     $cf->Load( $args{CustomField} );
     return (0, $self->loc("Invalid custom field")) unless $cf->id;
 
-    $cf->_CanonicalizeValue(\%args);
+    my ($val, $msg) = $cf->_CanonicalizeValue(\%args);
+    return ($val, $msg) unless $val;
 
     if ( defined $args{'Content'} && length( Encode::encode_utf8($args{'Content'}) ) > 255 ) {
         if ( defined $args{'LargeContent'} && length $args{'LargeContent'} ) {
diff --git a/lib/RT/ObjectCustomFieldValues.pm b/lib/RT/ObjectCustomFieldValues.pm
index bec3e81..ec3155d 100644
--- a/lib/RT/ObjectCustomFieldValues.pm
+++ b/lib/RT/ObjectCustomFieldValues.pm
@@ -135,7 +135,8 @@ sub HasEntry {
         my $args = $canon_value{ $cf->Type };
         if ( !$args ) {
             $args = { Content => $value, LargeContent => $large_content };
-            $cf->_CanonicalizeValue( $args );
+            my ($ok, $msg) = $cf->_CanonicalizeValue( $args );
+            next unless $ok;
             $canon_value{ $cf->Type } = $args;
         }
 

commit b3fd2ea293a75b5f1a4aa627d232f30d1a460dd1
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Wed Jun 4 16:22:56 2014 -0400

    Provide a better error message for loading OCFVs on an invalid CF

diff --git a/lib/RT/ObjectCustomFieldValue.pm b/lib/RT/ObjectCustomFieldValue.pm
index 3b8e0a3..d6017e2 100644
--- a/lib/RT/ObjectCustomFieldValue.pm
+++ b/lib/RT/ObjectCustomFieldValue.pm
@@ -141,6 +141,8 @@ sub LoadByCols {
     if ( $args{CustomField} ) {
         $cf = RT::CustomField->new( $self->CurrentUser );
         $cf->Load( $args{CustomField} );
+        return (0, $self->loc("Cannot load custom field [_1]",$args{CustomField})) unless $cf->id;
+
         if ( $cf->Type && $cf->Type eq 'IPAddressRange' ) {
 
             my ( $sIP, $eIP ) = $cf->ParseIPRange( $args{'Content'} );

commit 2d0964266852cb93ef2b79c77c4bc1cbb94e1f8d
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Wed Jun 4 16:23:29 2014 -0400

    Canonicalize all OCFVs on load, not just IP address ranges
    
    This ensures that IPs are padded to three digits per octet, and that
    dates are formatted in standard ISO time.

diff --git a/lib/RT/ObjectCustomFieldValue.pm b/lib/RT/ObjectCustomFieldValue.pm
index d6017e2..acf2afe 100644
--- a/lib/RT/ObjectCustomFieldValue.pm
+++ b/lib/RT/ObjectCustomFieldValue.pm
@@ -143,16 +143,8 @@ sub LoadByCols {
         $cf->Load( $args{CustomField} );
         return (0, $self->loc("Cannot load custom field [_1]",$args{CustomField})) unless $cf->id;
 
-        if ( $cf->Type && $cf->Type eq 'IPAddressRange' ) {
-
-            my ( $sIP, $eIP ) = $cf->ParseIPRange( $args{'Content'} );
-            if ( $sIP && $eIP ) {
-                $self->SUPER::LoadByCols( %args,
-                                          Content      => $sIP,
-                                          LargeContent => $eIP
-                                        );
-            }
-        }
+        my ($ok, $msg) = $cf->_CanonicalizeValue(\%args);
+        return ($ok, $msg) unless $ok;
     }
     return $self->SUPER::LoadByCols(%args);
 }

commit f48a6ee02222dc43c246e6c5cb70937e082e6948
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Thu Jun 5 13:57:08 2014 -0400

    Remove too-many-value OCFVs by id, not Content
    
    This merely serves to bulletproof the removal step; there is no need for
    the more expensive Load with Content.

diff --git a/lib/RT/Record.pm b/lib/RT/Record.pm
index 3da6608..c794d49 100644
--- a/lib/RT/Record.pm
+++ b/lib/RT/Record.pm
@@ -1981,8 +1981,8 @@ sub _AddCustomFieldValue {
                 $i++;
                 if ( $i < $cf_values ) {
                     my ( $val, $msg ) = $cf->DeleteValueForObject(
-                        Object  => $self,
-                        Content => $value->Content
+                        Object => $self,
+                        Id     => $value->id,
                     );
                     unless ($val) {
                         return ( 0, $msg );

commit d825f228fb1f7be68c27fe80507638764b2a6a6e
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Wed Jun 4 16:23:58 2014 -0400

    Reuse existing validation in OCVF canonicalization
    
    Rather than reiterate the logic in the canonicalization functions, use
    the error condition they return to determine the validation error.
    
    This also opens the door for invalid date and datetime fields to abort
    ticket updates, by providing a validation error if they do not parse.
    For backwards compatibility, this does not enforce such.

diff --git a/share/html/Elements/ValidateCustomFields b/share/html/Elements/ValidateCustomFields
index fc0049b..8320ed8 100644
--- a/share/html/Elements/ValidateCustomFields
+++ b/share/html/Elements/ValidateCustomFields
@@ -95,27 +95,12 @@ while ( my $CF = $CustomFields->Next ) {
 
     for my $value( @values ) {
         if ($value) {
-            if ( $CF->Type eq 'IPAddress' ) {
-                use Regexp::Common qw(RE_net_IPv4);
-                my $ip = RT::ObjectCustomFieldValue->ParseIP( $value );
-                unless ( $ip ) {
-                    my $msg =
-                      loc( "Input can not be parsed as an IP address" );
-                    $m->notes( ( 'InvalidField-' . $CF->Id ) => $msg );
-                    push @res, $CF->Name .': '. $msg;
-                    $valid = 0;
-                }
-            }
-            elsif ( $CF->Type eq 'IPAddressRange' ) {
-                my ( $start_ip, $end_ip ) =
-                  RT::ObjectCustomFieldValue->ParseIPRange($value);
-                unless ( $start_ip && $end_ip ) {
-                    my $msg =
-                      loc( "Input can not be parsed as an IP address range" );
-                    $m->notes( ( 'InvalidField-' . $CF->Id ) => $msg );
-                    push @res, $CF->Name .': '. $msg;
-                    $valid = 0;
-                }
+            my $ref = { Content => $value };
+            my ($ok, $msg) = $CF->_CanonicalizeValue( $ref );
+            unless ($ok) {
+                $m->notes( ( 'InvalidField-' . $CF->Id ) => $msg );
+                push @res, $CF->Name .': '. $msg;
+                $valid = 0;
             }
         }
 
diff --git a/t/customfields/ip.t b/t/customfields/ip.t
index 2537b1e..35a245c 100644
--- a/t/customfields/ip.t
+++ b/t/customfields/ip.t
@@ -147,7 +147,7 @@ diag "check that we parse correct IPs only" if $ENV{'TEST_VERBOSE'};
             }
         );
 
-        $agent->content_contains( 'can not be parsed as an IP address',
+        $agent->content_contains( 'is not a valid IP address',
             'ticket fails to create' );
     }
 
diff --git a/t/customfields/iprange.t b/t/customfields/iprange.t
index c99d1f1..2a323a3 100644
--- a/t/customfields/iprange.t
+++ b/t/customfields/iprange.t
@@ -197,7 +197,7 @@ diag "check that we parse correct IPs only" if $ENV{'TEST_VERBOSE'};
             }
         );
 
-        $agent->content_like( qr/can not be parsed as an IP address range/, 'ticket fails to create' );
+        $agent->content_like( qr/is not a valid IP address range/, 'ticket fails to create' );
     }
 
 }
diff --git a/t/customfields/iprangev6.t b/t/customfields/iprangev6.t
index 3d73a23..445df33 100644
--- a/t/customfields/iprangev6.t
+++ b/t/customfields/iprangev6.t
@@ -193,7 +193,7 @@ diag "check that we parse correct IPs only" if $ENV{'TEST_VERBOSE'};
             }
         );
 
-        $agent->content_like( qr/can not be parsed as an IP address range/,
+        $agent->content_like( qr/is not a valid IP address range/,
             'ticket fails to create' );
     }
 
diff --git a/t/customfields/ipv6.t b/t/customfields/ipv6.t
index 9c82d0d..24f7c2a 100644
--- a/t/customfields/ipv6.t
+++ b/t/customfields/ipv6.t
@@ -150,7 +150,7 @@ diag "check that we parse correct IPs only" if $ENV{'TEST_VERBOSE'};
             }
         );
 
-        $agent->content_contains( 'can not be parsed as an IP address',
+        $agent->content_contains( 'is not a valid IP address',
             'ticket fails to create' );
     }
 }

commit 3e8172a980fe4c30cd2db07804c24c09941e442b
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Wed Jun 4 16:24:27 2014 -0400

    Remove a needless block; ParseIPRange does the exact same as its first step

diff --git a/lib/RT/SearchBuilder.pm b/lib/RT/SearchBuilder.pm
index b8b2c2f..ce0e62d 100644
--- a/lib/RT/SearchBuilder.pm
+++ b/lib/RT/SearchBuilder.pm
@@ -529,13 +529,6 @@ sub _LimitCustomField {
                 $RT::Logger->warn("$value is not a valid IPAddress");
             }
         } elsif ( $type eq 'IPAddressRange' ) {
-            if ( $value =~ /^\s*$RE{net}{CIDR}{IPv4}{-keep}\s*$/o ) {
-                # convert incomplete 192.168/24 to 192.168.0.0/24 format
-                $value =
-                  join( '.', map $_ || 0, ( split /\./, $1 )[ 0 .. 3 ] ) . "/$2"
-                  || $value;
-            }
-
             my ( $start_ip, $end_ip ) =
               RT::ObjectCustomFieldValue->ParseIPRange($value);
             if ( $start_ip && $end_ip ) {

-----------------------------------------------------------------------


More information about the rt-commit mailing list