Mailing List Archive

> On Jan 17, 2017, at 10:04 PM, Peter <peter@pajamian.dhs.org> wrote:
>
> On 18/01/17 11:18, David Christensen wrote:
>> + $Vend::Robot = check_is_robot();
>> +
>> + $CGI::values{mv_tmp_session} ||= 1 if $Vend::Robot;
>> +}
>> +
>> +
>> +sub check_is_robot {
>> + my $ret = 1;
>> +
>> #::logDebug("Check robot UA=$Global::RobotUA IP=$Global::RobotIP");
>> if ($Global::RobotIP and $CGI::remote_addr =~ $Global::RobotIP) {
>> #::logDebug("It is a robot by IP!");
>> - $Vend::Robot = 1;
>> + $ret = 1;
>> }
>> elsif ($Global::HostnameLookups && $Global::RobotHost) {
>> if (!$CGI::remote_host && $CGI::remote_addr) {
>> @@ -291,7 +300,7 @@ EOF
>> }
>> if ($CGI::remote_host && $CGI::remote_host =~ $Global::RobotHost) {
>> #::logDebug("It is a robot by host!");
>> - $Vend::Robot = 1;
>> + $ret = 1;
>> }
>> }
>> unless ($Vend::Robot) {
>> @@ -300,11 +309,10 @@ EOF
>> }
>> elsif ($Global::RobotUA and $CGI::useragent =~ $Global::RobotUA) {
>> #::logDebug("It is a robot by UA!");
>> - $Vend::Robot = 1;
>> + $ret = 1;
>> }
>> }
>> -
>> - $CGI::values{mv_tmp_session} ||= 1 if $Vend::Robot;
>> + return $ret;
>> }
>
> I see some issues with this:
>
> 1. check_is_robot() will always return 1, I think you meant to
> initialize $ret to 0, not 1 at the top.
>
> 2. The previous code would not explicitly set $Vend::Robot to 0, only
> to 1, so there is an incompatible case where $Vend::Robot is already set
> to 1 here but check_is_robot() would return 0. You can fix this by
> changing:
>
> $Vend::Robot = check_is_robot();
>
> ...to...
>
> $Vend::Robot ||= check_is_robot();

Thanks for the feedback; I do know it’s a little wonky currently, thanks to some locally failing unit tests (yay!).

I tested a naïve implementation of the above suggestions and didn’t get things working as expected. I also noticed an additional place where I referenced $Vend::Robot *inside* this block, which was unintentional. I am going to revisit with a clear head tomorrow.

The intention is for check_is_robot() to just examine the relevant UA, etc, only and return a boolean determination, so $Vend::Robot should be =, not ||=; the only other places that refer to $Vend::Robot check for the flag, not having anything to do with it; the ||= comes into play when considering whether to set the mv_tmp_session, which we do respect the previous setting of.

Thanks again,

David
--
David Christensen
End Point Corporation
david@endpoint.com
785-727-1171




_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users

On Tue, 17 Jan 2017, David Christensen wrote:

> Thanks for the feedback; I do know it’s a little wonky currently, thanks
> to some locally failing unit tests (yay!).
>
> I tested a naïve implementation of the above suggestions and didn’t get
> things working as expected. I also noticed an additional place where I
> referenced $Vend::Robot *inside* this block, which was unintentional.
> I am going to revisit with a clear head tomorrow.

This would really be better done on a branch so we don't have known
half-working code on the master branch that we generally consider to be
production-worthy.

Would you mind pushing out a revert commit for all this work in progress
and moving it onto a branch where it can grow and be discussed and tested,
then merged in later?

Thanks,
Jon


--
Jon Jensen
End Point Corporation
https://www.endpoint.com/