Antipattern Prevention

Antipattern Prevention

When writing or reviewing code in Doorkeeper, use this skill to avoid common antipatterns that degrade maintainability and performance.

1. Ruby Iteration Instead of SQL

Severity: High

# BAD — loads all tokens into memory
AccessToken.all.select { |t| t.expired? }

# GOOD — push to database
# The codebase uses expiration_time_sql for this:
# lib/doorkeeper/models/concerns/expiration_time_sql_math.rb

# BAD
ids = AccessToken.pluck(:id).select { |id| id > 100 }

# GOOD
AccessToken.where("id > ?", 100).pluck(:id)

2. Fire and Forget (Missing Error Handling)

Severity: High

# BAD
def fetch_jwks(uri)
  response = Net::HTTP.get(URI(uri))
  JSON.parse(response)
rescue
  nil
end

# GOOD — use the existing HttpFetcher which handles timeouts and errors
# Reference: lib/doorkeeper/http_fetcher.rb
def fetch_jwks(uri)
  response = http_fetcher.fetch(uri)
  JSON.parse(response)
rescue HttpFetcher::FetchError => e
  Rails.logger.warn("JWKS fetch failed: #{e.message}")
  nil
end

3. Inaudible Failures (Silent Save)

Severity: Medium

# BAD — fails silently
token.save

# GOOD — raises on failure
token.save!

# ALSO GOOD — check return value
unless token.save
  handle_error(token.errors)
end

4. Callback Complexity

Severity: High

# BAD — hidden side effects
class AccessToken
  after_create :notify_admin, :update_metrics, :send_webhook
end

# GOOD — explicit orchestration in request objects
class AuthorizationCodeRequest
  def before_successful_response
    find_or_create_access_token(...)
    super
  end
end

Doorkeeper uses before_successful_response / after_successful_response hooks — this is the correct pattern.

5. Bare Rescue

Severity: High

# BAD — catches Exception, including SystemExit, Interrupt, NoMemoryError
rescue Exception => e
  nil
end

# BAD — swallows all StandardError subclasses indiscriminately
rescue
  nil
end

# GOOD — specific exception classes
rescue JWT::DecodeError, JWT::ExpiredSignature => e
  handle_jwt_error(e)
end

6. String Interpolation in SQL

Severity: Critical (security)

# BAD — SQL injection
where("token = '#{params[:token]}'")

# GOOD — parameterized
where(token: params[:token])
where("token = ?", params[:token])

7. String Equality on Secrets

Severity: Critical (security)

# BAD — timing attack
token == stored_token

# GOOD — constant-time
ActiveSupport::SecurityUtils.secure_compare(token, stored_token)

8. Tight Coupling to ActiveRecord

Severity: Medium

Doorkeeper supports multiple ORMs. Protocol logic in lib/doorkeeper/oauth/ should use the model mixin interface:

# BAD — AR-specific in protocol code
AccessToken.where(token: value).lock.first

# GOOD — use mixin method
AccessToken.by_token(value)

9. Shotgun Surgery

Severity: Medium

If adding a new token attribute requires editing 8+ files, consider whether the design is right. The custom_attributes pattern shows how to add token attributes generically without shotgun surgery.

10. Monolithic Methods

Severity: Medium

# BAD — one method doing too much
def authorize
  validate_client
  validate_scopes
  validate_redirect_uri
  create_grant
  generate_response
end

# GOOD — Doorkeeper's validation DSL
validate :client,        error: Errors::InvalidClient
validate :redirect_uri,  error: Errors::InvalidRedirectUri
validate :scopes,        error: Errors::InvalidScope

Quick Detection Patterns

# Bare rescue
grep -rn "rescue$" lib/ app/

# Silent save
grep -rn "\.save$" lib/ app/

# SQL interpolation
grep -rn 'where(".*#\{' lib/ app/

# String equality on secrets
grep -rn '== .*token\|== .*secret\|token.* ==' lib/ app/

# Ruby filtering instead of SQL
grep -rn '\.all\.select\|\.all\.map\|\.all\.each' lib/ app/

Verification

After changes:

1. bundle exec rubocop — catches many antipatterns automatically
2. bundle exec rspec — ensures behavior hasn't regressed
3. Manual review of the diff for the patterns above