Wire household creation on registration and fix datetime handling#6
Conversation
There was a problem hiding this comment.
Summary
This PR implements household creation on user registration — a key piece of your multi-tenant architecture. The code shows solid understanding of SQLAlchemy patterns (flush() for ID generation, timezone-aware datetimes), and the operational details like pool_pre_ping for Neon are excellent.
Critical issue: The on_after_register hook has a transaction safety problem. Because it runs after the user is already committed, a failure during household creation will leave an orphaned user with no household. See my inline comment for how to fix this by overriding create() instead.
What's working well:
- ✅ Foreign key fix (
"households.id"vs"household.id") - ✅ Timezone-aware datetime columns — critical for any production app
- ✅
pool_pre_ping=Truefor Neon's connection behavior - ✅ Good use of
flush()to get the generated ID mid-transaction - ✅ Helpful comment about FastAPI dependency caching
Minor improvements:
- Add a fallback for edge cases in email parsing (see inline)
- Remove the redundant
self.session.add(user)after modifyinghousehold_id
The attention to database details (foreign keys, timezones, connection pooling) is really strong. Once the transaction issue is fixed, this will be solid foundational code for the rest of the multi-household features.
Keep up the good work!
| await self.session.flush() | ||
| user.household_id = household.id | ||
| self.session.add(user) | ||
| await self.session.commit() |
There was a problem hiding this comment.
Critical: Transaction rollback issue
This hook runs after fastapi-users has already committed the user to the database. If the household creation or assignment fails (e.g., database constraint violation, connection issue), you'll commit() will fail, but the user already exists in the database without a household_id.
This breaks your data model invariant that every user must belong to a household.
Better approach: Wrap the entire registration in a single transaction. You can do this by:
- Override
create()method inUserManagerinstead of usingon_after_register - Or handle the rollback: catch exceptions here and delete the orphaned user
Example using create():
async def create(
self,
user_create: schemas.UC,
safe: bool = False,
request: Request | None = None,
) -> User:
# Create user object but don't commit yet
user = await super().create(user_create, safe=safe, request=request)
# Now create household in the same transaction
household = Household(name=f"{user.email.split('@')[0]}'s household")
self.session.add(household)
await self.session.flush()
user.household_id = household.id
self.session.add(user)
return userThis way, if household creation fails, the entire transaction (including user creation) rolls back automatically.
There was a problem hiding this comment.
Suggested override has a similar issue. Implemented a compensating transaction instead: if household creation fails, we delete the orphaned user and re-raise the exception.
| ) -> None: | ||
| # Create a household and assign it to the new user in one transaction. | ||
| # flush() writes the household row and generates its ID without | ||
| # committing, so both changes land together in a single commit. |
There was a problem hiding this comment.
Edge case: What if the email is malformed or starts with @?
"@example.com".split('@')[0] returns "", which would create a household named "'s household".
Suggestion: Add a fallback:
email_prefix = user.email.split('@')[0] or "user"
household = Household(name=f"{email_prefix}'s household")Or be more explicit:
prefix = user.email.split('@', 1)[0].strip() or "User"
household = Household(name=f"{prefix}'s Household")This is defensive programming — even though email validation should catch this earlier, models should be resilient to unexpected input.
There was a problem hiding this comment.
low priority because of fastApi email validation, applied regardless
| household = Household(name=f"{user.email.split('@')[0]}'s household") | ||
| self.session.add(household) | ||
| await self.session.flush() | ||
| user.household_id = household.id | ||
| self.session.add(user) |
There was a problem hiding this comment.
Good use of flush()!
This is the right pattern when you need a generated ID mid-transaction. flush() writes to the database and populates household.id without committing, so both changes are atomic.
One small note: you don't need the second self.session.add(user) — the user object is already tracked by the session. SQLAlchemy will automatically detect the change to user.household_id and include it in the commit.
So this can be simplified to:
household = Household(name=f"{user.email.split('@')[0]}'s household")
self.session.add(household)
await self.session.flush() # Generates household.id
user.household_id = household.id # Modify tracked object
await self.session.commit() # Commits both changes| # FastAPI caches dependencies per request, so session here is the same | ||
| # instance already open for get_user_db — no extra connection is opened. |
There was a problem hiding this comment.
Great comment! This explains a non-obvious behavior (dependency caching) that could confuse someone reading the code.
This is exactly the kind of comment that adds value — it documents a framework behavior that isn't immediately obvious from the code itself.
| # Nullable until the on_after_register hook creates and assigns a household. | ||
| household_id: int | None = Field( | ||
| default=None, foreign_key="household.id", index=True | ||
| default=None, foreign_key="households.id", index=True |
There was a problem hiding this comment.
Excellent catch!
The foreign key must reference the table name ("households") not the model name ("household"). This was a bug that would cause runtime errors when SQLModel tried to create the foreign key constraint.
Good attention to detail.
| created_at: datetime = Field( | ||
| default_factory=lambda: datetime.now(UTC), | ||
| sa_column=sa.Column(sa.DateTime(timezone=True), nullable=False), | ||
| ) |
There was a problem hiding this comment.
Excellent improvement!
Using sa.Column(sa.DateTime(timezone=True)) ensures the database column stores timezone-aware timestamps. This is critical for multi-timezone applications.
Without timezone=True, PostgreSQL stores timestamps as TIMESTAMP WITHOUT TIME ZONE, which loses timezone information and can cause subtle bugs when users are in different timezones.
With timezone=True, you get TIMESTAMP WITH TIME ZONE (PostgreSQL's timestamptz type), which normalizes everything to UTC internally but preserves timezone semantics.
This is a best practice worth carrying forward to any other datetime fields you add.
| engine = create_async_engine(DATABASE_URL, echo=settings.environment == "development") | ||
| # pool_pre_ping=True tests each connection before use and reconnects if Neon | ||
| # closed it due to inactivity (Neon drops idle connections after ~5 minutes). | ||
| engine = create_async_engine(DATABASE_URL, echo=settings.environment == "development", pool_pre_ping=True) |
There was a problem hiding this comment.
Smart addition for Neon!
pool_pre_ping=True is essential when using serverless databases like Neon that drop idle connections. SQLAlchemy will test each connection with a simple query (SELECT 1) before handing it to your code, and automatically reconnect if it's stale.
The performance overhead is minimal (microseconds), and it prevents cryptic "connection closed" errors in production.
Good operational awareness — this shows you're thinking about how the code will behave in a real deployment environment, not just on your dev machine.
| created_at: datetime = Field( | ||
| default_factory=lambda: datetime.now(UTC), | ||
| sa_column=sa.Column(sa.DateTime(timezone=True), nullable=False), | ||
| ) |
There was a problem hiding this comment.
Same timezone-aware improvement here — great consistency across models.
|
💰 Review cost: $0.10 |
Completes the registration flow — a user now gets a household assigned automatically on sign-up. Also fixes a datetime type mismatch that was causing all registrations to fail with a 500.
What's included
How to test
household row
households.id
What's next