Skip to content

Sjw/timestamp validation #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
supernullset wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Sjw/timestamp validation #12

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ce58335
add simple timestamp validation
supernullset Mar 31, 2015
aab2db0
add validation and proper error logging
supernullset Mar 31, 2015
ff75e5e
limit the exception
supernullset Mar 31, 2015
645d1c1
calms travis down about default rake task
supernullset Apr 1, 2015
e5a886f
address PR comments
supernullset Apr 1, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion lib/kmts.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,13 +191,23 @@ def generate_query(type, data, id = nil)
data.update('_k' => @key)
data.update '_d' => 1 if data['_t']
data['_t'] ||= Time.now.to_i

unsafe = Regexp.new("[^#{URI::REGEXP::PATTERN::UNRESERVED}]", false, 'N')

data.inject(query) do |query,key_val|
query_arr << key_val.collect { |i| URI.escape(i.to_s, unsafe) }.join('=')
end
query = '/' + type + '?' + query_arr.join('&')

begin
unless data['_t'].to_s =~ /\d{10}/ and data['_t'].to_i >= 0
Copy link
Contributor

@njm njm Mar 31, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you factor this validation logic into a separate method? Raising the exception only to be caught locally seems a bit ungainly, though I understand it's necessary given #log_error.

Otherwise, note your regular expression pattern is unanchored and arguably a bit overspecified; I'd suggest /\A\d+\z/ instead. It's also necessary to return after handling the exception, as otherwise the tracking request is still issued. Finally as a very minor style nit, our guide advises against and in favor of &&.

Copy link
Author

@supernullset supernullset Apr 1, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could! I thought that since its only going to be used right here, bringing it out would be overkill.

Totally agree about the raise here, I am not happy about it but I am working with the patterns in here.

Good calls on the rest, thanks for spotting it!

raise StandardError, "#{data['_t']} is not a valid unix timestamp"
end
rescue StandardError => e
log_query(query)
log_error(e)
end

if @use_cron
log_query(query)
else
Expand Down
20 changes: 20 additions & 0 deletions spec/km_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,26 @@
IO.readlines(KMTS::log_name(:error)).join.should =~ /Need to initialize first \(KMTS::init <your_key>\)/
end

describe "timstamp" do
it "should fail if timestamp is not unix" do
KMTS::init 'KM_OTHER', :log_dir => __('log'), :host => '127.0.0.1:9292'
KMTS::record 'bob', 'Signup', 'age' => 26, '_p' => 'billybob', '_k' => 'foo', '_n' => 'something else', '_t' => 'bad_timestamp'
IO.readlines(KMTS::log_name(:error)) =~ /is not a valid unix timestamp/
end

it "should fail if timestamp is not valid unix" do
KMTS::init 'KM_OTHER', :log_dir => __('log'), :host => '127.0.0.1:9292'
KMTS::record 'bob', 'Signup', 'age' => 26, '_p' => 'billybob', '_k' => 'foo', '_n' => 'something else', '_t' => -0000000001
IO.readlines(KMTS::log_name(:error)) =~ /is not a valid unix timestamp/
end

it "should succeed if timestamp is valid unix" do
KMTS::init 'KM_OTHER', :log_dir => __('log'), :host => '127.0.0.1:9292'
KMTS::record 'bob', 'Signup', 'age' => 26, '_p' => 'billybob', '_k' => 'foo', '_n' => 'something else', '_t' => 1234567890
File.exists?(KMTS::log_name(:error)).should == false
end
end

it "shouldn't fail on alias without identifying" do
KMTS::init 'KM_OTHER', :log_dir => __('log'), :host => '127.0.0.1:9292'
KMTS::alias 'peter','joe' # Alias "bob" to "robert"
Expand Down