Skip to content

Fix: Fix status code 500 when marks create - #14

Merged
WinerTy merged 1 commit into
mainfrom
dev-winerty
Dec 3, 2025
Merged

Fix: Fix status code 500 when marks create#14
WinerTy merged 1 commit into
mainfrom
dev-winerty

Conversation

@WinerTy

@WinerTy WinerTy commented Dec 3, 2025

Copy link
Copy Markdown
Collaborator

No description provided.

@WinerTy
WinerTy merged commit f954ef2 into main Dec 3, 2025
1 check passed
@claude

claude Bot commented Dec 3, 2025

Copy link
Copy Markdown

PR Review: Fix status code 500 when marks create

Summary

This PR fixes a critical bug in the BaseMark schema where default_factory=datetime.now() was incorrectly calling the function immediately instead of passing a callable reference. ✅

Code Quality: Excellent ⭐

What's Fixed

The change from:

Field(default_factory=datetime.now(), description="Current date")

to:

Field(default_factory=datetime.now, description="Current date")

This is exactly correct. The issue was that datetime.now() was being called once at import time, which would:

  1. Set the same timestamp for ALL mark instances created during the application lifetime
  2. Potentially cause serialization/validation errors that resulted in the 500 error

Technical Analysis

Root Cause: Pydantic's default_factory expects a callable (a function reference), not the result of calling a function. By including the parentheses (), the function was executed immediately at class definition time.

Impact: This bug affected the CreateMark schema (via inheritance from BaseMark) used in the POST /marks/ endpoint at realtimemap/api/v1/mark/view.py:122-149.

Correctness: The fix follows Pydantic best practices. Now datetime.now will be called each time a new mark instance is created, giving each mark the correct current timestamp.

Potential Issues: None ✅

No issues found. The fix is minimal, targeted, and correct.

Performance Considerations: Improved ✅

Before: All marks would have the same start_at timestamp (the application start time), which could cause:

  • Incorrect mark expiration calculations
  • Data integrity issues
  • Potential query performance problems due to duplicate timestamps

After: Each mark gets the accurate creation timestamp, ensuring correct behavior.

Security Concerns: None ✅

No security issues. This is a pure bug fix with no security implications.

Test Coverage: ⚠️ Recommendation

While the fix is correct, I notice there are no test files specifically for the mark creation functionality. Consider adding tests for:

  1. Unit test for BaseMark schema validation:
def test_base_mark_default_timestamp():
    """Ensure each mark instance gets a unique timestamp"""
    mark1 = BaseMark(mark_name="Test 1")
    time.sleep(0.1)
    mark2 = BaseMark(mark_name="Test 2")
    assert mark1.start_at < mark2.start_at
  1. Integration test for the create mark endpoint to prevent regression of this 500 error

However, this is a suggestion for future improvement and should not block this PR.

Recommendation: APPROVE

This is a clean, well-targeted bug fix that resolves a critical issue. The change is minimal and follows Python/Pydantic best practices.

Excellent work identifying and fixing this subtle but important bug! 🎯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant