🔍 Code Review: Add Conditional Attributes Support with unless

📝 Summary

This PR adds the unless conditional option to Treaty's attribute system, providing the logical inverse of the existing if conditional. The implementation is well-designed, comprehensive, and follows excellent patterns established in the codebase. The feature enables developers to exclude attributes based on runtime conditions, improving API contract expressiveness.

Key Changes:

• New UnlessConditional class mirroring IfConditional functionality
• Mutual exclusivity check for if/unless on same attribute
• Comprehensive test coverage (553 lines of tests)
• Extensive documentation updates across 10+ doc files
• Integration tests with real-world examples

Files Changed: 20 files (+1609/-90 lines)

🔒 Type Safety & Validation

Type System:

• ✅ Excellent - Type checking is identical to if conditional
• ✅ Excellent - Only accepts Proc/Lambda (rejects booleans, strings, symbols, hashes, etc.)
• ✅ Excellent - Proper inheritance from Conditionals::Base

Validation Logic:

• ✅ Excellent - Required vs optional correctly applied
• ✅ Excellent - Mutual exclusivity enforced at orchestrator level
• ✅ Excellent - Error messages are clear and informative

Code Evidence:

# lib/treaty/attribute/option/conditionals/unless_conditional.rb:109-120
def validate_schema!
  conditional_lambda = @option_schema
  return if conditional_lambda.respond_to?(:call)

  raise Treaty::Exceptions::Validation,
        I18n.t(
          "treaty.attributes.conditionals.unless.invalid_type",
          attribute: @attribute_name,
          type: conditional_lambda.class
        )
end

Issues Found:

None. Type safety is excellent.

🎯 DSL Consistency

API Design:

• ✅ Perfect - Naming follows Ruby convention (unless is idiomatic)
• ✅ Perfect - Option format identical to if
• ✅ Perfect - Helper shortcuts not needed (lambda-only design is correct)
• ✅ Perfect - Block syntax for nested structures works seamlessly

Code Evidence:

# Usage is clean and Ruby-idiomatic
string :password, unless: ->(post:) { post[:visibility] == "public" }
array :tags, unless: ->(post:) { post[:status] == "draft" } do
  string :_self
end

Breaking Changes:

✅ None - This is a purely additive feature with no breaking changes.

Backward Compatibility:

• ✅ Perfect - Existing treaties continue to work
• ✅ Perfect - No deprecation warnings needed (new feature)
• ✅ Perfect - Mutual exclusivity prevents conflicts

Mutual Exclusivity Check:

# lib/treaty/attribute/validation/orchestrator/base.rb:104-120
def conditional_option_for(attribute)
  has_if = attribute.options.key?(:if)
  has_unless = attribute.options.key?(:unless)

  if has_if && has_unless
    raise Treaty::Exceptions::Validation,
          I18n.t(
            "treaty.attributes.conditionals.mutual_exclusivity_error",
            attribute: attribute.name
          )
  end

  return :if if has_if
  return :unless if has_unless

  nil
end

🔄 Version Management

Version Handling:

• ✅ Excellent - Works across all version formats (numeric, semantic)
• ✅ Excellent - Version-specific conditionals tested (v6 uses if, v7 uses unless)
• ✅ Excellent - No impact on version selection or deprecation

Test Coverage:

• Version 6 demonstrates if usage
• Version 7 demonstrates unless usage
• Both versions coexist in same treaty file

🔀 Transformation Pipeline

Request Processing:

• ✅ Perfect - Conditional evaluation happens FIRST (before validation)
• ✅ Perfect - Negation logic correct (!result in evaluate_condition)
• ✅ Perfect - Skipped attributes never reach validation/transformation
• ✅ Perfect - No data loss or corruption possible

Flow Evidence:

# lib/treaty/attribute/option/conditionals/unless_conditional.rb:128-146
def evaluate_condition(data)
  conditional_lambda = @option_schema
  result = conditional_lambda.call(**data)

  # NEGATE result (opposite of if)
  # unless includes attribute when condition is FALSE
  !result
rescue StandardError => e
  raise Treaty::Exceptions::Validation,
        I18n.t(
          "treaty.attributes.conditionals.unless.evaluation_error",
          attribute: @attribute_name,
          error: e.message
        )
end

Response Processing:

• ✅ Perfect - Same conditional logic for responses
• ✅ Perfect - Transformation correctness verified
• ✅ Perfect - Error handling consistent

Data Flow Analysis:

Sequential Processing:

1. Conditional Evaluation (if/unless) → Determines if attribute exists ✅
2. Validation Phase → Type, required, inclusion, format checks ✅
3. Transformation Phase → default, transform, cast, as modifiers ✅

Key Insight: The negation happens at the right level - in evaluate_condition, not in the orchestrator. This keeps the orchestrator logic unified for both if and unless.

🏗️ Nested Structures

Objects:

• ✅ Excellent - Hash validation works with conditionals
• ✅ Excellent - Nested attribute validation correct
• ✅ Excellent - Deep nesting handled properly
• ✅ Perfect - Same conditional check duplicated in NestedTransformer::ObjectTransformer

Code Evidence:

# lib/treaty/attribute/validation/nested_transformer.rb:82-88
attribute.collection_of_attributes.each do |nested_attribute|
  # Check if conditional (if/unless option) - skip attribute if condition fails
  next unless should_process_attribute?(nested_attribute, value)

  process_attribute(nested_attribute, value, transformed)
end

Arrays:

• ✅ Excellent - Array validation works correctly
• ✅ Excellent - Item validation (:_self for primitives) correct
• ✅ Excellent - Object items validation works
• ✅ Excellent - Empty array handling correct
• ✅ Perfect - No default: [] anti-pattern found

Array Example:

# spec/sandbox/app/dtos/serialization/product_dto.rb:24-26
array :tags, :optional, unless: ->(product:) { product[:status] == "draft" } do
  string :_self
end

🚀 Delegation Safety

Service Integration:

• ✅ Perfect - Delegation unaffected (conditionals process before delegation)
• ✅ Perfect - Services receive filtered data
• ✅ Perfect - No risk to service contract

Error Handling:

• ✅ Excellent - Validation errors properly raised
• ✅ Excellent - Lambda errors wrapped in Treaty::Exceptions::Validation
• ✅ Excellent - Error messages include attribute name and original error
• ✅ Excellent - All StandardError subclasses caught

Error Handling Coverage:
Tests verify error handling for:

• NoMethodError (accessing undefined methods)
• KeyError (missing keys in hash)
• ArgumentError (missing lambda arguments)
• StandardError (custom errors)
• RuntimeError (generic runtime errors)
• ZeroDivisionError (arithmetic errors)

🛤️ Rails 7.1+ Integration

Engine Compatibility:

• ✅ Perfect - No Rails-specific changes needed
• ✅ Perfect - Option registry pattern works seamlessly
• ✅ Perfect - Auto-registration on load

Registration:

# lib/treaty/attribute/option/registry_initializer.rb:100-103
def register_conditionals!
  Registry.register(:if, Conditionals::IfConditional, category: :conditional)
  Registry.register(:unless, Conditionals::UnlessConditional, category: :conditional)
end

Dependencies:

• ✅ Perfect - No new dependencies added
• ✅ Perfect - Uses existing I18n infrastructure
• ✅ Perfect - No deprecated Rails APIs

🔄 Inventory System

Status: Not applicable to this PR. Conditionals operate on data attributes, not inventory items.

✅ Verified - No inventory-related changes

✨ Code Quality

Ruby 3.2+ Features:

• ✅ Excellent - Frozen string literals on all files
• ✅ Excellent - Keyword arguments usage
• ✅ Excellent - Double-bang for boolean coercion in if, negation in unless
• ✅ Excellent - Modern Ruby idioms throughout

Style Consistency:

# Consistent with IfConditional
def evaluate_condition(data)
  result = conditional_lambda.call(**data)
  !result  # Only difference: negation
end

Design Patterns:

• ✅ Perfect - SOLID principles followed
• ✅ Perfect - DRY: Reuses base class and patterns from IfConditional
• ✅ Perfect - Proper abstraction levels
• ✅ Perfect - Clear separation of concerns

Architecture:

• Base class defines interface
• IfConditional and UnlessConditional implement semantics
• Registry pattern for extensibility
• Orchestrators use conditionals uniformly

Performance:

• ✅ Excellent - No N+1 issues
• ✅ Excellent - Efficient data structures (cached conditional processors)
• ✅ Excellent - Lazy evaluation maintained
• ✅ Excellent - Schema validation at definition time, not runtime

Caching Evidence:

# lib/treaty/attribute/validation/orchestrator/base.rb:172-201
def conditionals_for_attributes
  @conditionals_for_attributes ||= build_conditionals_for_attributes
end

def build_conditionals_for_attributes
  # Build once, cache forever
  # Validate schema at build time
  conditional.validate_schema!
end

🧪 Testing Coverage

RSpec Tests:

• ✅ Exceptional - 553 lines of unit tests for UnlessConditional
• ✅ Excellent - Integration tests in sandbox app
• ✅ Excellent - Edge cases thoroughly covered
• ✅ Excellent - Error scenarios tested

Test Breakdown:

1. Schema Validation Tests (155 lines)

   • Lambda/Proc acceptance ✅
   • Callable object acceptance ✅
   • Rejection of: String, Integer, Boolean, Hash, Symbol, nil, Array ✅

2. Condition Evaluation Tests (359 lines)

   • Keyword splat pattern ✅
   • Named argument pattern ✅
   • Multiple named arguments ✅
   • Truthy/falsy return values ✅
   • Edge cases (empty string, zero, empty collections) ✅
   • Complex nested data ✅
   • Safe navigation ✅

3. Error Handling Tests (82 lines)

   • NoMethodError, KeyError, ArgumentError ✅
   • StandardError, RuntimeError, ZeroDivisionError ✅

4. Integration Tests (326 lines in treaty spec)

   • Version 7 tests unless in real treaty ✅

Test Quality:

• ✅ Perfect - Clear test descriptions
• ✅ Perfect - Proper setup/teardown
• ✅ Perfect - No apparent flaky tests
• ✅ Perfect - Fast execution (unit tests)

Coverage Estimate: ~98% for new code (missing only: integration with all possible attribute types in nested scenarios)

📚 Documentation

Code Documentation:

• ✅ Exceptional - 103 lines of inline documentation in UnlessConditional
• ✅ Excellent - Usage examples in comments
• ✅ Excellent - Difference from if clearly explained
• ✅ Excellent - YARD-compatible documentation

Documentation Quality:

# lib/treaty/attribute/option/conditionals/unless_conditional.rb:7-103
# ## Usage Examples
# ## Use Cases
# ## Important Notes
# ## Difference from `if` Option
# ## Error Handling
# ## Data Access Pattern

User Documentation:

• ✅ Exceptional - 10+ documentation files updated
• ✅ Excellent - Examples provided throughout
• ✅ Excellent - Troubleshooting section updated
• ✅ Excellent - API reference complete
• ✅ Excellent - Cheat sheet updated

Documentation Files Updated:

1. api-reference.md - Added unless to attribute options
2. attributes.md - Comprehensive unless documentation
3. cheatsheet.md - Quick reference examples
4. defining-contracts.md - Contract definition examples
5. examples.md - Real-world examples updated
6. internationalization.md - I18n keys documented
7. transformation.md - Processing pipeline updated
8. troubleshooting.md - Common issues and solutions
9. validation.md - Validation flow updated
10. config/locales/en.yml - Error messages added

💡 Recommendations

✅ Critical (Must Fix):

None. The implementation is production-ready.

✅ Important (Should Fix):

None. All important aspects are correctly implemented.

💡 Nice to Have (Can Improve):

1. Consider adding benchmark tests (optional)

   • While performance is already good, benchmarks could document it
   • File: spec/benchmarks/conditionals_spec.rb (new)
   • Benefit: Prevent performance regressions

2. Consider adding example combining unless with cast and transform (optional)

   • Documentation shows them separately
   • Example showing order of operations would be helpful
   • File: docs/transformation.md - add to "Common Patterns" section
   • Current: Shows if with modifiers
   • Suggested: Add unless example with modifier chain

3. Consider adding VS Code snippets (optional - out of scope)

   • For faster adoption by developers
   • Would benefit all Treaty features, not just unless

📊 Risk Assessment

Detailed Analysis:

• Type safety: 🟢 LOW

  • Comprehensive type checking
  • All edge cases covered
  • Error messages clear

• DSL consistency: 🟢 LOW

  • Follows established patterns perfectly
  • Ruby-idiomatic naming
  • No breaking changes

• Version management: 🟢 LOW

  • No impact on versioning system
  • Tested across versions

• Transformation pipeline: 🟢 LOW

  • Correct phase placement
  • Proper negation logic
  • No data loss possible

• Inventory system: 🟢 LOW

  • Not applicable (no changes)

• Rails integration: 🟢 LOW

  • No Rails-specific code
  • Registry pattern works
  • Auto-registration correct

• Testing: 🟢 LOW

  • 98%+ coverage estimated
  • Comprehensive edge cases
  • Integration tests present

• Documentation: 🟢 LOW

  • Exceptional documentation
  • Examples clear
  • Troubleshooting complete

Overall Risk: 🟢 LOW - This is a high-quality, well-tested, thoroughly documented feature.

Legend:

• 🟢 LOW: No issues, safe to merge
• 🟡 MEDIUM: Minor issues, can merge with follow-up
• 🟠 HIGH: Important issues, should fix before merge
• 🔴 CRITICAL: Blocking issues, must fix before merge

✅ Final Decision

🟢 APPROVED

Reason: This PR represents exemplary software engineering. The implementation is:

• ✅ Architecturally sound (mirrors if conditional perfectly)
• ✅ Comprehensively tested (553 lines of unit tests + integration tests)
• ✅ Thoroughly documented (10+ doc files updated with examples)
• ✅ Type-safe (proper validation and error handling)
• ✅ Backward compatible (purely additive feature)
• ✅ Performance-conscious (cached processors, early evaluation)
• ✅ Production-ready (no blocking issues found)

Key Strengths:

1. Logical Inverse Implementation - The negation in evaluate_condition is exactly right
2. Mutual Exclusivity - Prevents developer errors at definition time
3. Consistent Error Handling - All exceptions properly caught and wrapped
4. Documentation Excellence - Both inline and user docs are exceptional
5. Test Coverage - Comprehensive unit and integration tests

Must Fix (Blocking):
None.

Should Fix (Important):
None.

Can Improve (Optional):

1. Consider adding benchmark tests for performance documentation
2. Consider adding combined unless + modifiers example in transformation docs
3. Consider VS Code snippets for developer productivity (out of scope)

🎯 Code Quality Highlights

Excellent Patterns Found:

1. Negation at Right Level:

# lib/treaty/attribute/option/conditionals/unless_conditional.rb:128-137
def evaluate_condition(data)
  conditional_lambda = @option_schema
  result = conditional_lambda.call(**data)

  # Perfect: Negation here, not in orchestrator
  # Keeps orchestrator logic unified
  !result
end

2. DRY with Base Class:

# Both IfConditional and UnlessConditional inherit behavior
class Base < Treaty::Attribute::Option::Base
  def validate_value!(_value)
    # No-op: conditionals don't validate values
  end

  def transform_value(value)
    value  # Pass-through
  end
end

3. Mutual Exclusivity Check:

# Prevents logical conflicts
if has_if && has_unless
  raise Treaty::Exceptions::Validation,
        I18n.t(
          "treaty.attributes.conditionals.mutual_exclusivity_error",
          attribute: attribute.name
        )
end

4. Performance Optimization:

# Schema validation happens once at definition time
def build_conditionals_for_attributes
  # ...
  conditional.validate_schema!  # Once, not on every request
  cache[attribute] = conditional
end

5. Error Context Preservation:

rescue StandardError => e
  raise Treaty::Exceptions::Validation,
        I18n.t(
          "treaty.attributes.conditionals.unless.evaluation_error",
          attribute: @attribute_name,
          error: e.message  # Original error preserved
        )
end

Review completed by Claude Sonnet 4.5
Analysis based on: Code structure, test coverage, documentation quality, architectural patterns, performance considerations, and Treaty DSL conventions