Skip to content

Commit

Permalink
Show href provided by Ably and generate href help for error codes #TI…
Browse files Browse the repository at this point in the history
…4 #TI5
  • Loading branch information
mattheworiordan committed Sep 18, 2018
1 parent 5c2407d commit acae470
Show file tree
Hide file tree
Showing 5 changed files with 48 additions and 5 deletions.
3 changes: 3 additions & 0 deletions lib/ably/exceptions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ def to_s
additional_info << "base exception: #{@base_exception.class}" if @base_exception
additional_info << "request_id: #{request_id}" if request_id
message << "(#{additional_info.join(', ')})"
message << "-> see https://help.ably.io/error/#{code} for help" if code
end
message.join(' ')
end
Expand All @@ -54,6 +55,8 @@ def as_json(*args)
# An invalid request was received by Ably
class InvalidRequest < BaseAblyException; end

class InvalidCredentials < BaseAblyException; end

# Similar to 403 Forbidden, but specifically for use when authentication is required and has failed or has not yet been provided
class UnauthorizedRequest < BaseAblyException; end

Expand Down
5 changes: 3 additions & 2 deletions lib/ably/models/error_info.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def initialize(hash_object)
@hash_object = IdiomaticRubyWrapper(hash_object.clone.freeze)
end

%w(message code status_code).each do |attribute|
%w(message code href status_code).each do |attribute|
define_method attribute do
attributes[attribute.to_sym]
end
Expand All @@ -50,7 +50,8 @@ def attributes
end

def to_s
"<Error: #{message} (code: #{code}, http status: #{status})>"
see_msg = " -> see https://help.ably.io/error/#{code} for help" if code
"<Error: #{message} (code: #{code}, http status: #{status})>#{see_msg}"

This comment has been minimized.

Copy link
@SimonWoolf

SimonWoolf Sep 19, 2018

Member

In the work Paddy did, for protocolversions < 1.2, realtime also adds help text with the message, so people on pre-1.2 client libraries see it. So this is going to look like <Error: Invalid key in request (See https://help.ably.io/error/40005 for help.) (code: 40005, http status: 400)> -> see https://help.ably.io/error/40005 for help.

TBH I still don't really follow why we want make it the client lib's responsibility to add the url to the message (at least for errors received from realtime). Why not just let realtime do it? But if we are then at least we should only do it in v1.2 client libraries (for realtime-received errors, easily detectable here as the errorinfo will have an href field), to avoid the doubling-up.

(Also, this should use the href field if present, rather than constructing it from the code)

This comment has been minimized.

Copy link
@mattheworiordan

mattheworiordan Sep 19, 2018

Author Member

@SimonWoolf sure, I should have checked if the href attribute exists, that was an oversight. In terms of not generating a URL, I think we should do that if the help.ably.io URL does not appear in the message. I know that's a bit messy, but pre-1.2, I think that's acceptable personally, and ensures that users get error URLs even if they get it twice potentially. Not clean I know...

This comment has been minimized.

Copy link
@SimonWoolf

SimonWoolf Sep 20, 2018

Member

What's the problem with pre-1.2 logic of 'only add to the message if !message.href'? Since all messages from realtime will have an href attribute and a url in the message already, so this logic only needs to cover library-generated errors which won't yet have an href

end
end
end
16 changes: 16 additions & 0 deletions spec/acceptance/realtime/client_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,22 @@
end
end

context 'with an invalid API key' do
let(:custom_logger_object) { TestLogger.new }
let(:client) { Ably::Realtime::Client.new(client_options.merge(key: 'app.key:secret', logger: custom_logger_object)) }

it 'logs an entry with a help href url matching the code #TI5' do
client.connect
client.connection.once(:failed) do
expect(custom_logger_object.logs.find do |severity, message|
next unless %w(fatal error).include?(severity.to_s)
message.match(%r{https://help.ably.io/error/40400})
end).to_not be_nil
stop_reactor
end
end
end

context ':tls option' do
context 'set to false to force a plain-text connection' do
let(:client_options) { default_options.merge(tls: false, log_level: :none) }
Expand Down
13 changes: 13 additions & 0 deletions spec/acceptance/rest/client_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,19 @@ def encode64(text)
end
end

context 'with an invalid API key' do
let(:client) { Ably::Rest::Client.new(client_options.merge(key: 'app.key:secret', log_level: :fatal)) }

it 'logs an entry with a help href url matching the code #TI5' do
begin
client.channels.get('foo').publish('test')
raise 'Expected Ably::Exceptions::ResourceMissing'
rescue Ably::Exceptions::ResourceMissing => err
expect err.to_s.match(%r{https://help.ably.io/error/40400})
end
end
end

context 'with an explicit string :token' do
let(:client) { Ably::Rest::Client.new(client_options.merge(token: random_str)) }

Expand Down
16 changes: 13 additions & 3 deletions spec/unit/models/error_info_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,25 @@
describe Ably::Models::ErrorInfo do
subject { Ably::Models::ErrorInfo }

it_behaves_like 'a model', with_simple_attributes: %w(code status_code message) do
let(:model_args) { [] }
context '#TI1, #TI4' do
it_behaves_like 'a model', with_simple_attributes: %w(code status_code href message) do
let(:model_args) { [] }
end
end

context '#status' do
context '#status #TI1, #TI2' do
subject { Ably::Models::ErrorInfo.new('statusCode' => 401) }
it 'is an alias for #status_code' do
expect(subject.status).to eql(subject.status_code)
expect(subject.status).to eql(401)
end
end

context 'log entries container help link #TI5' do
subject { Ably::Models::ErrorInfo.new('code' => 44444) }

it 'includes https://help.ably.io/error/[CODE] in the stringified object' do
expect(subject.to_s).to include('https://help.ably.io/error/44444')
end
end
end

0 comments on commit acae470

Please sign in to comment.