[Rt-commit] rt branch, 4.0/ip-address-canonicalization, created. rt-4.0.20-32-g938e91a

Alex Vandiver alexmv at bestpractical.com
Thu Jun 5 14:17:34 EDT 2014


The branch, 4.0/ip-address-canonicalization has been created
        at  938e91a6f8472893f6aa2040f12c7957953db53f (commit)

- Log -----------------------------------------------------------------
commit da5169809e13d63faf1bca3cac6806dcb258593b
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 2ab8b08..b1e9964 100644
--- a/lib/RT/ObjectCustomFieldValues.pm
+++ b/lib/RT/ObjectCustomFieldValues.pm
@@ -150,7 +150,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 eb939ac13712d04544c6aa892563e95aab0ee3a4
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 fb92714..371a35b 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -1530,11 +1530,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(
@@ -1557,6 +1553,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 b1e9964..4403d03 100644
--- a/lib/RT/ObjectCustomFieldValues.pm
+++ b/lib/RT/ObjectCustomFieldValues.pm
@@ -136,12 +136,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 d6430ded4021984b90990e5861646c956d339e9b
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 371a35b..0c692c2 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -1530,8 +1530,6 @@ sub AddValueForObject {
         }
     }
 
-    $self->_CanonicalizeValue(\%args);
-
     my $newval = RT::ObjectCustomFieldValue->new( $self->CurrentUser );
     my ($val, $msg) = $newval->Create(
         ObjectType   => ref($obj),
@@ -1587,6 +1585,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 de4bc74..dec2767 100644
--- a/lib/RT/ObjectCustomFieldValue.pm
+++ b/lib/RT/ObjectCustomFieldValue.pm
@@ -84,36 +84,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 8b9a9a7c81249cfd2a0abb20a94715c1cc31dc34
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 0c692c2..f132caf 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -1555,10 +1555,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);
 }
 
@@ -1569,6 +1569,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
@@ -1583,6 +1584,7 @@ sub _CanonicalizeValueDate {
                    Value    => $args->{'Content'},
                  );
     $args->{'Content'} = $DateObj->Date( Timezone => 'user' );
+    return 1;
 }
 
 sub _CanonicalizeValueIPAddress {
@@ -1590,6 +1592,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 {
@@ -1603,6 +1608,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 dec2767..0361193 100644
--- a/lib/RT/ObjectCustomFieldValue.pm
+++ b/lib/RT/ObjectCustomFieldValue.pm
@@ -88,7 +88,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 4403d03..a2ec317 100644
--- a/lib/RT/ObjectCustomFieldValues.pm
+++ b/lib/RT/ObjectCustomFieldValues.pm
@@ -136,7 +136,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 70df354f498f56d69915b0c534787de60cc2fff1
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 0361193..c2b5d41 100644
--- a/lib/RT/ObjectCustomFieldValue.pm
+++ b/lib/RT/ObjectCustomFieldValue.pm
@@ -140,6 +140,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 df061979f5796ef2d0ff4fbd62fa1b06524c72d9
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 c2b5d41..c4d3030 100644
--- a/lib/RT/ObjectCustomFieldValue.pm
+++ b/lib/RT/ObjectCustomFieldValue.pm
@@ -142,16 +142,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 5e76118e0b691e48be2cbe535e443735a0f36f2c
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 45ec7cf..038865d 100644
--- a/lib/RT/Record.pm
+++ b/lib/RT/Record.pm
@@ -1726,8 +1726,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 cb25f53f583082dc2f7be2205c19c32db9f52620
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 713885f..8b49828 100644
--- a/share/html/Elements/ValidateCustomFields
+++ b/share/html/Elements/ValidateCustomFields
@@ -83,27 +83,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, $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, $msg;
-                    $valid = 0;
-                }
+            my $ref = { Content => $value };
+            my ($ok, $msg) = $CF->_CanonicalizeValue( $ref );
+            unless ($ok) {
+                $m->notes( ( 'InvalidField-' . $CF->Id ) => $msg );
+                push @res, $msg;
+                $valid = 0;
             }
         }
 
diff --git a/t/customfields/ip.t b/t/customfields/ip.t
index 3ab7fbd..37afcb7 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 af9a52f..4bccd9a 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 3b8a4d6..84fec16 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 f97420e..3b02ef9 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 938e91a6f8472893f6aa2040f12c7957953db53f
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/Tickets.pm b/lib/RT/Tickets.pm
index 6d536bb..3c4ce29 100644
--- a/lib/RT/Tickets.pm
+++ b/lib/RT/Tickets.pm
@@ -1544,15 +1544,6 @@ sub _CustomFieldLimit {
     }
 
     if ( $cf && $cf->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