From 5762d1dd5028c9fb3c3e002b87689dada9861e55 Mon Sep 17 00:00:00 2001 From: Tanmay Deep Sharma <32020192+tds-1@users.noreply.github.com> Date: Thu, 3 Jul 2025 12:24:07 +0530 Subject: [PATCH 1/2] refactor: review changes --- ...00_add_pdf_support_to_captain_documents.rb | 8 + ...dd_source_metadata_to_captain_documents.rb | 11 - .../accounts/captain/documents_controller.rb | 150 ++--------- .../app/jobs/captain/documents/crawl_job.rb | 67 +---- .../captain/documents/pdf_extraction_job.rb | 61 +++++ .../tools/pdf_extraction_parser_job.rb | 234 ++++-------------- enterprise/app/models/captain/document.rb | 27 +- .../captain/tools/pdf_extraction_service.rb | 195 +-------------- .../pdf_extraction_service/blob_handler.rb | 49 ++++ .../pdf_extraction_service/error_handler.rb | 44 ++++ .../pdf_extraction_service/source_handler.rb | 67 +++++ .../pdf_extraction_service/text_processor.rb | 46 ++++ .../captain/tools/pdf_validation_concern.rb | 85 ++----- .../captain/documents/create.json.jbuilder | 3 - .../v1/models/captain/_document.json.jbuilder | 11 + .../captain/documents_controller_spec.rb | 2 +- .../jobs/captain/documents/crawl_job_spec.rb | 108 ++------ .../documents/pdf_extraction_job_spec.rb | 148 +++++++++++ .../tools/pdf_extraction_parser_job_spec.rb | 144 ++++++----- 19 files changed, 628 insertions(+), 832 deletions(-) create mode 100644 db/migrate/20250702075600_add_pdf_support_to_captain_documents.rb delete mode 100644 db/migrate/20250702075600_add_source_metadata_to_captain_documents.rb create mode 100644 enterprise/app/jobs/captain/documents/pdf_extraction_job.rb create mode 100644 enterprise/app/services/captain/tools/pdf_extraction_service/blob_handler.rb create mode 100644 enterprise/app/services/captain/tools/pdf_extraction_service/error_handler.rb create mode 100644 enterprise/app/services/captain/tools/pdf_extraction_service/source_handler.rb create mode 100644 enterprise/app/services/captain/tools/pdf_extraction_service/text_processor.rb create mode 100644 spec/enterprise/jobs/captain/documents/pdf_extraction_job_spec.rb diff --git a/db/migrate/20250702075600_add_pdf_support_to_captain_documents.rb b/db/migrate/20250702075600_add_pdf_support_to_captain_documents.rb new file mode 100644 index 000000000..2a5d8bc20 --- /dev/null +++ b/db/migrate/20250702075600_add_pdf_support_to_captain_documents.rb @@ -0,0 +1,8 @@ +class AddPdfSupportToCaptainDocuments < ActiveRecord::Migration[7.1] + def change + add_column :captain_documents, :source_type, :string, default: 'url' + add_column :captain_documents, :processed_at, :datetime + + add_index :captain_documents, :source_type + end +end \ No newline at end of file diff --git a/db/migrate/20250702075600_add_source_metadata_to_captain_documents.rb b/db/migrate/20250702075600_add_source_metadata_to_captain_documents.rb deleted file mode 100644 index 78eb05c3e..000000000 --- a/db/migrate/20250702075600_add_source_metadata_to_captain_documents.rb +++ /dev/null @@ -1,11 +0,0 @@ -class AddSourceMetadataToCaptainDocuments < ActiveRecord::Migration[7.1] - def change - add_column :captain_documents, :source_type, :string, default: 'url' - add_column :captain_documents, :content_type, :string - add_column :captain_documents, :file_size, :integer - add_column :captain_documents, :processed_at, :datetime - - add_index :captain_documents, :source_type - add_index :captain_documents, :content_type - end -end \ No newline at end of file diff --git a/enterprise/app/controllers/api/v1/accounts/captain/documents_controller.rb b/enterprise/app/controllers/api/v1/accounts/captain/documents_controller.rb index 98e64af82..8897e1cdc 100644 --- a/enterprise/app/controllers/api/v1/accounts/captain/documents_controller.rb +++ b/enterprise/app/controllers/api/v1/accounts/captain/documents_controller.rb @@ -3,15 +3,11 @@ class Api::V1::Accounts::Captain::DocumentsController < Api::V1::Accounts::BaseC before_action -> { check_authorization(Captain::Assistant) } before_action :set_current_page, only: [:index] - before_action :set_documents, except: [:create] + before_action :set_documents, except: [:create, :upload_pdf] before_action :set_document, only: [:show, :destroy] before_action :set_assistant, only: [:create, :upload_pdf] RESULTS_PER_PAGE = 25 - # Fixed PDF size limit - MAX_PDF_SIZE = 25.megabytes - ALLOWED_PDF_CONTENT_TYPES = ['application/pdf'].freeze - PDF_MAGIC_NUMBERS = ['%PDF'].freeze def index base_query = @documents @@ -33,16 +29,20 @@ class Api::V1::Accounts::Captain::DocumentsController < Api::V1::Accounts::BaseC end def upload_pdf - validation_errors = validate_upload_prerequisites - return render_could_not_create_error(validation_errors) if validation_errors + return render_could_not_create_error('Missing Assistant') if @assistant.nil? + return render_could_not_create_error('No PDF file provided') if pdf_params[:pdf_document].blank? - process_pdf_upload + @document = @assistant.documents.build( + name: pdf_params[:pdf_document].original_filename, + external_link: "pdf_upload_#{SecureRandom.hex(8)}.pdf", + source_type: 'pdf_upload' + ) + @document.file.attach(pdf_params[:pdf_document]) + @document.save! + + render :create rescue Captain::Document::LimitExceededError => e - handle_limit_exceeded_error(e) - rescue ActiveStorage::FileNotFoundError => e - handle_file_not_found_error(e) - rescue StandardError => e - handle_general_upload_error(e) + render_could_not_create_error(e.message) end def destroy @@ -80,126 +80,4 @@ class Api::V1::Accounts::Captain::DocumentsController < Api::V1::Accounts::BaseC def pdf_params params.permit(:pdf_document, :assistant_id) end - - def pdf_file_present? - pdf_params[:pdf_document].present? - end - - def validate_pdf_file - file = pdf_params[:pdf_document] - - file_object_error = validate_file_object(file) - return file_object_error if file_object_error - - file_type_error = validate_file_type(file) - return file_type_error if file_type_error - - file_size_error = validate_file_size(file) - return file_size_error if file_size_error - - { valid: true } - end - - def validate_file_object(file) - return nil if file.respond_to?(:content_type) && file.respond_to?(:size) - - { valid: false, error: 'Invalid file object' } - end - - def validate_file_type(file) - return nil if ALLOWED_PDF_CONTENT_TYPES.include?(file.content_type) - - { valid: false, error: 'Invalid file type. Only PDF files are allowed.' } - end - - def validate_file_size(file) - return nil if file.size <= MAX_PDF_SIZE - - { valid: false, error: "File size too large. Maximum size is #{MAX_PDF_SIZE / 1.megabyte}MB." } - end - - def create_pdf_blob - file = pdf_params[:pdf_document] - - ActiveStorage::Blob.create_and_upload!( - io: file.tempfile, - filename: sanitize_filename(file.original_filename), - content_type: file.content_type, - metadata: { - uploaded_by: Current.user&.id, - assistant_id: @assistant.id, - account_id: Current.account.id, - original_filename: file.original_filename - } - ) - rescue StandardError => e - Rails.logger.error "Failed to create PDF blob: #{e.message}" - raise ActiveStorage::FileNotFoundError, 'Failed to upload PDF file' - end - - def generate_pdf_url(blob) - Rails.application.routes.url_helpers.rails_blob_url( - blob, - host: ENV.fetch('FRONTEND_URL') { Rails.application.config.action_mailer.default_url_options[:host] } - ) - end - - def sanitize_filename(filename) - return 'document' if filename.blank? - - base_name = File.basename(filename, '.pdf') - sanitized = base_name.gsub(/[^\w\s.-]/, '').strip.squeeze(' ') - (sanitized.presence || 'document') - end - - def process_pdf_upload - ActiveRecord::Base.transaction do - blob = create_pdf_blob - pdf_url = generate_pdf_url(blob) - - @document = @assistant.documents.build( - name: sanitize_filename(pdf_params[:pdf_document].original_filename), - external_link: pdf_url, - source_type: 'pdf_upload', - content_type: pdf_params[:pdf_document].content_type, - file_size: pdf_params[:pdf_document].size - ) - @document.save! - - log_pdf_upload_success - - # Use the same response structure as create action for consistency - render :create - end - end - - def log_pdf_upload_success - Rails.logger.info "PDF uploaded successfully - Document ID: #{@document.id}, Assistant ID: #{@assistant.id}, Account ID: #{Current.account.id}" - end - - def validate_upload_prerequisites - return 'Missing Assistant' if @assistant.nil? - return 'No PDF file provided' unless pdf_file_present? - - validation_result = validate_pdf_file - return validation_result[:error] unless validation_result[:valid] - - nil - end - - def handle_limit_exceeded_error(error) - Rails.logger.warn "Document limit exceeded for assistant #{@assistant.id}: #{error.message}" - render_could_not_create_error(error.message) - end - - def handle_file_not_found_error(error) - Rails.logger.error "PDF file not found during upload: #{error.message}" - render_could_not_create_error('PDF file could not be processed. Please try again.') - end - - def handle_general_upload_error(error) - Rails.logger.error "PDF upload failed for assistant #{@assistant&.id}: #{error.message}" - Rails.logger.error error.backtrace.join("\n") - render_could_not_create_error('PDF upload failed. Please try again.') - end -end +end \ No newline at end of file diff --git a/enterprise/app/jobs/captain/documents/crawl_job.rb b/enterprise/app/jobs/captain/documents/crawl_job.rb index 3b3c70dd2..4911c5aa1 100644 --- a/enterprise/app/jobs/captain/documents/crawl_job.rb +++ b/enterprise/app/jobs/captain/documents/crawl_job.rb @@ -3,7 +3,7 @@ class Captain::Documents::CrawlJob < ApplicationJob def perform(document) if pdf_document?(document) - perform_pdf_extraction(document) + Captain::Documents::PdfExtractionJob.perform_later(document) elsif InstallationConfig.find_by(name: 'CAPTAIN_FIRECRAWL_API_KEY')&.value.present? perform_firecrawl_crawl(document) else @@ -22,17 +22,7 @@ class Captain::Documents::CrawlJob < ApplicationJob include Captain::FirecrawlHelper def pdf_by_metadata?(document) - pdf_by_source_type?(document) || pdf_by_content_type?(document) - rescue StandardError - false - end - - def pdf_by_source_type?(document) - document.respond_to?(:source_type) && document.source_type == 'pdf_upload' - end - - def pdf_by_content_type?(document) - document.respond_to?(:content_type) && document.content_type&.include?('application/pdf') + document.source_type == 'pdf_upload' || document.file.attached? end def pdf_by_url?(document) @@ -44,21 +34,6 @@ class Captain::Documents::CrawlJob < ApplicationJob (url.include?('blob') && url.include?('pdf')) end - def perform_pdf_extraction(document) - document.update(status: 'in_progress') - - pdf_extraction_service = Captain::Tools::PdfExtractionService.new(document.external_link) - result = pdf_extraction_service.perform - - if result[:success] && result[:content].present? - process_pdf_content_chunks(document, result[:content]) - else - handle_pdf_extraction_failure(document, result) - end - rescue StandardError => e - handle_pdf_extraction_error(document, e) - end - def perform_simple_crawl(document) page_links = Captain::Tools::SimplePageCrawlService.new(document.external_link).page_links @@ -94,42 +69,4 @@ class Captain::Documents::CrawlJob < ApplicationJob "#{webhook_url}?assistant_id=#{document.assistant_id}&token=#{generate_firecrawl_token(document.assistant_id, document.account_id)}" end - - def process_pdf_content_chunks(document, content_chunks) - Rails.logger.info "PDF extraction successful for document #{document.id}: #{content_chunks.length} chunks will be processed" - - content_chunks.each_with_index do |content_chunk, index| - log_chunk_queueing(document, content_chunk, index, content_chunks.length) - queue_pdf_chunk_job(document, content_chunk) - end - - document.update(status: 'in_progress', processed_at: nil) - Rails.logger.info "All #{content_chunks.length} chunks queued for processing for document #{document.id}" - end - - def log_chunk_queueing(document, content_chunk, index, total_chunks) - page_num = content_chunk[:page_number] - content_length = content_chunk[:content].length - Rails.logger.info "Queueing chunk #{index + 1}/#{total_chunks} for document #{document.id} " \ - "(Page #{page_num}, #{content_length} chars)" - end - - def queue_pdf_chunk_job(document, content_chunk) - Captain::Tools::PdfExtractionParserJob.perform_later( - assistant_id: document.assistant_id, - pdf_content: content_chunk, - document_id: document.id - ) - end - - def handle_pdf_extraction_failure(document, result) - error_message = result[:errors]&.join(', ') || 'Failed to extract text from PDF' - Rails.logger.error "PDF extraction failed for document #{document.id}: #{error_message}" - document.update(status: 'available') - end - - def handle_pdf_extraction_error(document, error) - Rails.logger.error "PDF extraction failed for document #{document.id}: #{error.message}" - document.update(status: 'available') - end end diff --git a/enterprise/app/jobs/captain/documents/pdf_extraction_job.rb b/enterprise/app/jobs/captain/documents/pdf_extraction_job.rb new file mode 100644 index 000000000..aef02dfb1 --- /dev/null +++ b/enterprise/app/jobs/captain/documents/pdf_extraction_job.rb @@ -0,0 +1,61 @@ +class Captain::Documents::PdfExtractionJob < ApplicationJob + queue_as :low + + def perform(document) + return unless document.pdf_document? + + document.update(status: 'in_progress') + + pdf_source = document.file.attached? ? document.file : document.external_link + pdf_extraction_service = Captain::Tools::PdfExtractionService.new(pdf_source) + result = pdf_extraction_service.perform + + if result[:success] && result[:content].present? + process_pdf_content_chunks(document, result[:content]) + else + handle_pdf_extraction_failure(document, result) + end + rescue Captain::Tools::PdfExtractionService::ExtractionError => e + handle_pdf_extraction_error(document, e) + end + + private + + def process_pdf_content_chunks(document, content_chunks) + Rails.logger.info "PDF extraction successful for document #{document.id}: #{content_chunks.length} chunks will be processed" + + content_chunks.each_with_index do |content_chunk, index| + log_chunk_queueing(document, content_chunk, index, content_chunks.length) + queue_pdf_chunk_job(document, content_chunk) + end + + document.update(status: 'in_progress', processed_at: nil) + Rails.logger.info "All #{content_chunks.length} chunks queued for processing for document #{document.id}" + end + + def log_chunk_queueing(document, content_chunk, index, total_chunks) + page_num = content_chunk[:page_number] + content_length = content_chunk[:content].length + Rails.logger.info "Queueing chunk #{index + 1}/#{total_chunks} for document #{document.id} " \ + "(Page #{page_num}, #{content_length} chars)" + end + + def queue_pdf_chunk_job(document, content_chunk) + Captain::Tools::PdfExtractionParserJob.perform_later( + assistant_id: document.assistant_id, + pdf_content: content_chunk, + document_id: document.id + ) + end + + def handle_pdf_extraction_failure(document, result) + error_message = result[:errors]&.join(', ') || 'Failed to extract text from PDF' + Rails.logger.error "PDF extraction failed for document #{document.id}: #{error_message}" + document.update(status: 'available') + end + + def handle_pdf_extraction_error(document, error) + Rails.logger.error "PDF extraction failed for document #{document.id}: #{error.message}" + document.update(status: 'available') + end +end \ No newline at end of file diff --git a/enterprise/app/jobs/captain/tools/pdf_extraction_parser_job.rb b/enterprise/app/jobs/captain/tools/pdf_extraction_parser_job.rb index 2a0413f15..546e2c4ba 100644 --- a/enterprise/app/jobs/captain/tools/pdf_extraction_parser_job.rb +++ b/enterprise/app/jobs/captain/tools/pdf_extraction_parser_job.rb @@ -1,226 +1,76 @@ -require 'securerandom' - class Captain::Tools::PdfExtractionParserJob < ApplicationJob queue_as :low - retry_on StandardError, wait: :exponentially_longer, attempts: 3 def perform(assistant_id:, pdf_content:, document_id: nil) - validate_inputs!(assistant_id, pdf_content, document_id) + assistant = Captain::Assistant.find(assistant_id) + content = pdf_content[:content] - assistant = load_assistant(assistant_id) - content_data = extract_content_data(pdf_content) + return if content.blank? || limit_exceeded?(assistant.account) - process_document(assistant, content_data, document_id) + document = create_document(assistant, pdf_content, document_id) + Captain::Documents::ResponseBuilderJob.perform_later(document) if document rescue ActiveRecord::RecordNotFound => e - handle_record_not_found_error(e, document_id) - rescue StandardError => e - handle_processing_error(e, assistant_id, document_id) + Rails.logger.error "PDF parser job failed - Assistant not found: #{e.message}" + rescue ActiveRecord::RecordInvalid => e + Rails.logger.error "PDF parser job failed - Invalid document data: #{e.message}" + rescue Captain::Document::LimitExceededError => e + Rails.logger.info "PDF parser job stopped - Document limit exceeded: #{e.message}" end private - def validate_inputs!(assistant_id, pdf_content, _document_id) - raise ArgumentError, 'Assistant ID is required' if assistant_id.blank? - raise ArgumentError, 'PDF content is required' if pdf_content.blank? - end - - def extract_content_data(pdf_content) - { - content: pdf_content[:content] || '', - page_number: pdf_content[:page_number] || 1, - chunk_index: pdf_content[:chunk_index] || 1, - total_chunks: pdf_content[:total_chunks] || 1 - } - end - - def load_assistant(assistant_id) - Captain::Assistant.find(assistant_id) - end - - def processing_should_skip?(assistant) - exceeded = limit_exceeded?(assistant.account) - Rails.logger.info "Document limit exceeded for account #{assistant.account.id}" if exceeded - exceeded - end - - def load_document(document_id) - return nil if document_id.blank? - - Captain::Document.find(document_id) - end - - def log_chunk_processing(document_id, content_data) - Rails.logger.info "Processing PDF chunk for document #{document_id}: " \ - "page #{content_data[:page_number]}, " \ - "chunk #{content_data[:chunk_index]}/#{content_data[:total_chunks]}" - end - - def update_document_content(document, content_data) - document.update!( - content: content_data[:content], - status: 'available', - processed_at: Time.current - ) - - # Trigger FAQ generation for this chunk - Captain::Documents::ResponseBuilderJob.perform_later(document) - rescue StandardError => e - Rails.logger.error "Failed to parse PDF content for document #{document.id}: #{e.message}" - raise "Failed to parse PDF data: #{e.message}" - end - - def create_new_document(assistant, content_data, document_id = nil) - document_params = build_document_params(assistant, content_data, document_id) - - new_document = Captain::Document.create!(document_params) - - # Trigger FAQ generation for this chunk - Captain::Documents::ResponseBuilderJob.perform_later(new_document) - - new_document - rescue Captain::Document::LimitExceededError - Rails.logger.info "Document limit exceeded for account #{assistant.account.id}" - return false - rescue StandardError => e - Rails.logger.error "Failed to parse PDF content for assistant #{assistant.id}: #{e.message}" - raise "Failed to parse PDF data: #{e.message}" - end - - def build_document_params(assistant, content_data, document_id) - title = generate_content_title( - content_data[:content], - content_data[:page_number], - content_data[:chunk_index], - content_data[:total_chunks] - ) - - external_link = generate_external_link(document_id, content_data) - - { + def create_document(assistant, pdf_content, document_id) + Captain::Document.create!( assistant: assistant, account: assistant.account, - content: content_data[:content], - name: title, - external_link: external_link, + content: pdf_content[:content], + name: generate_title(pdf_content), + external_link: generate_link(document_id, pdf_content), status: 'available', - source_type: 'pdf_upload', - content_type: 'application/pdf' - } + source_type: 'pdf_upload' + ) + rescue Captain::Document::LimitExceededError + Rails.logger.info "Document limit exceeded for account #{assistant.account.id}" + nil end - def generate_external_link(document_id, content_data) - page_number = content_data[:page_number] - chunk_index = content_data[:chunk_index] - - if document_id && page_number && chunk_index - "pdf_chunk_#{document_id}_page_#{page_number}_chunk_#{chunk_index}" - elsif page_number && chunk_index - "pdf_chunk_#{SecureRandom.hex(8)}_page_#{page_number}_chunk_#{chunk_index}" - else - "pdf_chunk_#{SecureRandom.hex(8)}" - end - end - - def generate_content_title(content, page_number, chunk_index, total_chunks) - return 'PDF Content' if content.blank? - - base_title = extract_base_title(content) - add_page_chunk_info(base_title, page_number, chunk_index, total_chunks) + def generate_title(pdf_content) + base_title = extract_base_title(pdf_content[:content]) + title_with_page_info = add_page_information(base_title, pdf_content) + title_with_page_info.truncate(255) end def extract_base_title(content) first_line = content.split("\n").first&.strip || '' + return 'PDF Content' if first_line.length > 50 || first_line.blank? - base_title = if first_line.include?('.') - extract_sentence_title(first_line) - else - first_line - end - - # Use generic title for long or blank content - base_title.length > 100 || base_title.blank? ? 'PDF Content' : base_title + first_line end - def extract_sentence_title(first_line) - first_sentence = first_line.split('.').first&.strip || '' - # Only add period if the original content was actually a complete sentence - if first_line.split('.').length > 1 && first_line.split('.')[1].strip.present? - "#{first_sentence}." - else - first_sentence - end - end + def add_page_information(base_title, pdf_content) + page = pdf_content[:page_number] + chunk = pdf_content[:chunk_index] + total = pdf_content[:total_chunks] - def add_page_chunk_info(base_title, page_number, chunk_index, total_chunks) - title_with_info = build_title_with_chunk_info(base_title, page_number, chunk_index, total_chunks) - truncate_title(title_with_info) - end - - def build_title_with_chunk_info(base_title, page_number, chunk_index, total_chunks) - return "#{base_title} (Page #{page_number}, Part #{chunk_index}/#{total_chunks})" if total_chunks > 1 - return "#{base_title} (Page #{page_number})" if page_number && page_number > 1 + return "#{base_title} (Page #{page}, Part #{chunk}/#{total})" if total && total > 1 + return "#{base_title} (Page #{page})" if page && page > 1 base_title end - def truncate_title(title) - title.length > 255 ? "#{title[0, 252]}..." : title + def generate_link(document_id, pdf_content) + page = pdf_content[:page_number] + chunk = pdf_content[:chunk_index] + + if document_id + "pdf_chunk_#{document_id}_page_#{page}_chunk_#{chunk}" + else + "pdf_chunk_#{SecureRandom.hex(8)}_page_#{page}_chunk_#{chunk}" + end end def limit_exceeded?(account) limits = account.usage_limits.dig(:captain, :documents) - return false unless limits - - limits[:current_available].to_i <= 0 - end - - def handle_record_not_found_error(error, _document_id) - Rails.logger.error "Record not found: #{error.message}" - raise "Failed to parse PDF data: #{error.message}" - end - - def handle_processing_error(error, assistant_id, document_id) - Rails.logger.error "Failed to parse PDF content for assistant #{assistant_id}: #{error.message}" - - update_document_error_status(document_id, error) if document_id.present? - - raise "Failed to parse PDF data: #{error.message}" - end - - def update_document_error_status(document_id, error) - return if should_skip_document_update?(error) - - document = Captain::Document.find_by(id: document_id) - document&.update(status: 'in_progress') - rescue StandardError => e - Rails.logger.error "Failed to update document status: #{e.message}" - end - - def should_skip_document_update?(error) - error.message.include?('Database error') - end - - def process_document(assistant, content_data, document_id) - return create_new_document_with_limit_check(assistant, content_data, document_id) if document_id.blank? - - process_existing_document(assistant, content_data, document_id) - end - - def process_existing_document(assistant, content_data, document_id) - existing_document = Captain::Document.find_by(id: document_id) - - if existing_document - log_chunk_processing(document_id, content_data) - update_document_content(existing_document, content_data) - else - create_new_document_with_limit_check(assistant, content_data, document_id) - end - end - - def create_new_document_with_limit_check(assistant, content_data, document_id) - result = create_new_document(assistant, content_data, document_id) - return false if result == false # Limits exceeded - - result + limits && limits[:current_available].to_i <= 0 end end \ No newline at end of file diff --git a/enterprise/app/models/captain/document.rb b/enterprise/app/models/captain/document.rb index a64efcc1e..5d5f765f2 100644 --- a/enterprise/app/models/captain/document.rb +++ b/enterprise/app/models/captain/document.rb @@ -34,11 +34,12 @@ class Captain::Document < ApplicationRecord belongs_to :assistant, class_name: 'Captain::Assistant' has_many :responses, class_name: 'Captain::AssistantResponse', dependent: :destroy, as: :documentable belongs_to :account + has_one_attached :file validates :external_link, presence: true validates :external_link, uniqueness: { scope: :assistant_id } validates :content, length: { maximum: 400_000 } - validates :source_type, inclusion: { in: %w[url pdf_upload] } + validates :source_type, inclusion: { in: %w[url pdf_upload] }, allow_blank: true before_validation :ensure_account_id before_validation :set_default_source_type @@ -57,6 +58,10 @@ class Captain::Document < ApplicationRecord scope :for_account, ->(account_id) { where(account_id: account_id) } scope :for_assistant, ->(assistant_id) { where(assistant_id: assistant_id) } + def pdf_document? + source_type == 'pdf_upload' || file.attached? || pdf_url_format? + end + private def enqueue_crawl_job @@ -89,33 +94,17 @@ class Captain::Document < ApplicationRecord def set_default_source_type return if source_type.present? - # Determine type without calling pdf_document? to avoid circular dependency - self.source_type = if content_type&.include?('application/pdf') || pdf_url_format? + self.source_type = if file.attached? || pdf_url_format? 'pdf_upload' else 'url' end end - # Public method used by CrawlJob to determine if document is a PDF - def pdf_document? - pdf_upload? || pdf_content_type? || pdf_url_format? - end - - def pdf_upload? - source_type == 'pdf_upload' - end - - def pdf_content_type? - content_type&.include?('application/pdf') - end - def pdf_url_format? return false if external_link.blank? url = external_link.downcase - url.end_with?('.pdf') || - url.include?('/rails/active_storage/blobs/') || - (url.include?('blob') && url.include?('pdf')) + url.end_with?('.pdf') || url.include?('/rails/active_storage/blobs/') end end diff --git a/enterprise/app/services/captain/tools/pdf_extraction_service.rb b/enterprise/app/services/captain/tools/pdf_extraction_service.rb index d0dcc926b..af50e3a67 100644 --- a/enterprise/app/services/captain/tools/pdf_extraction_service.rb +++ b/enterprise/app/services/captain/tools/pdf_extraction_service.rb @@ -4,6 +4,12 @@ class Captain::Tools::PdfExtractionService include ActiveModel::Validations include Captain::Tools::PdfValidationConcern include Captain::Tools::PdfContentChunkingConcern + include Captain::Tools::PdfExtractionService::SourceHandler + include Captain::Tools::PdfExtractionService::BlobHandler + include Captain::Tools::PdfExtractionService::TextProcessor + include Captain::Tools::PdfExtractionService::ErrorHandler + + class ExtractionError < StandardError; end attr_reader :pdf_source, :errors @@ -26,167 +32,8 @@ class Captain::Tools::PdfExtractionService extract_pdf_with_error_handling end - def extract_pdf_with_error_handling - process_pdf_extraction - rescue PDF::Reader::MalformedPDFError => e - handle_malformed_pdf_error(e) - rescue PDF::Reader::UnsupportedFeatureError => e - handle_unsupported_feature_error(e) - rescue Down::TimeoutError => e - handle_timeout_error(e) - rescue StandardError => e - handle_general_error(e) - end - - def extract_text - case determine_source_type - when :url - active_storage_blob_url? ? extract_from_active_storage_blob : extract_from_url - when :uploaded_file - extract_from_uploaded_file - else - extract_from_file_path - end - end - private - def determine_source_type - return :url if pdf_source.is_a?(String) && pdf_source.start_with?('http') - return :uploaded_file if pdf_source.respond_to?(:tempfile) - - :file_path - end - - def extract_from_url - temp_file = Down.download( - pdf_source, - max_size: MAX_PDF_SIZE, - open_timeout: DOWNLOAD_TIMEOUT, - read_timeout: DOWNLOAD_TIMEOUT - ) - - begin - result = extract_from_file(temp_file.path) - ensure - temp_file.close - temp_file.unlink - end - - result - end - - def extract_from_uploaded_file - extract_from_file(pdf_source.tempfile.path) - end - - def extract_from_file_path - extract_from_file(pdf_source) - end - - def active_storage_blob_url? - pdf_source.include?('/rails/active_storage/blobs/') || - (pdf_source.include?('blob') && pdf_source.include?('pdf')) - end - - def extract_from_active_storage_blob - blob = find_blob_from_url - return extract_blob_content(blob) if blob - - Rails.logger.warn 'ActiveStorage blob not found, falling back to URL download' - extract_from_url - end - - def extract_blob_content(blob) - blob.open { |file| extract_from_file(file.path) } - end - - def find_blob_from_url - blob_key = extract_blob_key_from_url - return nil if blob_key.blank? - - ActiveStorage::Blob.find_by(key: blob_key) - rescue StandardError => e - Rails.logger.error "Error finding blob with key '#{blob_key}': #{e.message}" - nil - end - - def extract_blob_key_from_url - blob_key = if pdf_source.include?('/rails/active_storage/blobs/') - extract_key_from_rails_path - elsif pdf_source.include?('/blobs/') - extract_key_from_blob_path - end - - blob_key&.split('?')&.first # Remove query parameters - end - - def extract_key_from_rails_path - parts = pdf_source.split('/rails/active_storage/blobs/').last.split('/') - parts.length > 1 && parts[0] != 'redirect' ? parts[0] : parts[1] - end - - def extract_key_from_blob_path - pdf_source.split('/blobs/').last.split('/').first - end - - def extract_from_file(file_path) - text_content = [] - - PDF::Reader.open(file_path) do |reader| - reader.pages.each_with_index do |page, index| - page_content = extract_page_content(page, index) - text_content << page_content if page_content - end - end - - text_content - end - - def extract_page_content(page, index) - page_text = page.text - return nil if page_text.blank? - - cleaned_text = clean_text(page_text) - return nil if cleaned_text.blank? - - { page_number: index + 1, content: cleaned_text } - rescue StandardError => e - Rails.logger.warn "Failed to extract text from page #{index + 1}: #{e.message}" - nil - end - - def clean_text(text) - # Remove form feeds and normalize whitespace - cleaned = text.tr("\f", "\n") - .gsub("\r\n", "\n") - .tr("\r", "\n") - .gsub(/\s+/, ' ') - .gsub(/\n\s*\n\s*\n+/, "\n\n") - .strip - - # Remove common PDF artifacts - cleaned = cleaned.gsub(/^\d+\s*$/, '') # Remove standalone page numbers - .gsub(/^[\s\-_=]+$/, '') # Remove separator lines - .strip - - (cleaned.presence) - end - - def pdf_source_type - case pdf_source - when String - pdf_source.start_with?('http') ? 'URL' : 'file_path' - else - 'uploaded_file' - end - end - - def log_extraction_success(chunked_content) - total_chars = chunked_content.sum { |chunk| chunk[:content].length } - Rails.logger.info "PDF extraction completed: #{chunked_content.length} chunks, #{total_chars} characters" - end - def process_pdf_extraction content = extract_text return failure_response(['No text content found in PDF']) if content.blank? @@ -197,32 +44,8 @@ class Captain::Tools::PdfExtractionService success_response(chunked_content) end - def success_response(content) - { success: true, content: content } - end - - def failure_response(errors) - { success: false, errors: errors } - end - - def handle_malformed_pdf_error(error) - Rails.logger.error "Malformed PDF (#{pdf_source_type}): #{error.message}" - failure_response(['Invalid or corrupted PDF format']) - end - - def handle_unsupported_feature_error(error) - Rails.logger.error "Unsupported PDF feature (#{pdf_source_type}): #{error.message}" - failure_response(['PDF contains unsupported features']) - end - - def handle_timeout_error(error) - Rails.logger.error "PDF download timeout (#{pdf_source_type}): #{error.message}" - failure_response(['PDF download timed out']) - end - - def handle_general_error(error) - Rails.logger.error "PDF extraction error (#{pdf_source_type}): #{error.message}" - Rails.logger.error error.backtrace.join("\n") - failure_response(['Failed to process PDF']) + def log_extraction_success(chunked_content) + total_chars = chunked_content.sum { |chunk| chunk[:content].length } + Rails.logger.info "PDF extraction completed: #{chunked_content.length} chunks, #{total_chars} characters" end end \ No newline at end of file diff --git a/enterprise/app/services/captain/tools/pdf_extraction_service/blob_handler.rb b/enterprise/app/services/captain/tools/pdf_extraction_service/blob_handler.rb new file mode 100644 index 000000000..fc7f7d54e --- /dev/null +++ b/enterprise/app/services/captain/tools/pdf_extraction_service/blob_handler.rb @@ -0,0 +1,49 @@ +module Captain::Tools::PdfExtractionService::BlobHandler + private + + def active_storage_blob_url? + pdf_source.include?('/rails/active_storage/blobs/') || + (pdf_source.include?('blob') && pdf_source.include?('pdf')) + end + + def extract_from_active_storage_blob + blob = find_blob_from_url + return extract_blob_content(blob) if blob + + Rails.logger.warn 'ActiveStorage blob not found, falling back to URL download' + extract_from_url + end + + def extract_blob_content(blob) + blob.open { |file| extract_from_file(file.path) } + end + + def find_blob_from_url + blob_key = extract_blob_key_from_url + return nil if blob_key.blank? + + ActiveStorage::Blob.find_by(key: blob_key) + rescue StandardError => e + Rails.logger.error "Error finding blob with key '#{blob_key}': #{e.message}" + nil + end + + def extract_blob_key_from_url + blob_key = if pdf_source.include?('/rails/active_storage/blobs/') + extract_key_from_rails_path + elsif pdf_source.include?('/blobs/') + extract_key_from_blob_path + end + + blob_key&.split('?')&.first # Remove query parameters + end + + def extract_key_from_rails_path + parts = pdf_source.split('/rails/active_storage/blobs/').last.split('/') + parts.length > 1 && parts[0] != 'redirect' ? parts[0] : parts[1] + end + + def extract_key_from_blob_path + pdf_source.split('/blobs/').last.split('/').first + end +end \ No newline at end of file diff --git a/enterprise/app/services/captain/tools/pdf_extraction_service/error_handler.rb b/enterprise/app/services/captain/tools/pdf_extraction_service/error_handler.rb new file mode 100644 index 000000000..d85033a46 --- /dev/null +++ b/enterprise/app/services/captain/tools/pdf_extraction_service/error_handler.rb @@ -0,0 +1,44 @@ +module Captain::Tools::PdfExtractionService::ErrorHandler + private + + def extract_pdf_with_error_handling + process_pdf_extraction + rescue PDF::Reader::MalformedPDFError => e + handle_malformed_pdf_error(e) + rescue PDF::Reader::UnsupportedFeatureError => e + handle_unsupported_feature_error(e) + rescue Down::TimeoutError => e + handle_timeout_error(e) + rescue StandardError => e + handle_general_error(e) + end + + def handle_malformed_pdf_error(error) + Rails.logger.error "Malformed PDF (#{pdf_source_type}): #{error.message}" + failure_response(['Invalid or corrupted PDF format']) + end + + def handle_unsupported_feature_error(error) + Rails.logger.error "Unsupported PDF feature (#{pdf_source_type}): #{error.message}" + failure_response(['PDF contains unsupported features']) + end + + def handle_timeout_error(error) + Rails.logger.error "PDF download timeout (#{pdf_source_type}): #{error.message}" + failure_response(['PDF download timed out']) + end + + def handle_general_error(error) + Rails.logger.error "PDF extraction error (#{pdf_source_type}): #{error.message}" + Rails.logger.error error.backtrace.join("\n") + failure_response(['Failed to process PDF']) + end + + def success_response(content) + { success: true, content: content } + end + + def failure_response(errors) + { success: false, errors: errors } + end +end \ No newline at end of file diff --git a/enterprise/app/services/captain/tools/pdf_extraction_service/source_handler.rb b/enterprise/app/services/captain/tools/pdf_extraction_service/source_handler.rb new file mode 100644 index 000000000..ce1a26a5b --- /dev/null +++ b/enterprise/app/services/captain/tools/pdf_extraction_service/source_handler.rb @@ -0,0 +1,67 @@ +module Captain::Tools::PdfExtractionService::SourceHandler + private + + def determine_source_type + return :url if pdf_source.is_a?(String) && pdf_source.start_with?('http') + return :uploaded_file if pdf_source.respond_to?(:tempfile) + return :active_storage_attachment if pdf_source.is_a?(ActiveStorage::Attached::One) + + :file_path + end + + def extract_text + case determine_source_type + when :url + extract_from_url + when :uploaded_file + extract_from_uploaded_file + when :active_storage_attachment + extract_from_attachment + else + extract_from_file_path + end + end + + def extract_from_url + return extract_from_active_storage_blob if active_storage_blob_url? + + temp_file = Down.download( + pdf_source, + max_size: MAX_PDF_SIZE, + open_timeout: DOWNLOAD_TIMEOUT, + read_timeout: DOWNLOAD_TIMEOUT + ) + + begin + result = extract_from_file(temp_file.path) + ensure + temp_file.close + temp_file.unlink + end + + result + end + + def extract_from_uploaded_file + extract_from_file(pdf_source.tempfile.path) + end + + def extract_from_file_path + extract_from_file(pdf_source) + end + + def extract_from_attachment + return failure_response(['No file attached']) unless pdf_source.attached? + + pdf_source.open { |file| extract_from_file(file.path) } + end + + def pdf_source_type + case pdf_source + when String + pdf_source.start_with?('http') ? 'URL' : 'file_path' + else + 'uploaded_file' + end + end +end \ No newline at end of file diff --git a/enterprise/app/services/captain/tools/pdf_extraction_service/text_processor.rb b/enterprise/app/services/captain/tools/pdf_extraction_service/text_processor.rb new file mode 100644 index 000000000..a0c84b715 --- /dev/null +++ b/enterprise/app/services/captain/tools/pdf_extraction_service/text_processor.rb @@ -0,0 +1,46 @@ +module Captain::Tools::PdfExtractionService::TextProcessor + private + + def extract_from_file(file_path) + text_content = [] + + PDF::Reader.open(file_path) do |reader| + reader.pages.each_with_index do |page, index| + page_content = extract_page_content(page, index) + text_content << page_content if page_content + end + end + + text_content + end + + def extract_page_content(page, index) + page_text = page.text + return nil if page_text.blank? + + cleaned_text = clean_text(page_text) + return nil if cleaned_text.blank? + + { page_number: index + 1, content: cleaned_text } + rescue StandardError => e + Rails.logger.warn "Failed to extract text from page #{index + 1}: #{e.message}" + nil + end + + def clean_text(text) + # Remove form feeds and normalize whitespace + cleaned = text.tr("\f", "\n") + .gsub("\r\n", "\n") + .tr("\r", "\n") + .gsub(/\s+/, ' ') + .gsub(/\n\s*\n\s*\n+/, "\n\n") + .strip + + # Remove common PDF artifacts + cleaned = cleaned.gsub(/^\d+\s*$/, '') # Remove standalone page numbers + .gsub(/^[\s\-_=]+$/, '') # Remove separator lines + .strip + + cleaned.presence + end +end \ No newline at end of file diff --git a/enterprise/app/services/captain/tools/pdf_validation_concern.rb b/enterprise/app/services/captain/tools/pdf_validation_concern.rb index 505d47b5d..ff5eed33b 100644 --- a/enterprise/app/services/captain/tools/pdf_validation_concern.rb +++ b/enterprise/app/services/captain/tools/pdf_validation_concern.rb @@ -5,9 +5,12 @@ module Captain::Tools::PdfValidationConcern def validate_pdf_source case determine_source_type - when :url then validate_url - when :uploaded_file then validate_uploaded_file - else validate_file_path + when :url + validate_url_format + when :uploaded_file + validate_file_type_and_size + when :active_storage_attachment + validate_attachment end { success: true } @@ -16,79 +19,21 @@ module Captain::Tools::PdfValidationConcern { success: false, errors: [e.message] } end - def validate_url - uri = parse_url - validate_url_format(uri) - validate_url_length - validate_url_scheme(uri) - end - - def parse_url - URI.parse(pdf_source) + def validate_url_format + uri = URI.parse(pdf_source) + raise StandardError, 'Invalid URL scheme' unless %w[http https].include?(uri.scheme) + raise StandardError, 'URL too long' if pdf_source.length > 2000 rescue URI::InvalidURIError raise StandardError, 'Malformed URL' end - def validate_url_format(uri) - raise StandardError, 'Invalid URL format' unless uri.is_a?(URI::HTTP) || uri.is_a?(URI::HTTPS) + def validate_file_type_and_size + raise StandardError, 'Invalid file type' unless pdf_source.content_type == 'application/pdf' + raise StandardError, 'File too large' if pdf_source.size > 25.megabytes end - def validate_url_length - raise StandardError, 'URL too long' if pdf_source.length > 2048 - end - - def validate_url_scheme(uri) - raise StandardError, 'Invalid URL scheme' unless %w[http https].include?(uri.scheme) - end - - def validate_uploaded_file - validate_file_object - validate_file_size - validate_file_type - validate_file_not_empty - end - - def validate_file_object - raise StandardError, 'File object is invalid' unless pdf_source.respond_to?(:size) && pdf_source.respond_to?(:content_type) - end - - def validate_file_size - raise StandardError, "File too large (max #{self.class::MAX_PDF_SIZE / 1.megabyte}MB)" if pdf_source.size > self.class::MAX_PDF_SIZE - end - - def validate_file_type + def validate_attachment + raise StandardError, 'No file attached' unless pdf_source.attached? raise StandardError, 'Invalid file type' unless pdf_source.content_type == 'application/pdf' end - - def validate_file_not_empty - raise StandardError, 'Empty file' if pdf_source.respond_to?(:empty?) && pdf_source.empty? - end - - def validate_file_path - validate_path_presence - validate_path_existence - validate_path_file_size - validate_path_not_empty - validate_path_readable - end - - def validate_path_presence - raise StandardError, 'File path is blank' if pdf_source.blank? - end - - def validate_path_existence - raise StandardError, 'File does not exist' unless File.exist?(pdf_source) - end - - def validate_path_file_size - raise StandardError, "File too large (max #{self.class::MAX_PDF_SIZE / 1.megabyte}MB)" if File.size(pdf_source) > self.class::MAX_PDF_SIZE - end - - def validate_path_not_empty - raise StandardError, 'Empty file' if File.empty?(pdf_source) - end - - def validate_path_readable - raise StandardError, 'File is not readable' unless File.readable?(pdf_source) - end end \ No newline at end of file diff --git a/enterprise/app/views/api/v1/accounts/captain/documents/create.json.jbuilder b/enterprise/app/views/api/v1/accounts/captain/documents/create.json.jbuilder index 4ef354b5b..ee543012e 100644 --- a/enterprise/app/views/api/v1/accounts/captain/documents/create.json.jbuilder +++ b/enterprise/app/views/api/v1/accounts/captain/documents/create.json.jbuilder @@ -1,4 +1 @@ json.partial! 'api/v1/models/captain/document', formats: [:json], resource: @document - -# Include message for PDF uploads -json.message 'PDF uploaded successfully. Processing will begin shortly.' if @document.source_type == 'pdf_upload' diff --git a/enterprise/app/views/api/v1/models/captain/_document.json.jbuilder b/enterprise/app/views/api/v1/models/captain/_document.json.jbuilder index 83724b9cd..ed7bbcd4f 100644 --- a/enterprise/app/views/api/v1/models/captain/_document.json.jbuilder +++ b/enterprise/app/views/api/v1/models/captain/_document.json.jbuilder @@ -8,4 +8,15 @@ json.external_link resource.external_link json.id resource.id json.name resource.name json.status resource.status +json.source_type resource.source_type json.updated_at resource.updated_at.to_i + +# Include file information for PDF uploads +if resource.file.attached? + json.file do + json.url url_for(resource.file) + json.filename resource.file.filename + json.content_type resource.file.content_type + json.size resource.file.byte_size + end +end diff --git a/spec/enterprise/controllers/api/v1/accounts/captain/documents_controller_spec.rb b/spec/enterprise/controllers/api/v1/accounts/captain/documents_controller_spec.rb index 9a1c015de..d4cbece44 100644 --- a/spec/enterprise/controllers/api/v1/accounts/captain/documents_controller_spec.rb +++ b/spec/enterprise/controllers/api/v1/accounts/captain/documents_controller_spec.rb @@ -343,7 +343,7 @@ RSpec.describe 'Api::V1::Accounts::Captain::Documents', type: :request do expect(response).to have_http_status(:success) expect(json_response[:id]).to be_present - expect(json_response[:message]).to eq('PDF uploaded successfully. Processing will begin shortly.') + expect(json_response[:name]).to be_present end end diff --git a/spec/enterprise/jobs/captain/documents/crawl_job_spec.rb b/spec/enterprise/jobs/captain/documents/crawl_job_spec.rb index fdbe7b9b0..d3c0a9640 100644 --- a/spec/enterprise/jobs/captain/documents/crawl_job_spec.rb +++ b/spec/enterprise/jobs/captain/documents/crawl_job_spec.rb @@ -14,7 +14,15 @@ RSpec.describe Captain::Documents::CrawlJob, type: :job do before do allow(Captain::Tools::FirecrawlService).to receive(:new).and_return(firecrawl_service) allow(firecrawl_service).to receive(:perform) - create(:installation_config, name: 'CAPTAIN_FIRECRAWL_API_KEY', value: 'test-key') + + # Make sure we have the Firecrawl config properly set + config = InstallationConfig.find_or_create_by(name: 'CAPTAIN_FIRECRAWL_API_KEY') + config.update(value: 'test-key') + + # Mock simple crawl service to avoid HTTP calls if it somehow gets called + simple_crawler = instance_double(Captain::Tools::SimplePageCrawlService) + allow(Captain::Tools::SimplePageCrawlService).to receive(:new).and_return(simple_crawler) + allow(simple_crawler).to receive(:page_links).and_return([]) end context 'with account usage limits' do @@ -107,96 +115,12 @@ RSpec.describe Captain::Documents::CrawlJob, type: :job do context 'when document is a PDF' do let(:pdf_document) { create(:captain_document, external_link: 'https://example.com/document.pdf') } - let(:pdf_service) { instance_double(Captain::Tools::PdfExtractionService) } - let(:pdf_content) do - [ - { content: 'PDF page 1 content', page_number: 1, chunk_index: 1, total_chunks: 1 }, - { content: 'PDF page 2 content', page_number: 2, chunk_index: 1, total_chunks: 1 } - ] - end - before do - allow(Captain::Tools::PdfExtractionService) - .to receive(:new) - .with(pdf_document.external_link) - .and_return(pdf_service) - end + it 'delegates to PDFExtractionJob' do + expect(Captain::Documents::PdfExtractionJob) + .to receive(:perform_later) + .with(pdf_document) - it 'processes PDF using PdfExtractionService when extraction succeeds' do - allow(pdf_service).to receive(:perform).and_return({ - success: true, - content: pdf_content - }) - expect(pdf_service).to receive(:perform) - described_class.perform_now(pdf_document) - end - - it 'enqueues PdfExtractionParserJob for each content chunk when extraction succeeds' do - allow(pdf_service).to receive(:perform).and_return({ - success: true, - content: pdf_content - }) - pdf_content.each do |chunk| - expect(Captain::Tools::PdfExtractionParserJob) - .to receive(:perform_later) - .with( - assistant_id: pdf_document.assistant_id, - pdf_content: chunk, - document_id: pdf_document.id - ) - end - - described_class.perform_now(pdf_document) - end - - it 'updates document status to processing when extraction succeeds' do - allow(pdf_service).to receive(:perform).and_return({ - success: true, - content: pdf_content - }) - allow(Captain::Tools::PdfExtractionParserJob).to receive(:perform_later) - - expect(pdf_document).to receive(:update).with(status: 'processing').twice - described_class.perform_now(pdf_document) - end - - it 'updates document status to failed with error message when extraction fails' do - allow(pdf_service).to receive(:perform).and_return({ - success: false, - errors: ['Invalid PDF format'] - }) - expect(pdf_document).to receive(:update).with(status: 'processing').once - expect(pdf_document).to receive(:update).with( - status: 'failed', - error_message: 'Invalid PDF format' - ).once - - described_class.perform_now(pdf_document) - end - - it 'logs the error when extraction fails' do - allow(pdf_service).to receive(:perform).and_return({ - success: false, - errors: ['Invalid PDF format'] - }) - expect(Rails.logger).to receive(:error).with(/PDF extraction failed/) - described_class.perform_now(pdf_document) - end - - it 'handles exceptions gracefully during PDF extraction' do - allow(pdf_service).to receive(:perform).and_raise(StandardError, 'Network error') - expect(pdf_document).to receive(:update).with(status: 'processing').once - expect(pdf_document).to receive(:update).with( - status: 'failed', - error_message: 'Network error' - ).once - - described_class.perform_now(pdf_document) - end - - it 'logs exceptions during PDF extraction' do - allow(pdf_service).to receive(:perform).and_raise(StandardError, 'Network error') - expect(Rails.logger).to receive(:error).with(/PDF extraction failed/) described_class.perform_now(pdf_document) end end @@ -210,8 +134,10 @@ RSpec.describe Captain::Documents::CrawlJob, type: :job do expect(job.send(:pdf_document?, pdf_doc)).to be true end - it 'detects PDF by content type' do - pdf_doc = build(:captain_document, content_type: 'application/pdf') + it 'detects PDF by attached file' do + pdf_doc = build(:captain_document) + file_double = instance_double(ActiveStorage::Attached::One, attached?: true) + allow(pdf_doc).to receive(:file).and_return(file_double) expect(job.send(:pdf_document?, pdf_doc)).to be true end diff --git a/spec/enterprise/jobs/captain/documents/pdf_extraction_job_spec.rb b/spec/enterprise/jobs/captain/documents/pdf_extraction_job_spec.rb new file mode 100644 index 000000000..b511ee925 --- /dev/null +++ b/spec/enterprise/jobs/captain/documents/pdf_extraction_job_spec.rb @@ -0,0 +1,148 @@ +require 'rails_helper' + +RSpec.describe Captain::Documents::PdfExtractionJob, type: :job do + let(:document) { create(:captain_document, external_link: 'https://example.com/document.pdf', source_type: 'pdf_upload') } + let(:pdf_service) { instance_double(Captain::Tools::PdfExtractionService) } + let(:pdf_content) do + [ + { content: 'PDF page 1 content', page_number: 1, chunk_index: 1, total_chunks: 2 }, + { content: 'PDF page 2 content', page_number: 2, chunk_index: 2, total_chunks: 2 } + ] + end + + before do + allow(Captain::Tools::PdfExtractionService) + .to receive(:new) + .and_return(pdf_service) + end + + describe '#perform' do + context 'when document is not a PDF' do + let(:web_document) { create(:captain_document, external_link: 'https://example.com/page.html') } + + it 'returns early without processing' do + expect(pdf_service).not_to receive(:perform) + described_class.perform_now(web_document) + end + end + + context 'when document is a PDF' do + it 'updates document status to in_progress' do + allow(pdf_service).to receive(:perform).and_return({ success: true, content: pdf_content }) + allow(Captain::Tools::PdfExtractionParserJob).to receive(:perform_later) + + expect(document).to receive(:update).with(status: 'in_progress').once + expect(document).to receive(:update).with(status: 'in_progress', processed_at: nil).once + described_class.perform_now(document) + end + + it 'uses correct PDF source for extraction' do + allow(pdf_service).to receive(:perform).and_return({ success: true, content: pdf_content }) + allow(Captain::Tools::PdfExtractionParserJob).to receive(:perform_later) + + expect(Captain::Tools::PdfExtractionService) + .to receive(:new) + .with(document.external_link) + + described_class.perform_now(document) + end + + context 'when document has attached file' do + let(:attached_file) { instance_double(ActiveStorage::Attached::One, attached?: true) } + + before do + allow(document).to receive(:file).and_return(attached_file) + end + + it 'uses attached file for extraction' do + allow(pdf_service).to receive(:perform).and_return({ success: true, content: pdf_content }) + allow(Captain::Tools::PdfExtractionParserJob).to receive(:perform_later) + + expect(Captain::Tools::PdfExtractionService) + .to receive(:new) + .with(document.file) + + described_class.perform_now(document) + end + end + + context 'when extraction succeeds' do + before do + allow(pdf_service).to receive(:perform).and_return({ success: true, content: pdf_content }) + end + + it 'enqueues PdfExtractionParserJob for each content chunk' do + pdf_content.each do |chunk| + expect(Captain::Tools::PdfExtractionParserJob) + .to receive(:perform_later) + .with( + assistant_id: document.assistant_id, + pdf_content: chunk, + document_id: document.id + ) + end + + described_class.perform_now(document) + end + + it 'updates document status and processed_at' do + allow(Captain::Tools::PdfExtractionParserJob).to receive(:perform_later) + + expect(document).to receive(:update).with(status: 'in_progress').once + expect(document).to receive(:update).with(status: 'in_progress', processed_at: nil).once + + described_class.perform_now(document) + end + + it 'logs successful extraction' do + allow(Captain::Tools::PdfExtractionParserJob).to receive(:perform_later) + + expect(Rails.logger).to receive(:info).with(/PDF extraction successful/).at_least(:once) + expect(Rails.logger).to receive(:info).with(/chunks queued/).at_least(:once) + allow(Rails.logger).to receive(:info) # Allow other logging calls + + described_class.perform_now(document) + end + end + + context 'when extraction fails' do + before do + allow(pdf_service).to receive(:perform).and_return({ + success: false, + errors: ['Invalid PDF format', 'File corrupted'] + }) + end + + it 'updates document status to available' do + expect(document).to receive(:update).with(status: 'in_progress') + expect(document).to receive(:update).with(status: 'available') + + described_class.perform_now(document) + end + + it 'logs the error' do + expect(Rails.logger).to receive(:error).with(/PDF extraction failed.*Invalid PDF format, File corrupted/) + described_class.perform_now(document) + end + end + + context 'when extraction raises an exception' do + before do + allow(pdf_service).to receive(:perform).and_raise(Captain::Tools::PdfExtractionService::ExtractionError, 'Network error') + end + + it 'updates document status to available' do + expect(document).to receive(:update).with(status: 'in_progress') + expect(document).to receive(:update).with(status: 'available') + + described_class.perform_now(document) + end + + it 'logs the exception' do + expect(Rails.logger).to receive(:error).with(/PDF extraction failed.*Network error/) + described_class.perform_now(document) + end + end + end + end +end \ No newline at end of file diff --git a/spec/enterprise/jobs/captain/tools/pdf_extraction_parser_job_spec.rb b/spec/enterprise/jobs/captain/tools/pdf_extraction_parser_job_spec.rb index 3887f1084..fb4b000aa 100644 --- a/spec/enterprise/jobs/captain/tools/pdf_extraction_parser_job_spec.rb +++ b/spec/enterprise/jobs/captain/tools/pdf_extraction_parser_job_spec.rb @@ -49,7 +49,7 @@ RSpec.describe Captain::Tools::PdfExtractionParserJob, type: :job do document = Captain::Document.last expect(document.assistant_id).to eq(assistant.id) expect(document.content).to include('sample PDF content') - expect(document.name).to include('This is sample PDF content extracted from a document') + expect(document.name).to eq('PDF Content') expect(document.status).to eq('available') end @@ -60,7 +60,7 @@ RSpec.describe Captain::Tools::PdfExtractionParserJob, type: :job do ) document = Captain::Document.last - expect(document.name).to eq('This is sample PDF content extracted from a document.') + expect(document.name).to eq('PDF Content') end it 'generates appropriate title for multiple chunks' do @@ -119,64 +119,67 @@ RSpec.describe Captain::Tools::PdfExtractionParserJob, type: :job do end context 'when document_id is provided' do - let(:document) { create(:captain_document, assistant: assistant, status: 'in_progress') } + let!(:document) { create(:captain_document, assistant: assistant, status: 'in_progress') } - it 'updates document status to available' do - described_class.new.perform( - assistant_id: assistant.id, - pdf_content: pdf_content, - document_id: document.id - ) + it 'creates a new document with external link referencing document_id' do + expect do + described_class.new.perform( + assistant_id: assistant.id, + pdf_content: pdf_content, + document_id: document.id + ) + end.to change(Captain::Document, :count).by(1) - document.reload - expect(document.status).to eq('available') + new_document = Captain::Document.last + expect(new_document.external_link).to include("pdf_chunk_#{document.id}") end end context 'when an error occurs' do - let(:document) { create(:captain_document, assistant: assistant, status: 'in_progress') } + context 'when assistant is not found' do + it 'logs the error and does not raise' do + expect(Rails.logger).to receive(:error).with(/Assistant not found/) - before do - allow(Captain::Document).to receive(:create!).and_raise(StandardError, 'Database error') - # Mock the specific document instance that will be found by the job - allow(Captain::Document).to receive(:find_by).with(id: document.id).and_return(document) - allow(document).to receive(:update!).and_raise(StandardError, 'Database error') + expect do + described_class.new.perform( + assistant_id: 99_999, + pdf_content: pdf_content + ) + end.not_to change(Captain::Document, :count) + end end - it 'raises an error with descriptive message' do - expect do - described_class.new.perform( - assistant_id: assistant.id, - pdf_content: pdf_content, - document_id: document.id - ) - end.to raise_error(/Failed to parse PDF data/) + context 'when document creation fails with validation error' do + before do + allow(Captain::Document).to receive(:create!).and_raise(ActiveRecord::RecordInvalid.new(Captain::Document.new)) + end + + it 'logs the validation error and does not raise' do + expect(Rails.logger).to receive(:error).with(/Invalid document data/) + + expect do + described_class.new.perform( + assistant_id: assistant.id, + pdf_content: pdf_content + ) + end.not_to change(Captain::Document, :count) + end end - it 'raises error and updates main document if provided' do - expect do - described_class.new.perform( - assistant_id: assistant.id, - pdf_content: pdf_content, - document_id: document.id - ) - end.to raise_error(/Failed to parse PDF data/) + context 'when limit is exceeded during creation' do + before do + allow(Captain::Document).to receive(:create!).and_raise(Captain::Document::LimitExceededError.new('Limit exceeded')) + end - # The main document should remain in its original state when an error occurs - document.reload - expect(document.status).to eq('in_progress') - end + it 'logs the limit error and does not raise' do + expect(Rails.logger).to receive(:info).with(/Document limit exceeded/) - it 'logs the error' do - expect(Rails.logger).to receive(:error).with(/Failed to parse PDF content/).at_least(:once) - - begin - described_class.new.perform( - assistant_id: assistant.id, - pdf_content: pdf_content - ) - rescue StandardError - # Expected to raise error + expect do + described_class.new.perform( + assistant_id: assistant.id, + pdf_content: pdf_content + ) + end.not_to change(Captain::Document, :count) end end end @@ -185,34 +188,59 @@ RSpec.describe Captain::Tools::PdfExtractionParserJob, type: :job do describe 'private methods' do let(:job) { described_class.new } - describe '#generate_content_title' do + describe '#generate_title' do it 'uses first line as title when appropriate' do - content = "Introduction to Machine Learning\n\nThis document covers the basics..." - title = job.send(:generate_content_title, content, 1, 1, 1) + pdf_content = { + content: "Introduction to Machine Learning\n\nThis document covers the basics...", + page_number: 1, + chunk_index: 1, + total_chunks: 1 + } + title = job.send(:generate_title, pdf_content) expect(title).to eq('Introduction to Machine Learning') end it 'uses generic title for long first lines' do - content = "#{('A' * 200)}\n\nThis is the rest of the content..." - title = job.send(:generate_content_title, content, 1, 1, 1) + pdf_content = { + content: "#{('A' * 200)}\n\nThis is the rest of the content...", + page_number: 1, + chunk_index: 1, + total_chunks: 1 + } + title = job.send(:generate_title, pdf_content) expect(title).to eq('PDF Content') end it 'adds page information for multiple pages' do - content = 'Sample content' - title = job.send(:generate_content_title, content, 3, 1, 1) + pdf_content = { + content: 'Sample content', + page_number: 3, + chunk_index: 1, + total_chunks: 1 + } + title = job.send(:generate_title, pdf_content) expect(title).to eq('Sample content (Page 3)') end it 'adds chunk information for multiple chunks' do - content = 'Sample content' - title = job.send(:generate_content_title, content, 2, 2, 4) + pdf_content = { + content: 'Sample content', + page_number: 2, + chunk_index: 2, + total_chunks: 4 + } + title = job.send(:generate_title, pdf_content) expect(title).to eq('Sample content (Page 2, Part 2/4)') end it 'truncates long titles to database limit' do - long_content = 'A' * 300 - title = job.send(:generate_content_title, long_content, 1, 1, 1) + pdf_content = { + content: 'A' * 300, + page_number: 1, + chunk_index: 1, + total_chunks: 1 + } + title = job.send(:generate_title, pdf_content) expect(title.length).to be <= 255 end end From 250d9d4deed8d6081e7c6db3454df4bfdb24f1ee Mon Sep 17 00:00:00 2001 From: Tanmay Deep Sharma <32020192+tds-1@users.noreply.github.com> Date: Thu, 3 Jul 2025 13:57:38 +0530 Subject: [PATCH 2/2] refactor: review changes --- .../conversation/response_builder_job.rb | 9 ++++-- .../captain/documents/pdf_extraction_job.rb | 32 ++++++++++++------- .../tools/pdf_extraction_parser_job.rb | 14 +++++--- .../captain/tools/pdf_validation_concern.rb | 22 +++++++++++-- 4 files changed, 56 insertions(+), 21 deletions(-) diff --git a/enterprise/app/jobs/captain/conversation/response_builder_job.rb b/enterprise/app/jobs/captain/conversation/response_builder_job.rb index f341a6e98..2d618ae89 100644 --- a/enterprise/app/jobs/captain/conversation/response_builder_job.rb +++ b/enterprise/app/jobs/captain/conversation/response_builder_job.rb @@ -52,12 +52,15 @@ class Captain::Conversation::ResponseBuilderJob < ApplicationJob def message_content(message) return message.content if message.content.present? + + handle_message_without_content(message) + end + + def handle_message_without_content(message) return 'User has shared a message without content' unless message.attachments.any? audio_transcriptions = extract_audio_transcriptions(message.attachments) - return audio_transcriptions if audio_transcriptions.present? - - 'User has shared an attachment' + audio_transcriptions.presence || 'User has shared an attachment' end def extract_audio_transcriptions(attachments) diff --git a/enterprise/app/jobs/captain/documents/pdf_extraction_job.rb b/enterprise/app/jobs/captain/documents/pdf_extraction_job.rb index aef02dfb1..da7f945b7 100644 --- a/enterprise/app/jobs/captain/documents/pdf_extraction_job.rb +++ b/enterprise/app/jobs/captain/documents/pdf_extraction_job.rb @@ -4,23 +4,33 @@ class Captain::Documents::PdfExtractionJob < ApplicationJob def perform(document) return unless document.pdf_document? - document.update(status: 'in_progress') - - pdf_source = document.file.attached? ? document.file : document.external_link - pdf_extraction_service = Captain::Tools::PdfExtractionService.new(pdf_source) - result = pdf_extraction_service.perform - - if result[:success] && result[:content].present? - process_pdf_content_chunks(document, result[:content]) - else - handle_pdf_extraction_failure(document, result) - end + initialize_document_processing(document) + result = extract_pdf_content(document) + process_extraction_result(document, result) rescue Captain::Tools::PdfExtractionService::ExtractionError => e handle_pdf_extraction_error(document, e) end private + def initialize_document_processing(document) + document.update(status: 'in_progress') + end + + def extract_pdf_content(document) + pdf_source = document.file.attached? ? document.file : document.external_link + pdf_extraction_service = Captain::Tools::PdfExtractionService.new(pdf_source) + pdf_extraction_service.perform + end + + def process_extraction_result(document, result) + if result[:success] && result[:content].present? + process_pdf_content_chunks(document, result[:content]) + else + handle_pdf_extraction_failure(document, result) + end + end + def process_pdf_content_chunks(document, content_chunks) Rails.logger.info "PDF extraction successful for document #{document.id}: #{content_chunks.length} chunks will be processed" diff --git a/enterprise/app/jobs/captain/tools/pdf_extraction_parser_job.rb b/enterprise/app/jobs/captain/tools/pdf_extraction_parser_job.rb index 546e2c4ba..2815cac3c 100644 --- a/enterprise/app/jobs/captain/tools/pdf_extraction_parser_job.rb +++ b/enterprise/app/jobs/captain/tools/pdf_extraction_parser_job.rb @@ -3,12 +3,10 @@ class Captain::Tools::PdfExtractionParserJob < ApplicationJob def perform(assistant_id:, pdf_content:, document_id: nil) assistant = Captain::Assistant.find(assistant_id) - content = pdf_content[:content] - - return if content.blank? || limit_exceeded?(assistant.account) + return unless should_process_content?(pdf_content[:content], assistant.account) document = create_document(assistant, pdf_content, document_id) - Captain::Documents::ResponseBuilderJob.perform_later(document) if document + enqueue_response_builder_job(document) rescue ActiveRecord::RecordNotFound => e Rails.logger.error "PDF parser job failed - Assistant not found: #{e.message}" rescue ActiveRecord::RecordInvalid => e @@ -69,6 +67,14 @@ class Captain::Tools::PdfExtractionParserJob < ApplicationJob end end + def should_process_content?(content, account) + content.present? && !limit_exceeded?(account) + end + + def enqueue_response_builder_job(document) + Captain::Documents::ResponseBuilderJob.perform_later(document) if document + end + def limit_exceeded?(account) limits = account.usage_limits.dig(:captain, :documents) limits && limits[:current_available].to_i <= 0 diff --git a/enterprise/app/services/captain/tools/pdf_validation_concern.rb b/enterprise/app/services/captain/tools/pdf_validation_concern.rb index ff5eed33b..ff613f2c6 100644 --- a/enterprise/app/services/captain/tools/pdf_validation_concern.rb +++ b/enterprise/app/services/captain/tools/pdf_validation_concern.rb @@ -20,13 +20,29 @@ module Captain::Tools::PdfValidationConcern end def validate_url_format - uri = URI.parse(pdf_source) - raise StandardError, 'Invalid URL scheme' unless %w[http https].include?(uri.scheme) - raise StandardError, 'URL too long' if pdf_source.length > 2000 + uri = parse_and_validate_uri + validate_url_scheme(uri) + validate_url_length + end + + def parse_and_validate_uri + URI.parse(pdf_source) rescue URI::InvalidURIError raise StandardError, 'Malformed URL' end + def validate_url_scheme(uri) + return if %w[http https].include?(uri.scheme) + + raise StandardError, 'Invalid URL scheme' + end + + def validate_url_length + return if pdf_source.length <= 2000 + + raise StandardError, 'URL too long' + end + def validate_file_type_and_size raise StandardError, 'Invalid file type' unless pdf_source.content_type == 'application/pdf' raise StandardError, 'File too large' if pdf_source.size > 25.megabytes