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

create UI #1

Merged
merged 8 commits into from
Sep 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 3 additions & 3 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@

source 'https://rubygems.org'

gem 'rack'
gem 'puma'
gem 'thor'
gem 'json'
gem 'httpx', '~> 1.3'
gem "puma", ">= 6.4.3"
gem 'rack'
gem 'thor'

group :development, :test do
gem 'rspec', '~> 3.13'
Expand Down
6 changes: 3 additions & 3 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ GEM
httpx (1.3.0)
http-2 (>= 1.0.0)
json (2.7.1)
nio4r (2.7.1)
puma (6.4.2)
nio4r (2.7.3)
puma (6.4.3)
nio4r (~> 2.0)
rack (2.2.9)
rack-test (1.1.0)
Expand All @@ -34,7 +34,7 @@ PLATFORMS
DEPENDENCIES
httpx (~> 1.3)
json
puma
puma (>= 6.4.3)
rack
rack-test (~> 1.1)
rspec (~> 3.13)
Expand Down
3 changes: 1 addition & 2 deletions lib/command_executor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,11 @@

class CommandExecutor
attr_reader :logger, :client_uuid, :endpoint_url
REACTOR_URL = ENV['MOCKSI_REACTOR_URL'] || "https://crowllectordb.onrender.com/api/v1/reactor"

def initialize(logger, client_uuid)
@logger = logger
@client_uuid = client_uuid
@endpoint_url = REACTOR_URL
@endpoint_url = Hawksi.configuration.reactor_url
end

def execute_command(command, params)
Expand Down
14 changes: 8 additions & 6 deletions lib/file_uploader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,6 @@

class FileUploader
# FIXME: use a base URL for the upload and process URLs
UPLOAD_URL = ENV['MOCKSI_UPLOAD_URL'] || "https://crowllectordb.onrender.com/api/v1/upload"
PROCESS_URL = ENV['MOCKSI_PROCESS_URL'] || "https://crowllectordb.onrender.com/api/v1/process"

def initialize(logger, client_uuid)
@logger = logger
@client_uuid = client_uuid
Expand All @@ -19,10 +16,15 @@ def upload_files(tar_gz_files)

def process_files
HTTPX.wrap do |client|
response = client.post(PROCESS_URL, headers: { "x-client-id" => @client_uuid })
response = begin
client.post(Hawksi.configuration.process_url, headers: { "x-client-id" => @client_uuid })
rescue => e
@logger.error "Failed to process files. Error: #{e.message}"
end

if response.is_a?(HTTPX::Response)
@logger.info "Processing uploaded files. Status: #{response.status}"
else
elsif response.is_a?(HTTPX::ErrorResponse)
@logger.error "Failed to process files. Error: #{response.error}"
end
end
Expand Down Expand Up @@ -58,7 +60,7 @@ def upload_file(tar_gz_file)

def post_file(client, tar_gz_file)
filename = File.basename(tar_gz_file)
client.post("#{UPLOAD_URL}?filename=#{filename}",
client.post("#{Hawksi.configuration.upload_url}?filename=#{filename}",
Copy link

Choose a reason for hiding this comment

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

Dynamic configuration for upload URL

The change to use Hawksi.configuration.upload_url is a good improvement, aligning with the overall strategy of using dynamic configurations instead of hardcoded URLs.

However, there are two important issues from previous reviews that still need to be addressed:

  1. URI-encode the filename to handle special characters:
 def post_file(client, tar_gz_file)
   filename = File.basename(tar_gz_file)
+  encoded_filename = URI.encode_www_form_component(filename)
-  client.post("#{Hawksi.configuration.upload_url}?filename=#{filename}",
+  client.post("#{Hawksi.configuration.upload_url}?filename=#{encoded_filename}",
               headers: { "x-client-id" => @client_uuid },
               body: File.read(tar_gz_file))
 rescue => e
   @logger.error "Failed to upload #{tar_gz_file}: #{e.message}"
   nil
 end
  1. Handle missing or invalid upload_url configuration:
 def post_file(client, tar_gz_file)
+  upload_url = get_config_url('upload_url')
+  return nil if upload_url.nil?
+
   filename = File.basename(tar_gz_file)
   encoded_filename = URI.encode_www_form_component(filename)
-  client.post("#{Hawksi.configuration.upload_url}?filename=#{encoded_filename}",
+  client.post("#{upload_url}?filename=#{encoded_filename}",
               headers: { "x-client-id" => @client_uuid },
               body: File.read(tar_gz_file))
 rescue => e
   @logger.error "Failed to upload #{tar_gz_file}: #{e.message}"
   nil
 end

These changes will improve the robustness and reliability of the post_file method.

Committable suggestion was skipped due to low confidence.

headers: { "x-client-id" => @client_uuid },
body: File.read(tar_gz_file))
rescue => e
Expand Down
16 changes: 15 additions & 1 deletion lib/hawksi.rb
Original file line number Diff line number Diff line change
@@ -1,2 +1,16 @@
require 'hawksi/configuration'
require 'cli'
require 'request_interceptor'
require 'request_interceptor'


BANNER = <<~BANNER
_ _
/\ /\__ ___ _| | _____(_)
/ /_/ / _` \ \ /\ / / |/ / __| |
/ __ / (_| |\ V V /| <\__ \ |
\/ /_/ \__,_| \_/\_/ |_|\_\___/_|

BANNER


puts BANNER
29 changes: 29 additions & 0 deletions lib/hawksi/configuration.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
module Hawksi
class Configuration
attr_accessor :mocksi_server, :reactor_url, :upload_url, :process_url

def initialize
@mocksi_server = get_env('MOCKSI_SERVER') || "https://app.mocksi.ai"
@reactor_url = get_env('MOCKSI_REACTOR_URL') || "https://api.mocksi.ai/api/v1/reactor"
@upload_url = get_env('MOCKSI_UPLOAD_URL') || "https://api.mocksi.ai/api/v1/upload"
@process_url = get_env('MOCKSI_PROCESS_URL') || "https://api.mocksi.ai/api/v1/process"
end
Comment on lines +5 to +10
Copy link

Choose a reason for hiding this comment

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

Consider refactoring to reduce code duplication

The initialize method assigns values to several instance variables using a similar pattern. Refactoring this can make the code more concise and maintainable.

Apply this diff to simplify the assignments:

 def initialize
-    @mocksi_server = get_env('MOCKSI_SERVER') || "https://app.mocksi.ai"
-    @reactor_url = get_env('MOCKSI_REACTOR_URL') || "https://api.mocksi.ai/api/v1/reactor"
-    @upload_url = get_env('MOCKSI_UPLOAD_URL') || "https://api.mocksi.ai/api/v1/upload"
-    @process_url = get_env('MOCKSI_PROCESS_URL') || "https://api.mocksi.ai/api/v1/process"
+    {
+      mocksi_server: "https://app.mocksi.ai",
+      reactor_url: "https://api.mocksi.ai/api/v1/reactor",
+      upload_url: "https://api.mocksi.ai/api/v1/upload",
+      process_url: "https://api.mocksi.ai/api/v1/process"
+    }.each do |attr, default|
+      instance_variable_set("@#{attr}", get_env(attr.to_s.upcase) || default)
+    end
 end

This refactoring uses a hash to store default values and iterates over it to set instance variables dynamically.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def initialize
@mocksi_server = get_env('MOCKSI_SERVER') || "https://app.mocksi.ai"
@reactor_url = get_env('MOCKSI_REACTOR_URL') || "https://api.mocksi.ai/api/v1/reactor"
@upload_url = get_env('MOCKSI_UPLOAD_URL') || "https://api.mocksi.ai/api/v1/upload"
@process_url = get_env('MOCKSI_PROCESS_URL') || "https://api.mocksi.ai/api/v1/process"
end
def initialize
{
mocksi_server: "https://app.mocksi.ai",
reactor_url: "https://api.mocksi.ai/api/v1/reactor",
upload_url: "https://api.mocksi.ai/api/v1/upload",
process_url: "https://api.mocksi.ai/api/v1/process"
}.each do |attr, default|
instance_variable_set("@#{attr}", get_env(attr.to_s.upcase) || default)
end
end


private

def get_env(key)
env_value = ENV.fetch(key, '').strip
env_value.empty? ? nil : env_value
end
end

class << self
def configuration
@configuration ||= Configuration.new
end
Comment on lines +20 to +23
Copy link

Choose a reason for hiding this comment

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

Ensure thread-safe initialization of the configuration

If the application is running in a multithreaded environment, lazy initialization of @configuration could lead to race conditions. Eagerly initializing the configuration can prevent potential issues.

Apply this diff to initialize the configuration when the module is loaded:

 class << self
+  @configuration = Configuration.new
   def configuration
-    @configuration ||= Configuration.new
+    @configuration
   end

Initializing @configuration at the class level ensures it's set up before any threads access it.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
class << self
def configuration
@configuration ||= Configuration.new
end
class << self
@configuration = Configuration.new
def configuration
@configuration
end


def configure
yield(configuration)
end
end
end
105 changes: 105 additions & 0 deletions lib/mocksi_handler.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
require 'httpx'

HAWK_SVG = <<~SVG
<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16">
<text x="9" y="8" font-size="8" text-anchor="middle" font-family="sans-serif">Hwk</text>
</svg>
SVG

module MocksiHandler
class << self
def handle(request)
if request.path == '/favicon.ico'
return [200, { 'Content-Type' => 'image/svg+xml' }, [HAWK_SVG]]
end

begin
# Get the mocksi server URL from configuration
mocksi_server_url = Hawksi.configuration.mocksi_server
raise "Mocksi server URL not configured" if mocksi_server_url.nil? || mocksi_server_url.empty?

# Prepare the full URL (mocksi_server + request path + query string)
target_uri = URI.join(mocksi_server_url, request.fullpath)

# Prepare headers from the request
headers = {}
request.env.each do |key, value|
if key.start_with?('HTTP_')
header_key = key.sub(/^HTTP_/, '').split('_').map(&:capitalize).join('-')
headers[header_key] = value
end
## Yay for Rack's weirdness. See https://github.com/rack/rack/issues/1311
if key == 'CONTENT_TYPE'
headers['Content-Type'] = value
end
if key == 'CONTENT_LENGTH'
headers['Content-Length'] = value
end
end

# Forward the cookies
if request.cookies.any?
headers['Cookie'] = request.cookies.map { |k, v| "#{k}=#{v}" }.join('; ')
end

# Initialize httpx with headers
http_client = HTTPX.with(headers: headers)

# Forward the body content if it's a POST or PUT request
body = nil
if %w[POST PUT].include?(request.request_method)
request.body.rewind
body = request.body.read
end

# Make the HTTP request using the appropriate method
response = http_client.request(request.request_method.downcase.to_sym, target_uri, body: body)

# Clone headers to allow modification
response_headers = response.headers.dup

# Check for chunked transfer encoding and remove it
if response_headers["transfer-encoding"]&.include?("chunked")
response_headers.delete("transfer-encoding")
end

# Handle gzip or deflate content-encoding if present
response_body = response.body.to_s
if response_headers["content-encoding"]&.include?("gzip")
response_body = safe_decompress_gzip(response_body)
response_headers.delete("content-encoding") # Remove content-encoding since the content is decompressed
elsif response_headers["content-encoding"]&.include?("deflate")
response_body = decompress_deflate(response_body)
response_headers.delete("content-encoding") # Remove content-encoding since the content is decompressed
end

# Return the response in a format compatible with Rack
[response.status, response_headers, [response_body]]
rescue => e
# Handle any errors that occur during the reverse proxy operation
[500, { 'Content-Type' => 'text/plain' }, ["Error: #{e.message}"]]
Copy link

Choose a reason for hiding this comment

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

Avoid exposing internal error messages to clients

Returning internal error messages to the client may expose sensitive information and pose a security risk. It's better to log the error internally and return a generic message to the client.

Apply this diff to fix the issue:

-           [500, { 'Content-Type' => 'text/plain' }, ["Error: #{e.message}"]]
+           # Log the error message internally (implementation depends on your logging setup)
+           # logger.error(e.message)
+           [500, { 'Content-Type' => 'text/plain' }, ["An internal error occurred. Please try again later."]]

Committable suggestion was skipped due to low confidence.

end
end
Comment on lines +11 to +82
Copy link

Choose a reason for hiding this comment

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

Consider refactoring handle method to reduce complexity

The handle method has high complexity metrics as indicated by static analysis tools:

  • Assignment Branch Condition size: 59.88/23
  • Cyclomatic Complexity: 19/7
  • Perceived Complexity: 20/8

High complexity can make the code harder to read, maintain, and test. Consider refactoring by extracting some of the logic into smaller, reusable helper methods. This will improve readability and maintainability.

Tools
rubocop

[convention] 11-82: Assignment Branch Condition size for handle is too high. [<21, 52, 21> 59.88/23]

(Metrics/AbcSize)


[convention] 11-82: Cyclomatic complexity for handle is too high. [19/7]

(Metrics/CyclomaticComplexity)


[convention] 11-82: Perceived complexity for handle is too high. [20/8]

(Metrics/PerceivedComplexity)


private

# Helper method to safely decompress gzip content, returning the original body if it's not in gzip format
def safe_decompress_gzip(body)
io = StringIO.new(body)
begin
gzip_reader = Zlib::GzipReader.new(io)
decompressed_body = gzip_reader.read
gzip_reader.close
decompressed_body
rescue Zlib::GzipFile::Error
# If the body is not actually gzip, return the original body
body
end
end

# Helper method to decompress deflate content
def decompress_deflate(body)
Zlib::Inflate.inflate(body)
end
Comment on lines +101 to +103
Copy link

Choose a reason for hiding this comment

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

Add exception handling to decompress_deflate method

If the body is not valid deflate-compressed data, Zlib::Inflate.inflate(body) may raise an exception, potentially crashing the application. Consider adding exception handling to handle invalid deflate data gracefully.

Apply this diff to fix the issue:

 def decompress_deflate(body)
+  begin
     Zlib::Inflate.inflate(body)
+  rescue Zlib::DataError
+    # If the body is not actually deflate-compressed, return the original body
+    body
+  end
 end
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def decompress_deflate(body)
Zlib::Inflate.inflate(body)
end
def decompress_deflate(body)
begin
Zlib::Inflate.inflate(body)
rescue Zlib::DataError
# If the body is not actually deflate-compressed, return the original body
body
end
end

end
end
11 changes: 10 additions & 1 deletion lib/request_interceptor.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
require 'json'
require 'logger'
require 'digest'
require_relative '../lib/file_storage'
require_relative './file_storage'
require_relative './mocksi_handler'

module Hawksi
class RequestInterceptor
Expand All @@ -13,6 +14,14 @@ def initialize(app, logger: Logger.new('hawksi.log'), storage: FileStorage)

def call(env)
request = Rack::Request.new(env)

if request.path.end_with?('/favicon.ico')
return MocksiHandler.handle(request)
end

if request.path.start_with?('/mocksi') || request.path.start_with?('/_') || request.path.start_with?('/api')
return MocksiHandler.handle(request)
end
Comment on lines +18 to +24
Copy link

Choose a reason for hiding this comment

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

Simplify Conditional Logic for Request Path Handling

The two conditional statements checking the request path can be combined to improve readability and maintainability. Since both conditions result in the same action (return MocksiHandler.handle(request)), you can merge them into a single condition using start_with? and end_with? methods.

Apply this diff to combine the conditions:

-if request.path.end_with?('/favicon.ico')
-  return MocksiHandler.handle(request)
-end
-
-if request.path.start_with?('/mocksi') || request.path.start_with?('/_') || request.path.start_with?('/api')
-  return MocksiHandler.handle(request)
-end
+if request.path.end_with?('/favicon.ico') || request.path.start_with?('/mocksi', '/_', '/api')
+  return MocksiHandler.handle(request)
+end

This refactor leverages Ruby's ability to pass multiple arguments to start_with?, simplifying the condition.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if request.path.end_with?('/favicon.ico')
return MocksiHandler.handle(request)
end
if request.path.start_with?('/mocksi') || request.path.start_with?('/_') || request.path.start_with?('/api')
return MocksiHandler.handle(request)
end
if request.path.end_with?('/favicon.ico') || request.path.start_with?('/mocksi', '/_', '/api')
return MocksiHandler.handle(request)
end

request_hash = generate_request_hash(request) # Generate a hash of the request
log_request(request, request_hash)

Expand Down
81 changes: 81 additions & 0 deletions spec/lib/hawksi/configuration_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@

require 'spec_helper'
require 'hawksi/configuration'

RSpec.describe Hawksi::Configuration do
describe '.configuration' do
it 'returns a Configuration instance' do
expect(Hawksi.configuration).to be_an_instance_of(Hawksi::Configuration)
end

it 'memoizes the configuration instance' do
config1 = Hawksi.configuration
config2 = Hawksi.configuration
expect(config1).to eq(config2)
Copy link

Choose a reason for hiding this comment

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

Use equal matcher to check object identity instead of eq

In the test for memoization on line 14, you are using expect(config1).to eq(config2), which checks for value equality. To ensure that both config1 and config2 refer to the exact same object instance (i.e., memoization), you should use the equal matcher:

- expect(config1).to eq(config2)
+ expect(config1).to equal(config2)

This change will assert that the two variables point to the same object, verifying the memoization effectively.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
expect(config1).to eq(config2)
expect(config1).to equal(config2)

end
end

describe '.configure' do
it 'yields the configuration instance' do
expect { |b| Hawksi.configure(&b) }.to yield_with_args(Hawksi.configuration)
end
end

describe '#initialize' do
let(:config) { Hawksi::Configuration.new }

it 'sets default values for URLs' do
expect(config.instance_variable_get(:@mocksi_server)).to eq('https://app.mocksi.ai')
expect(config.instance_variable_get(:@reactor_url)).to eq('https://api.mocksi.ai/api/v1/reactor')
expect(config.instance_variable_get(:@upload_url)).to eq('https://api.mocksi.ai/api/v1/upload')
expect(config.instance_variable_get(:@process_url)).to eq('https://api.mocksi.ai/api/v1/process')
Comment on lines +28 to +31
Copy link

Choose a reason for hiding this comment

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

Avoid accessing instance variables directly in tests

In your tests on lines 28-31, 51-54, and 74-77, you're using instance_variable_get to access private instance variables (@mocksi_server, @reactor_url, etc.). Directly accessing instance variables breaks encapsulation and can make tests brittle. Instead, consider adding public accessor methods to Hawksi::Configuration and testing via those methods.

Add the following accessor methods to your configuration class:

attr_reader :mocksi_server, :reactor_url, :upload_url, :process_url

Update your tests accordingly:

- expect(config.instance_variable_get(:@mocksi_server)).to eq('https://app.mocksi.ai')
+ expect(config.mocksi_server).to eq('https://app.mocksi.ai')

This approach adheres to good object-oriented principles and keeps your tests resilient to internal changes of the class.

Also applies to: 51-54, 74-77

end

context 'when environment variables are set' do
before do
ENV['MOCKSI_SERVER'] = 'https://custom.mocksi.ai'
ENV['MOCKSI_REACTOR_URL'] = 'https://custom.mocksi.ai/reactor'
ENV['MOCKSI_UPLOAD_URL'] = 'https://custom.mocksi.ai/upload'
ENV['MOCKSI_PROCESS_URL'] = 'https://custom.mocksi.ai/process'
end

after do
ENV.delete('MOCKSI_SERVER')
ENV.delete('MOCKSI_REACTOR_URL')
ENV.delete('MOCKSI_UPLOAD_URL')
ENV.delete('MOCKSI_PROCESS_URL')
end
Comment on lines +35 to +47
Copy link

Choose a reason for hiding this comment

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

Isolate environment variable changes to prevent side effects

Modifying environment variables directly in the before and after blocks can lead to side effects affecting other tests. Consider using around hooks or helper methods to temporarily set environment variables for the scope of your tests. Alternatively, you can use gems like climate_control to manage environment variables safely.

Example using around hook:

around do |example|
  original_env = ENV.to_hash
  ENV['MOCKSI_SERVER'] = 'https://custom.mocksi.ai'
  # ... set other environment variables
  example.run
  ENV.replace(original_env)
end

This ensures that any changes to ENV are scoped to the test and cleaned up afterward, preventing interference with other tests.


it 'uses environment variables for URLs' do
config = Hawksi::Configuration.new
expect(config.instance_variable_get(:@mocksi_server)).to eq('https://custom.mocksi.ai')
expect(config.instance_variable_get(:@reactor_url)).to eq('https://custom.mocksi.ai/reactor')
expect(config.instance_variable_get(:@upload_url)).to eq('https://custom.mocksi.ai/upload')
expect(config.instance_variable_get(:@process_url)).to eq('https://custom.mocksi.ai/process')
end
end
context 'when a value is set to an empty string' do
before do
ENV['MOCKSI_SERVER'] = ''
ENV['MOCKSI_REACTOR_URL'] = ''
ENV['MOCKSI_UPLOAD_URL'] = ''
ENV['MOCKSI_PROCESS_URL'] = ''
end

after do
ENV.delete('MOCKSI_SERVER')
ENV.delete('MOCKSI_REACTOR_URL')
ENV.delete('MOCKSI_UPLOAD_URL')
ENV.delete('MOCKSI_PROCESS_URL')
end

it 'uses default values for empty string environment variables' do
config = Hawksi::Configuration.new
expect(config.instance_variable_get(:@mocksi_server)).to eq('https://app.mocksi.ai')
expect(config.instance_variable_get(:@reactor_url)).to eq('https://api.mocksi.ai/api/v1/reactor')
expect(config.instance_variable_get(:@upload_url)).to eq('https://api.mocksi.ai/api/v1/upload')
expect(config.instance_variable_get(:@process_url)).to eq('https://api.mocksi.ai/api/v1/process')
end
end
end
end
Loading