Skip to content
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.

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

MHV Activity Logging Client #442

Merged
merged 8 commits into from
Nov 4, 2016
1 change: 1 addition & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -78,3 +78,4 @@ Style/AccessorMethodName:
Exclude:
- 'lib/rx/**/*'
- 'lib/sm/**/*'
- 'lib/mhv_logging/**/*'
Copy link
Contributor

Choose a reason for hiding this comment

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

How come?

Copy link
Contributor

@mphprogrammer mphprogrammer Nov 4, 2016

Choose a reason for hiding this comment

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

If I remember correctly, rubocop doesn't like methods named get_something..., post_something.... However, that naming style was adopted for the client apis.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can tell it to ignore those specific lines if you want.

# rubocop:disable RULE

We could also disable that rule, which IMHO makes no sense whatsoever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, i like using get_ and put_ for methods that are making those types of http calls and this rule prevents me from doing so.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the idea of the rule is that you shouldn't put get in the name of a getter method (just call it x, not get_x), but it makes no sense not to say "get" in the context of making an HTTP client.

4 changes: 3 additions & 1 deletion config/initializers/breakers.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# frozen_string_literal: true
require 'breakers/statsd_plugin'
require 'mhv_logging/configuration'
require 'rx/configuration'
require 'sm/configuration'

Expand All @@ -14,7 +15,8 @@

services = [
Rx::Configuration.instance.breakers_service,
SM::Configuration.instance.breakers_service
SM::Configuration.instance.breakers_service,
MHVLogging::Configuration.instance.breakers_service
# TODO: (AJM) Disabled for testing, add these back in for launch
# EVSS::ClaimsService.breakers_service,
# EVSS::CommonService.breakers_service,
Expand Down
17 changes: 17 additions & 0 deletions lib/mhv_logging/api/audits.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# frozen_string_literal: true
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand why this module is separate from the Sessions one or really why they're modules at all rather than being inside client.rb directly. Seems like a strategy to factor the code out, but if it's not being reused then I think we're adding complexity with little gain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aub I did it this way very simply because I wanted to have an MVC style concept I guess, where the api methods correlate to the rails controllers calling them. By separating these out into modules that get mixed in to the client, the specific modules are kind of the equivalent of a Controller for the client. The idea was that later, with minor refactoring I could mixin those same modules into the Model itself, and for example a Message object would be able to lookup values via the client in an ActiveRecord like way. I ultimately opted against doing this.

However, this allowed us to write specs for each of the apis sections in an organized manner, it's more a matter of separating out related MHV endpoints into distinct units. It's true, they could all be on client, but its one big jumbled mess if I did it that way. Take for example Secure messaging, the api modules map directly to rails controllers, specs are organized in a similar fashion. There is actually also a rubocop rule related to length of a class, and secure messaging would easily exceed that limit without this variety of separation. I strongly feel it helps with organizing the methods.

Until I have a chance to DRY up clients and remove some of the repetitive cruft into a parent client class, I prefer to keep this separation / mixin structure even if it only serves to keep things organized.

module MHVLogging
module API
# This module defines the prescription actions
Copy link
Contributor

Choose a reason for hiding this comment

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

Bad copy/paste?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, i will fix this.

module Audits
def auditlogin
body = { isSuccessful: true, activityDetails: 'Signed in Vets.gov' }
perform(:post, 'activity/auditlogin', body, token_headers)
end

def auditlogout
body = { isSuccessful: true, activityDetails: 'Signed out Vets.gov' }
perform(:post, 'activity/auditlogout', body, token_headers)
end
end
end
end
17 changes: 17 additions & 0 deletions lib/mhv_logging/api/sessions.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# frozen_string_literal: true
module MHVLogging
module API
# This module defines the session actions
module Sessions
def get_session
env = perform(:get, 'session', nil, auth_headers)
req_headers = env.request_headers
res_headers = env.response_headers
# This session store inherits from Rx, therefore it is the same redis_store
MHVLogging::ClientSession.new(user_id: req_headers['mhvCorrelationId'],
expires_at: res_headers['expires'],
token: res_headers['token'])
end
end
end
end
101 changes: 101 additions & 0 deletions lib/mhv_logging/client.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
# frozen_string_literal: true
require 'faraday'
require 'common/client/errors'
require 'common/client/middleware/response/json_parser'
require 'common/client/middleware/response/raise_error'
require 'common/client/middleware/response/snakecase'
require 'mhv_logging/configuration'
require 'mhv_logging/client_session'
require 'mhv_logging/api/audits'
require 'mhv_logging/api/sessions'

module MHVLogging
# Core class responsible for api interface operations
class Client
include MHVLogging::API::Audits
include MHVLogging::API::Sessions

REQUEST_TYPES = %i(get post).freeze
USER_AGENT = 'Vets.gov Agent'
BASE_REQUEST_HEADERS = {
'Accept' => 'application/json',
'Content-Type' => 'application/json',
'User-Agent' => USER_AGENT
}.freeze

attr_reader :config, :session

def initialize(session:)
@config = MHVLogging::Configuration.instance
@session = MHVLogging::ClientSession.find_or_build(session)
Copy link
Contributor

Choose a reason for hiding this comment

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

Having @session and session is pretty confusing. Should it be @mhv_session?

Copy link
Contributor Author

@saneshark saneshark Nov 4, 2016

Choose a reason for hiding this comment

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

honestly, I would like to change the DSL for these initialize altogether, but that will happen in a separate refactor.

I want to make the clients such that they are invoked in the following manner.

MHVLogging::Client.user(user_id) -- this sets up a new session given the user id.
MHVLogging::Client.existing_session(session) -- this sets up an existing session

Both of these would return an instance of Client, and calling .new would require you to pass one or the other.

Copy link
Contributor Author

@saneshark saneshark Nov 4, 2016

Choose a reason for hiding this comment

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

MHVLogging uses the same redisstore as prescription because those sessions do not need to be unique. Currently this is the same DSL used by the other two clients, so I did more for consistency between those than anything else. What's confusing about it? session can be passed in as a hash of values, which will be used by the find_or_build method to either build a new session if none exists in redis or deserialize a session based on the input and persist it to redis upon authentication.

ClientSession is a type of RedisStore.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just confused by the naming of the variables. You have a session variable that gets passed in and is used to create an instance variable of the same name but that is a different type of thing. Note that this is not a big deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean now. Especially since one is a hash and the other is an object federalized from hash having the same name. I will fix this in a larger refactor though if that's OK.

I will need to do that anyway as part of building a generic client class to out in common::client

end

# Note this uses the same session store as Rx
def authenticate
if @session.expired?
@session = get_session
@session.save
end
self
end

private

def perform(method, path, params, headers = nil)
raise NoMethodError, "#{method} not implemented" unless REQUEST_TYPES.include?(method)

send(method, path, params || {}, headers)
end

def request(method, path, params = {}, headers = {})
raise_not_authenticated if headers.keys.include?('Token') && headers['Token'].nil?
connection.send(method.to_sym, path, params) { |request| request.headers.update(headers) }.env
rescue Faraday::Error::TimeoutError, Timeout::Error => error
raise Common::Client::Errors::RequestTimeout, error
rescue Faraday::Error::ClientError => error
raise Common::Client::Errors::Client, error
end

def get(path, params, headers = base_headers)
request(:get, path, params, headers)
end

def post(path, params, headers = base_headers)
request(:post, path, params, headers)
end

def raise_not_authenticated
raise Common::Client::Errors::NotAuthenticated, 'Not Authenticated'
end

def connection
@connection ||= Faraday.new(config.base_path, headers: BASE_REQUEST_HEADERS, request: request_options) do |conn|
conn.use :breakers
conn.request :json
# Uncomment this out for generating curl output to send to MHV dev and test only
# conn.request :curl, ::Logger.new(STDOUT), :warn

# conn.response :logger, ::Logger.new(STDOUT), bodies: true
conn.response :snakecase
conn.response :raise_error
conn.response :json_parser
conn.adapter Faraday.default_adapter
end
end

def auth_headers
BASE_REQUEST_HEADERS.merge('appToken' => config.app_token, 'mhvCorrelationId' => @session.user_id.to_s)
end

def token_headers
BASE_REQUEST_HEADERS.merge('Token' => @session.token)
end

def request_options
{
open_timeout: config.open_timeout,
timeout: config.read_timeout
}
end
end
end
10 changes: 10 additions & 0 deletions lib/mhv_logging/client_session.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# frozen_string_literal: true
require 'common/client/session'
module MHVLogging
# Session Model -- EXACT SAME AS Rx
class ClientSession < Common::Client::Session
redis_store REDIS_CONFIG['rx_store']['namespace']
redis_ttl 900
redis_key :user_id
end
end
7 changes: 7 additions & 0 deletions lib/mhv_logging/configuration.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# frozen_string_literal: true
require 'rx/configuration'
module MHVLogging
# Configuration class used to setup the environment used by client
class Configuration < Rx::Configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

How can it be a subclass of Rx::Configuration when things like the base path are pretty different between the two?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The basepath for Rx and MhvLogging are identical. In fact everything about it is, and it uses the same ClientSession redis store. I just made the clients distinct from one another rather than using Rx, because audit log can also be invoked by secure messaging, and it has distinctly different base path and different client session for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

So the requests go to URLs like #{host}/mhv-api/patient/v1/activity/auditlogout? If that's true, then we still have a problem with breakers for this. It has to be able to distinguish requests to the different services in order to tell them apart. Take a look at the matcher in Rx::Configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does breakers need to distinguish it? Rx and MHVLogging are technically the same server is my understanding, so if one is down, it would follow that the other is down. Perhaps I should just use the same Rx breaker then maybe?

Only reason why this client is not just using Rx is that it didn't make a whole lot of sense for me to invoke a client for prescriptions from secure messaging, and of course the fact that the sessions, for whatever reason are distinct between SM and Rx+MHVLogging

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, breakers needs to distinguish if it's going to report the things as being from separate services. If they should really be counted as the same, which is fine with me, I think the simplest solution would be to not add the MHVConfiguration one in the breakers initializer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change this then and possibly look at naming the configuration something like MHVConfiguation vs Rx::Configuration.

end
end
31 changes: 31 additions & 0 deletions spec/lib/mhv_logging/api/audits_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
# frozen_string_literal: true
require 'rails_helper'
require 'mhv_logging/client'

describe 'mhv logging client' do
describe 'audits' do
before(:all) do
VCR.use_cassette 'mhv_logging_client/session', record: :new_episodes do
@client ||= begin
client = MHVLogging::Client.new(session: { user_id: ENV['MHV_USER_ID'] })
client.authenticate
client
end
end
end

let(:client) { @client }

it 'submits an audit log for signing in', :vcr do
client_response = client.auditlogin
expect(client_response.status).to eq(200)
expect(client_response.body).to eq(status: 'success')
end

it 'submits an audit log for signing out', :vcr do
client_response = client.auditlogout
expect(client_response.status).to eq(200)
expect(client_response.body).to eq(status: 'success')
end
end
end

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

45 changes: 45 additions & 0 deletions spec/support/vcr_cassettes/mhv_logging_client/session.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.