-
Notifications
You must be signed in to change notification settings - Fork 0
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
create UI #1
Changes from all commits
8eb487b
6cea6de
60b798d
88bab77
6c8ac57
ba3c90b
d8617f5
9624ec7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider refactoring to reduce code duplication The 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
Suggested change
|
||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
def configure | ||||||||||||||||||||||||||||||||||
yield(configuration) | ||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||
end |
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}"]] | ||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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."]]
|
||||||||||||||||||||||||
end | ||||||||||||||||||||||||
end | ||||||||||||||||||||||||
Comment on lines
+11
to
+82
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider refactoring The
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. Toolsrubocop
|
||||||||||||||||||||||||
|
||||||||||||||||||||||||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add exception handling to If the body is not valid deflate-compressed data, 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
Suggested change
|
||||||||||||||||||||||||
end | ||||||||||||||||||||||||
end |
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 | ||||||||||||||||||||||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ( 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 Committable suggestion
Suggested change
|
||||||||||||||||||||||
request_hash = generate_request_hash(request) # Generate a hash of the request | ||||||||||||||||||||||
log_request(request, request_hash) | ||||||||||||||||||||||
|
||||||||||||||||||||||
|
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) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use In the test for memoization on line 14, you are using - 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
Suggested change
|
||||||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Example using 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 |
||||||
|
||||||
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 |
There was a problem hiding this comment.
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:
filename
to handle special characters:upload_url
configuration:These changes will improve the robustness and reliability of the
post_file
method.