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

Alex Vandiver alexmv at bestpractical.com
Thu Jun 5 16:26:57 EDT 2014


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

- Log -----------------------------------------------------------------
commit 5e2dc55ac098de24d8a4f3f7aa34208cecb8a38b
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 66cf1d4262d7f33381c4d73e74f755db2c48dc51
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 d3928d7b7a052b31fb60ec58e2ce0250d73bc2a4
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Thu Jun 5 16:18:16 2014 -0400

    Move canonicalization into RT::ObjectCustomFieldValue->Create
    
    Previously, value canonicalization happened for dates and datetimes in
    RT::CustomField->AddValueForObject, and for IP addresses and ranges in
    RT::ObjectCustomFieldValue->Create.  Standardize on the latter, as it is
    lower down and thus harder to skip accidentally.
    
    This requires creating the CustomField object as the current user, not
    as the system user, as Date and DateTime canonicalization cares about
    the time zone of the current user.  This, in turn, requires switching to
    _Value('Type') instead of Type, as the constructed $cf object is not
    guaranteed to have the right context objects for it to be readable by
    the user.

diff --git a/lib/RT/CustomField.pm b/lib/RT/CustomField.pm
index 17271b3..53a83ac 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),
@@ -1702,9 +1700,10 @@ sub _CanonicalizeValue {
     my $self = shift;
     my $args = shift;
 
-    return unless $self->Type;
+    my $type = $self->_Value('Type');
+    return unless $type;
 
-    my $method = '_CanonicalizeValue'.$self->Type;
+    my $method = '_CanonicalizeValue'. $type;
     return unless $self->can($method);
     $self->$method($args);
 }
diff --git a/lib/RT/ObjectCustomFieldValue.pm b/lib/RT/ObjectCustomFieldValue.pm
index 07f452d..b788835 100644
--- a/lib/RT/ObjectCustomFieldValue.pm
+++ b/lib/RT/ObjectCustomFieldValue.pm
@@ -82,11 +82,12 @@ sub Create {
         @_,
     );
 
+    my $cf = RT::CustomField->new( $self->CurrentUser );
+    $cf->Load( $args{CustomField} );
 
-    my $cf_as_sys = RT::CustomField->new(RT->SystemUser);
-    $cf_as_sys->Load($args{'CustomField'});
+    $cf->_CanonicalizeValue(\%args);
 
-    if($cf_as_sys->Type eq 'IPAddress') {
+    if($cf->_Value('Type') eq 'IPAddress') {
         if ( $args{'Content'} ) {
             $args{'Content'} = $self->ParseIP( $args{'Content'} );
         }
@@ -99,7 +100,7 @@ sub Create {
         }
     }
 
-    if($cf_as_sys->Type eq 'IPAddressRange') {
+    if($cf->_Value('Type') eq 'IPAddressRange') {
         if ($args{'Content'}) {
             ($args{'Content'}, $args{'LargeContent'}) = $self->ParseIPRange( $args{'Content'} );
         }

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

    Add common IP address and range canonicalizers
    
    The IP address and range canonicalization 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.

diff --git a/lib/RT/CustomField.pm b/lib/RT/CustomField.pm
index 53a83ac..5180e4c 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -1731,6 +1731,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 b788835..4ee1ff1 100644
--- a/lib/RT/ObjectCustomFieldValue.pm
+++ b/lib/RT/ObjectCustomFieldValue.pm
@@ -87,33 +87,6 @@ sub Create {
 
     $cf->_CanonicalizeValue(\%args);
 
-    if($cf->_Value('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->_Value('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;
-        }
-    }
-
     if ( defined $args{'Content'} && length( Encode::encode_utf8($args{'Content'}) ) > 255 ) {
         if ( defined $args{'LargeContent'} && length $args{'LargeContent'} ) {
             $RT::Logger->error("Content is longer than 255 bytes and LargeContent specified");

commit b7a8c03f6ba1485489d3df10347e9c3cc1dff1a2
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 5180e4c..efa7c81 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -1701,10 +1701,10 @@ sub _CanonicalizeValue {
     my $args = shift;
 
     my $type = $self->_Value('Type');
-    return unless $type;
+    return 1 unless $type;
 
     my $method = '_CanonicalizeValue'. $type;
-    return unless $self->can($method);
+    return 1 unless $self->can($method);
     $self->$method($args);
 }
 
@@ -1715,6 +1715,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
@@ -1729,6 +1730,7 @@ sub _CanonicalizeValueDate {
                    Value    => $args->{'Content'},
                  );
     $args->{'Content'} = $DateObj->Date( Timezone => 'user' );
+    return 1;
 }
 
 sub _CanonicalizeValueIPAddress {
@@ -1736,6 +1738,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 {
@@ -1749,6 +1754,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 4ee1ff1..19b17a1 100644
--- a/lib/RT/ObjectCustomFieldValue.pm
+++ b/lib/RT/ObjectCustomFieldValue.pm
@@ -85,7 +85,8 @@ sub Create {
     my $cf = RT::CustomField->new( $self->CurrentUser );
     $cf->Load( $args{CustomField} );
 
-    $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 404787322dffbf7faa13f43a3ad9ff0691ea6b55
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 19b17a1..83c8739 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 8ed92aed53e673f317e844453abae6aca490fcee
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 83c8739..110928c 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 cd370c4ac0a50c47a81d5c9b06895c5d406a48ba
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 d0cef42ff8a0ed64026bb01b14913adde5da439a
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 cd94a08cbb807b16ccf985501085db3cf658a380
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