design code suggestions?

parser_worker.rb

class ToolParserWorker
  include Sidekiq::Worker

  def perform(scan)

    ## On a near futute NessusParser will die an become a ParserFactory based on scan tool type
    report = NessusParser.execute(self.file.current_path) # execute returns a Report object

    scan.update_parsed_report(report.to_json) # scan is mongo document
  end
end

Pensei em criar uma classe pra encapsular o parser e o update do scan algo tipo.

class ToolParserWorker
  include Sidekiq::Worker

  def perform(scan)
    report = NessusParser.execute(self.file.current_path)
    scan.update_parsed_report(report.to_json)
  end
end

Assim ficou melhor? :-)

Falando sério agora...

Eu gosto da ideia de criar uma classe com uma responsabilidade bem definida (nesse caso fazer o updade). Essa classe vai orquestrar tudo (delegando parte do serviço para outras, se for necessário).

o parâmetro scan no método perform não pode ser um objeto, vai dar caquinha ao serializar no redis. o ideal é que seja algo nativo do json, como um inteiro. passe o id do objeto e recupere dentro do perform.

O que esse método update_parsed_report faz? Possivelmente ele pode ir para dentro dessa classe e sair do seu modelo.

@rtopitt bem lembrado!

O problema não é apenas a questão de dar ou não erro no Redis. Se a serialização funcionar você PODE (depende de como você fizer) ter um problema pois o seu objeto recuperado da serialização pode não ser mais o que ele deveria ser (ter sido atualizado ou até mesmo deletado).

...os comentários eram só pra dar ideia do que acontecerá em breve, o código real está sem. :)

@rtopitt e @tapajos entendi a questão de passar o objeto como paramentro, vou passar o id mesmo, como no DJ, que depois eu recupero o objeto por inteiro com o diretório do arquivo a ser parseado. A sugestão do Tapa de colocar em uma classe responsável, era o que estava pensando, porém fiquei na dúvida se haveria alguma ideia melhor. pensei em algo como:

class ToolParserWorker
  include Sidekiq::Worker

  def perform(scan_id)
    UpdateScanParser.execute_for(scan_id)
  end
end

class UpdateScanParser
  def execute_for(scan_id)
    scan = Scan.find(id: scan_id)
    report = NessusParser.execute(scan.file.current_path)
    scan.update_parsed_report(report.to_json)
  end
end

Ainda sim tenho a sensação que não ta legal. :|

não melhora muito, mas eu faria assim:

class ToolParserWorker
  include Sidekiq::Worker
  def perform(scan_id)
    UpdateScanParser.new(scan_id).execute
  end
end

class UpdateScanParser
  def initialize(scan_id)
    @scan = Scan.find(scan_id)
    @report = NessusParser.execute(@scan.file.current_path)
  end
  def execute
    @scan.update_parsed_report(@report)
  end
end

Novamente, o que é update_parsed_report? Não faz sentido ficar nessa classe?

Eu não gosto da ideia de fazer ações no método de inicialização da classe. Eu acho que acaba violando o principio da Least Surprise. Não fica claro pros clientes dessa classe que você vai fazer um lookup no banco pra inicializar um objeto, tampouco fazer o scan de um json.

De acordo com isso eu faria algo assim:

class ToolParserWorker
  include Sidekiq::Worker

  def perform(scan_id)
    UpdateScanParser.new(scan_id).parse # talvez dê melhor a ideia do que o método faz
  end
end

class UpdateScanParser

  def initialize(scan_id, parser=NessusParser.new)
    @scan_id = scan_id
    @parser  = parser
  end 

  def parse
    scan = Scan.find(id: scan_id)
    @parser.execute(scan)
  end
end

class NessusParser
  def parse(scan)
     report = execute(scan.file_path) # esse método faria um file.current_path, mas não exporia o detalhe de usar o file internamente
     scan.update_parsed_report(report.to_json)
  end 
  private
  def execute(path)
     # ....
     return report
  end 
end 

Sobre criar uma instancia de um NessusParser dentro do initialize, eu faria isso pois seria fácil trocar a implementação de parser caso necessário e/ou nos testes. Também, ficaria independente de uma implementação específica. Por mais que use a NessusParser como default, você pode especificar outros parsers em outras situações.

No scan, eu não exporia que internamente o scan utiliza um file, pra não violar o encapsulamento, faria então esse @scan.file_path ou algo do gênero.

Curti a alternativa do @rtopitt, pois é mais minimalista.

Eu faria assim:

class ToolParserWorker
  include Sidekiq::Worker
  def perform(scan_id)
    UpdateScanParser.new(scan_id).execute
  end
end

class UpdateScanParser
  def initialize(scan_id)
    @scan_id = scan_id
  end
  def execute
    scan.update_parsed_report(report.to_json)
  end
  private
    def report
      NessusParser.execute(scan.file.current_path)
    end
    def scan
      @scan ||= Scan.find(@scan_id)
    end
end

Acho que a ideia do pellegrino é a mais legal. A.classe de UpdateScanParser deve receber um objeto de parser o que vai deixar ela bem generica para fazer.a.persistencia de.qualquer tipo de parser. So nao achei legal inicializar o objeto de parser na inicialização. Acho que é melhor dar um tratamento com uma exception se nao for o objeto, do tipo, ou exatamente o que.vc.espera.

Gostei da sugestão do @pellegrino, apenas uma correção em UpdateScanParser#parse:

class ToolParserWorker
  include Sidekiq::Worker

  def perform(scan_id)
    UpdateScanParser.new(scan_id).parse
  end
end

class UpdateScanParser

  def initialize(scan_id, parser=NessusParser.new)
    @scan_id = scan_id
    @parser  = parser
  end 

  def parse
    scan = Scan.find(id: scan_id)
    @parser.parse(scan) # aqui é parse, e não execute
  end
end

class NessusParser
  def parse(scan)
     report = execute(scan.file_path)
     scan.update_parsed_report(report.to_json)
  end 
  private
  def execute(path)
     # ....
     return report
  end 
end

O que me agrada nessa solução é que as responsabilidades estão bem definidas em cada uma das classes. A observação dele quanto a scan.file_path vs. scan.file.current_path também é bem legal, pois obedece a Lei de Demeter ("Only talk to your immediate friends.").

O problema que existe nessa solução é que de qualquer forma o Model ficará acoplando com o Parser, que é justamente o que estou querendo evitar. O NessusParser é só um, de um conjunto de parsers de xml, que eu não queria que tivessem dependências com a regra da aplicação, ou seja, só faz parsear o xml e retorna um objeto report. O que motivou a criar os parsers próprios e não utilizar os existentes, foi o os módules de parser terem dependência com ORM.

Dado esse cenário, utilizando a sugestão do @pellegrino, o que pensei:

class ToolParserWorker
  include Sidekiq::Worker

  def perform(scan_id)
    UpdateScanParser.new(scan_id).parse
  end
end

class UpdateScanParser
  def initialize(scan_id)
    @scan_id = scan_id
  end

  def parse
    report = parser.parse(scan)
    scan.update_parsed_report(report.to_json)
  end

  private
  def scan
    scan ||= Scan.find(id: @scan_id)
  end

  def parser
    parser ||= ParserFactory.instance(scan.tool)
  end
end

class NessusParser
  def execute(path)
     # ....
     return report
  end
end

class Scan
  #...
  def update_parsed_report(json)
    self.parsed_document = json
    sef.save!
  end
end

Dessa forma, fica obscuro para que está chamando UpdateScanParser que ele chama uma fábrica de Parser internamente, o que me incomoda é que método UpdateScanParser#parse, parece desrespeitar o SRP, mas ao mesmo tempo não acho que deveria criar mais uma classe pra tratar a o update externamente.

@tapajos coloquei o que o update_parsed_report faz ;)

Não tinha entendido que haveriam vários parsers, neste caso fico com a solução do @pellegrino, reescrita pelo @caike