fix(data): fall back to time.Now() for non-xid BaseModel IDs#648
fix(data): fall back to time.Now() for non-xid BaseModel IDs#648
Conversation
…d IDs Non-xid BaseModel IDs (typically test fixtures or legacy hand-crafted migration seeds) can no longer derive a deterministic CreatedAt from the id's embedded timestamp. Rather than blocking the insert, log a WARN and fall back to time.Now(). createdAtFromID still returns an error so callers that want strict behaviour — or want to surface the violation — can do so. Production IDs always come from util.IDString() and take the hot path.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the BeforeCreate hook in data/model.go to allow the use of non-xid IDs, which now trigger a fallback to time.Now() for the CreatedAt timestamp instead of returning an error. This change is aimed at supporting test fixtures and legacy migrations. Tests in datastore/repository_test.go have been updated accordingly. Review feedback suggests simplifying the warning log message to avoid redundancy, as the ID and error details are already included as log fields.
| util.Log(db.Statement.Context). | ||
| WithError(err). | ||
| WithField("id", model.ID). | ||
| Warn("BaseModel.ID is not a valid xid; falling back to time.Now() for CreatedAt") |
There was a problem hiding this comment.
The log message is redundant because the id and the validation error are already included in the log fields via WithField("id", model.ID) and WithError(err). Simplifying the message improves log clarity and reduces verbosity.
| util.Log(db.Statement.Context). | |
| WithError(err). | |
| WithField("id", model.ID). | |
| Warn("BaseModel.ID is not a valid xid; falling back to time.Now() for CreatedAt") | |
| util.Log(db.Statement.Context). | |
| WithError(err). | |
| WithField("id", model.ID). | |
| Warn("falling back to time.Now() for CreatedAt") |
1 participant
Summary
Follow-up to v1.94.2. The strict xid check in BeforeCreate broke existing
tests and hand-crafted migration seeds that use readable non-xid IDs like
`"client-1"` or `"test-event-123"`. Those are ergonomic fixtures — not
a production pattern — and blocking inserts on them makes frame upgrades
painful for downstream repos.
Now: `createdAtFromID` still returns an error for non-xid IDs, but
`BeforeCreate` catches it, logs a WARN, and falls back to time.Now().
Production IDs always take the xid path (because util.IDString produces
xids) so the sort-by-id ≡ sort-by-created_at guarantee still holds in
practice.
Test plan