Anti-patterns Guide

[gugahoa]

Related to: #1253

Full disclaimer: I used AI to write the first draft. For the example I either pulled directly from my work's codebase and anonymized it, got from Oban's doc or let the AI cook up an example.

I added one more section than from the issue, as it's related to another thing I noticed from my team: not differentiating between discard and cancel returns.

[gugahoa]

pretty sure this is hallucination as the workflow is never inserted into the database; will have to double check

[gugahoa]

This sparked some discussion of "where is the line drawn at?" in my team.

I myself don't quite have a good rule of thumb in this instance, it's kinda of "how much does the entity matter?" question. What do you think?

[gugahoa]

Example did not use recorded: true or fetch_recorded/2

Suggested change

	```elixir
	defmodule MyApp.Workers.ExternalEntityImporter do
	use Oban.Pro.Workers.Workflow, queue: :imports
	@impl true
	def process(%Job{args: %{"external_id" => external_id}}) do
	with {:ok, response} <- MyApp.ExternalAPI.fetch_entity(external_id),
	# Store the entity in a dedicated database table
	{:ok, entity} <- MyApp.Entities.create_entity(%{
	external_id: external_id,
	name: response.name,
	details: response.details
	}) do
	# Pass only the reference ID in the job result
	{:ok, %{entity_id: entity.id}}
	end
	end
	end
	defmodule MyApp.Workers.ExternalEntityProcessor do
	use Oban.Pro.Workers.Workflow, queue: :processing
	@impl true
	def process(%Job{args: %{"entity_id" => entity_id}}) do
	# Retrieve data from a proper database table
	with {:ok, entity} <- MyApp.Entities.get_entity(entity_id) do
	MyApp.Processor.process_entity(entity)
	end
	end
	end
	```
	```elixir
	defmodule MyApp.Workers.ExternalEntityImporter do
	use Oban.Pro.Workers.Workflow, recorded: true, queue: :imports
	@impl true
	def process(%Job{args: %{"external_id" => external_id}}) do
	with {:ok, response} <- MyApp.ExternalAPI.fetch_entity(external_id),
	# Store the entity in a dedicated database table
	{:ok, entity} <- MyApp.Entities.create_entity(%{
	external_id: external_id,
	name: response.name,
	details: response.details
	}) do
	# Pass only the reference ID in the job result
	{:ok, %{entity_id: entity.id}}
	end
	end
	end
	defmodule MyApp.Workers.ExternalEntityProcessor do
	use Oban.Pro.Workers.Workflow, queue: :processing
	@impl true
	def process(%Job{args: %{"entity_id" => entity_id}} = job) do
	# Retrieve data from a proper database table
	with {:ok, prev_results} <- fetch_recorded(job, "import_entity"),
	{:ok, entity} <- MyApp.Entities.get_entity(prev_result["entity_id"]) do
	# Use entity data from the database, but workflow coordination data from recorded
	MyApp.Processor.process_entity(entity)
	end
	end
	# Helper to fetch recorded results from upstream workflow jobs
	defp fetch_recorded(job, job_name) do
	case Oban.Pro.Workers.Workflow.fetch_recorded(job, names: [job_name]) do
	{:ok, result} -> {:ok, result}
	{:error, _} -> {:error, :missing_workflow_data}
	end
	end
	end

[gugahoa]

Or Workflow

[gugahoa]

Hallucination, will have to update with the proper values (:ignore, :hold)

[gugahoa]

Since the example is weak, the refactor also seems weak

[sorentwo]

Using a job result temporarily is fine, which is why there's a recorded option in the first place. If the result is important in the long term then it should be written to a proper database entity, and certinly not a JobResult.

[gugahoa]

This is a new one, not in the issue. It arose from a discussion within my team where it was not clear the distinction on when to use {:discard, any()} and when to use {:cancel, any()}

My take on this is that :discard should be used either when attempts are exhausted or we faced a unrecoverable error. And :cancel should be used when although not successful, it's not an error – kinda like a no-op return

[sorentwo]

Noted on Slack:

Regarding the cancel vs discard discussion, the discard tuple is deprecated and undocumented now. Discard should always be because of exhausted retries, and cancel is always a manual action.

[gugahoa]

I think this is a bad example on :snooze. I use it when some resource the job needs is not ready yet, but it should be.

[gugahoa]

Although that may lead to another anti pattern? Infinitely snoozing jobs

[sorentwo]

It's better to use the backoff provided by Oban itself, either through the worker (backoff/1) or the Oban.Backoff module. This would lead to infinitely snoozing jobs, which is bad. Also, I wouldn't include arbitrary logger warnings in example code.

[gugahoa]

Maybe a good example of when to use cancel? As the job became a no-op

[sorentwo]

Definitely use {:cancel, :user_not_found} rather than {:discard, _}. And don't include logging.

[gugahoa]

This whole section was the AI being too eager, I left it here in case you find it valuable. Though it should be in another guide and not in the anti-patterns one