@logger.catch decorator causes silent failures and get_one_or_create() filters on all kwargs

[fsecada01]

Issue Report for sqlmodel-crud-utilities

For submission to: https://github.com/jonra1993/sqlmodel-crud-utilities/issues

---

Title

@logger.catch decorator causes silent failures and get_one_or_create() filters on all kwargs

Environment

• Python: 3.13.1
• SQLModel: 0.0.22
• SQLAlchemy: 2.0.36
• sqlmodel-crud-utilities: 0.1.0
• Database: PostgreSQL 14+

Summary

We encountered three critical issues while integrating sqlmodel-crud-utilities into a production multi-tenant CMS application:

1. Silent exception swallowing — The @logger.catch decorator suppresses all exceptions and returns None, making debugging extremely difficult
2. Incorrect filtering behavior — get_one_or_create() filters on ALL provided kwargs instead of just match fields, causing unique constraint violations
3. Unexpected return values — Functions return None on error instead of raising exceptions, breaking error handling patterns

These issues make the library unsafe for production use without significant modifications.

---

Issue 1: @logger.catch Silently Swallows Exceptions

Problem

The @logger.catch decorator is applied to functions like get_one_or_create(), which causes ALL exceptions (including programming errors, database constraint violations, etc.) to be caught silently and None returned instead.

Example Code

from sqlmodel_crud_utilities import get_one_or_create
from sqlalchemy.ext.asyncio import AsyncSession
from models import Tenant

async def create_tenant(session: AsyncSession):
    # If this fails (e.g., unique constraint violation), it returns None silently
    tenant, existed = await get_one_or_create(
        session,
        Tenant,
        slug="my-tenant",
        name="My Tenant"
    )
    # TypeError: cannot unpack non-iterable NoneType object
    # Instead of seeing the actual database error!

Expected Behavior

Exceptions should propagate to the caller so they can be handled appropriately. The @logger.catch decorator should either:

1. Be removed entirely (preferred for a library)
2. Only catch and log exceptions, then re-raise them
3. Be made optional via configuration

Actual Behavior

All exceptions are caught, logged, and None is returned. This makes it impossible to:

• Debug actual errors (you only see a cryptic "cannot unpack NoneType" error)
• Implement proper error recovery
• Distinguish between "record exists" and "database error"

Impact

Critical for production use. Silent failures can lead to data corruption, lost operations, and extremely difficult debugging sessions.

---

Issue 2: get_one_or_create() Filters on ALL kwargs

Problem

The get_one_or_create() function uses ALL provided kwargs as filter conditions, not just the unique identifier fields. This causes two major issues:

1. Unique constraint violations when creating new records
2. Unexpected behavior when you want to match on ID but update other fields

Example Code

# Trying to match by slug, but create with additional fields
tenant, existed = await get_one_or_create(
    session,
    Tenant,
    slug="my-tenant",          # Match field
    name="My Tenant Name",      # Should be for creation only
    primary_color="#ff0000",    # Should be for creation only
)

# What happens:
# SELECT * FROM tenants WHERE slug = 'my-tenant'
#   AND name = 'My Tenant Name'
#   AND primary_color = '#ff0000'
#
# This will NOT find existing tenant with different name/color,
# so it tries to INSERT with slug='my-tenant' again -> UNIQUE VIOLATION

Expected Behavior

The function should accept a separate parameter for "match fields" vs "create fields":

tenant, existed = await get_one_or_create(
    session,
    Tenant,
    match_fields={"slug": "my-tenant"},  # Only filter on these
    create_fields={                      # Use these for INSERT if not found
        "name": "My Tenant Name",
        "primary_color": "#ff0000"
    }
)

Or at minimum, support a match_fields parameter:

tenant, existed = await get_one_or_create(
    session,
    Tenant,
    slug="my-tenant",
    name="My Tenant Name",
    primary_color="#ff0000",
    match_fields=["slug"]  # Only filter on slug field
)

Actual Behavior

All kwargs are used in the WHERE clause, making it impossible to create records with all fields populated without risking unique constraint violations.

Impact

High. This makes the function unusable for any model with unique constraints beyond the primary key. You must either:

• Only pass the unique field (losing the convenience of the function)
• Risk unique constraint violations
• Not use the function at all

---

Issue 3: Inconsistent Return Types and Error Handling

Problem

When an error occurs (caught by @logger.catch), functions return None instead of raising exceptions. This breaks type contracts and standard Python error handling patterns.

Example

# Function signature suggests it always returns tuple[Model, bool]
tenant, existed = await get_one_or_create(session, Tenant, slug="test")

# But on error, it returns None, causing:
# TypeError: cannot unpack non-iterable NoneType object

Expected Behavior

Python functions should either:

1. Return the expected type (tuple[Model, bool])
2. Raise an exception with clear error information

Actual Behavior

Returns None on any error, breaking unpacking and type expectations.

Impact

Medium. Makes the library incompatible with type checkers (mypy, pyright) and standard error handling patterns.

---

Proposed Solutions

1. Remove @logger.catch Decorator

For a library, it's better to let the caller handle exceptions:

# Instead of:
@logger.catch
async def get_one_or_create(...):
    ...

# Use:
async def get_one_or_create(...):
    """
    Raises:
        IntegrityError: If unique constraint is violated
        SQLAlchemyError: For other database errors
    """
    # Let exceptions propagate naturally

If logging is desired, log BEFORE re-raising:

async def get_one_or_create(...):
    try:
        # ... implementation ...
    except Exception as e:
        logger.error(f"Error in get_one_or_create: {e}")
        raise  # Re-raise the exception

2. Add Separate Match Fields Parameter

async def get_one_or_create(
    session: AsyncSession,
    model: Type[SQLModel],
    match_fields: dict[str, Any],
    create_fields: dict[str, Any] | None = None,
) -> tuple[SQLModel, bool]:
    """
    Get or create a record.

    Args:
        match_fields: Fields to use in SELECT query
        create_fields: Additional fields for INSERT (merged with match_fields)

    Returns:
        tuple[model_instance, existed]: Model instance and whether it existed

    Raises:
        IntegrityError: If unique constraint violated
        SQLAlchemyError: For other database errors
    """
    # Filter only on match_fields
    result = await session.execute(
        select(model).filter_by(**match_fields)
    )
    instance = result.scalar_one_or_none()

    if instance:
        return instance, True

    # Create with both match and create fields
    all_fields = {**match_fields, **(create_fields or {})}
    instance = model(**all_fields)
    session.add(instance)
    await session.flush()
    return instance, False

3. Update Type Hints and Documentation

from typing import TypeVar

T = TypeVar('T', bound=SQLModel)

async def get_one_or_create(
    session: AsyncSession,
    model: Type[T],
    match_fields: dict[str, Any],
    create_fields: dict[str, Any] | None = None,
) -> tuple[T, bool]:  # Clear return type, never None
    """
    Get or create a record.

    Example:
        tenant, existed = await get_one_or_create(
            session,
            Tenant,
            match_fields={"slug": "acme"},
            create_fields={"name": "ACME Corp", "active": True}
        )

        if existed:
            print(f"Found existing tenant: {tenant.name}")
        else:
            print(f"Created new tenant: {tenant.name}")
    """

---

Workaround (Current)

For now, we're using standard SQLAlchemy patterns instead:

async def get_or_create_tenant(
    session: AsyncSession,
    slug: str,
    **create_fields
) -> tuple[Tenant, bool]:
    """Get or create tenant. Returns (instance, existed)."""
    result = await session.execute(
        select(Tenant).where(Tenant.slug == slug)
    )
    tenant = result.scalar_one_or_none()

    if tenant:
        return tenant, True

    tenant = Tenant(slug=slug, **create_fields)
    session.add(tenant)
    await session.flush()
    return tenant, False

This is more verbose but explicit, type-safe, and production-ready.

---

Additional Context

We really appreciate this library and want to use it! The bulk operations (bulk_upsert_mappings) look very promising. However, the current error handling and filtering behavior makes it risky for production use.

If these issues can be addressed, we'd be happy to:

• Continue using the library in our production CMS
• Contribute test cases for these edge cases
• Help with documentation improvements

Thank you for maintaining this library! 🙏

---

Reproduction Repository

We can provide a minimal reproduction repository if needed. The issues are reproducible with:

1. Any SQLModel with unique constraints
2. Any attempt to create records with multiple fields
3. Any error condition (network, constraint violation, etc.)

---

Related Issues

• Similar to common concerns around blanket exception handling in libraries
• Related to SQLAlchemy's recommended patterns for get-or-create
• Type safety concerns with None returns

---

Checklist

• Issue affects production use cases
• Workaround exists but is verbose
• Proposed solutions provided
• Willing to contribute fixes if guidance provided
• Minimal reproduction example (can provide if requested)