PERL-807: Implement Initial DNS Seedlist discovery spec #147
Conversation
xdg
left a comment
xdg
left a comment
There was a problem hiding this comment.
Overall, this passes test so that's awesome! My comments are mostly style and clarity with a just few edge cases that need fixing.
lib/MongoDB/_URI.pm
Outdated
| ( | ||
| $result{username}, $result{password}, $result{hostids}, | ||
| $result{db_name}, $result{options} | ||
| ) = ( $1, $2, $3, $4, $5 ); |
There was a problem hiding this comment.
To match line 346, I think you should run _unescape_all on hostids if defined. That would make me more confident about the check at line 231 and so on.
| $result{options} = $self->_parse_options( $valid, \%result ); | ||
| } | ||
|
|
||
| require Net::DNS; |
There was a problem hiding this comment.
Could you please pull the DNS parts out to a separate subroutine? Maybe returning \@hosts and $options? I think it would make this subroutine easier to read and follow.
lib/MongoDB/_URI.pm
Outdated
|
|
||
| my @hosts; | ||
| my $options = {}; | ||
| my @split_name = split( '\.', $result{hostids} ); |
There was a problem hiding this comment.
Please check if you get at least 2 parts before indexing [1..$#split_name] next.
There was a problem hiding this comment.
Actually, this might need to be three? The spec talks about {hostname}.{domainname} where {domainname} is supposed to be at least two parts.
lib/MongoDB/_URI.pm
Outdated
| next unless $rr->type eq 'SRV'; | ||
| my $target = $rr->target; | ||
| # search for dot before domain name for a valid hostname - can have sub-subdomain | ||
| unless ( $target =~ /\.${\$domain_name}$/ ) { |
There was a problem hiding this comment.
I'm not familiar with this idiom for regex interpolation. Why not $target =~ /\.\Q$domain_name\E$/?
There was a problem hiding this comment.
Ah this was just cos I was thinking in string interpolation mode and it worked. Will swap it over as its not a documented way of doing it as far as i can see in perlre
| my $domain_name = join( '.', @split_name[1..$#split_name] ); | ||
| if ( $srv_data ) { | ||
| foreach my $rr ( $srv_data->answer ) { | ||
| next unless $rr->type eq 'SRV'; |
There was a problem hiding this comment.
Is this needed if we queried for 'SRV'?
lib/MongoDB/_URI.pm
Outdated
| } | ||
| } | ||
| $result{options} = \%parsed; | ||
| $result{options} = $self->_parse_options( $valid, \%result ); |
There was a problem hiding this comment.
You can get rid of the temporary $valid here.
lib/MongoDB/_URI.pm
Outdated
| return $set; | ||
| } | ||
|
|
||
| use Devel::Dwarn; |
lib/MongoDB/_URI.pm
Outdated
|
|
||
| use Devel::Dwarn; | ||
| sub _parse_options { | ||
| my ( $self, $valid, $result, $err_unsupported ) = @_; |
There was a problem hiding this comment.
Rather than a boolean $err_unsupported I think we should pass in the TXT record itself for use in errors, so s/err_unsupported/txt_record/
lib/MongoDB/_URI.pm
Outdated
| ( my $lc_k = $k ) =~ tr[A-Z][a-z]; | ||
| if ( !$valid->{$lc_k} ) { | ||
| if ( $err_unsupported ) { | ||
| MongoDB::Error->throw("Unsupported option '$k' in URI $self\n"); |
There was a problem hiding this comment.
Here, I think we'll be less confusing if we reported the TXT records. "... in URI $self with TXT record $txt_record ..."
lib/MongoDB/_URI.pm
Outdated
| next; | ||
| } | ||
| if ( exists $parsed{$lc_k} && !exists $options_with_list_type{$lc_k} ) { | ||
| warn "Multiple options were found for the same value '$lc_k'\n"; |
There was a problem hiding this comment.
As long as I'm looking at this, we should have the warning message clarify that only the first one is used.
|
Changes pushed. Have left the checks for SRV and TXT records in as I am unsure without diving through most of the DNS specs to figure out if it will actually return anything BUT those in a request - however Net::DNS::RR does warn against not checking first: https://metacpan.org/pod/Net::DNS::RR#METHODS so am erring on the side of caution! |
|
Squashed and committed as 32e90bc |
3 participants
No description provided.