Skip to content

Conversation

@YonesSohrabi
Copy link

@YonesSohrabi YonesSohrabi commented Oct 7, 2025

Fixes

This PR fixes the following issues:


Summary

This PR fixes the issue where newly created MUC rooms have created_at = 1970-01-02 00:00:00 and adds proper separation between creation time and update time.

Problem

Currently:

  • ❌ New rooms get created_at = 1970-01-02 00:00:00
  • created_at changes on every room update (UPSERT behavior)
  • ❌ No way to track when room configuration was modified
  • ❌ Semantic confusion between hibernation_time and created_at

Solution

This PR:

  1. ✅ Adds updated_at column to muc_room table
  2. ✅ Ensures created_at is set once on creation and never changes
  3. ✅ Tracks room modifications with updated_at
  4. ✅ Separates concerns: created_atupdated_athibernation_time
  5. ✅ Adds hibernation_time support in API (optional)

Changes

Modified Files

src/mod_muc_sql.erl (lines 75-78, 142-185, 295)

  • Added updated_at column to schema
  • Modified store_room/5 to check if room exists:
    • Existing room: Preserve created_at, update updated_at
    • New room: Set both to current time
  • Removed hardcoded 1970-01-02 00:00:00 fallback
  • Fixed get_hibernated_rooms_older_than query

src/mod_muc_admin.erl (line 1734)

  • Added hibernation_time support in format_room_option
  • Allows API users to optionally set hibernation time

New Files

  • PULL_REQUEST_PROPOSAL.md - Detailed analysis with alternatives
  • MIGRATION_GUIDE.md - Step-by-step migration instructions
  • sql/migration_add_updated_at.sql - SQL migration script

Database Migration Required

-- MySQL/MariaDB
ALTER TABLE muc_room ADD COLUMN updated_at timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP;
UPDATE muc_room SET updated_at = created_at;

-- PostgreSQL
ALTER TABLE muc_room ADD COLUMN updated_at timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP;
UPDATE muc_room SET updated_at = created_at;

See MIGRATION_GUIDE.md for complete instructions for all databases.

Testing

Before This PR

# Create room
curl -X POST /api/create_room_with_opts -d '{...}'

# Check database
SELECT name, created_at FROM muc_room WHERE name='testroom';
# Result: created_at = 1970-01-02 00:00:00 ❌

After This PR

# Create room
curl -X POST /api/create_room_with_opts -d '{...}'

# Check database
SELECT name, created_at, updated_at FROM muc_room WHERE name='testroom';
# Result: 
# created_at = 2025-01-07 12:34:56 ✅
# updated_at = 2025-01-07 12:34:56 ✅

# Update room configuration
ejabberdctl change_room_option testroom conference.localhost title "New Title"

# Check database again
SELECT name, created_at, updated_at FROM muc_room WHERE name='testroom';
# Result:
# created_at = 2025-01-07 12:34:56 ✅ (unchanged!)
# updated_at = 2025-01-07 13:45:00 ✅ (updated!)

Backward Compatibility

Fully backward compatible with migration script
No breaking changes to API
Existing rooms work without changes
Old rooms can be updated with migration script

Benefits

  1. Accurate timestamps: Room creation time is correct
  2. Track changes: Know when room configuration was modified
  3. Better analytics: Proper data for reporting
  4. Clear semantics: Separate creation, update, and hibernation concepts
  5. Industry standard: Follows common created_at + updated_at pattern

Documentation

  • PULL_REQUEST_PROPOSAL.md: Detailed problem analysis and alternative solutions
  • MIGRATION_GUIDE.md: Complete migration instructions with troubleshooting
  • sql/migration_add_updated_at.sql: Ready-to-use SQL migration scripts

Checklist

  • Code follows ejabberd style guidelines
  • Changes are backward compatible
  • Migration guide included
  • SQL migration scripts included
  • Documentation updated
  • No breaking changes to API
  • Tested manually (see Testing section)

Related Files

  • src/mod_muc_sql.erl - Main changes
  • src/mod_muc_admin.erl - API support
  • PULL_REQUEST_PROPOSAL.md - Detailed proposal
  • MIGRATION_GUIDE.md - Migration instructions

Copy link
Member

@prefiks prefiks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see two things that require change:

  • sql files in sql/ will need to be update with that new column definitions
  • store_room can be simplified by just using special syntax in in upsert statement (check by comment near this code).

"!host=%(Host)s",
"server_host=%(LServer)s",
"opts=%(SOpts)s",
"created_at=%(CreatedAt)t",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you can replace that whole conditional code by just having single upsert with "-created_at=..." instead of "created_at=...", having "-" at start will only make that field be set when value is inserted.

@YonesSohrabi YonesSohrabi force-pushed the fix-muc-room-timestamps branch from 1803f80 to 644a79e Compare November 15, 2025 06:59
@YonesSohrabi
Copy link
Author

Hi @Neustradamus and @prefiks ,

Thank you for the feedback! I've updated the PR based on the review comments:

Changes Made

1. Simplified store_room/5 using UPSERT special syntax

  • Replaced conditional logic with -created_at syntax
  • This uses ejabberd's built-in feature where -field means "only set on INSERT, not on UPDATE"
  • Reduced code from ~30 lines to ~10 lines
  • Improved performance (one less query)

**Before:ang
case ejabberd_sql:sql_query_t(...) of
{selected, [{CreatedAt}]} ->
?SQL_UPSERT_T("muc_room", [..., "created_at=%(CreatedAt)t", ...]);
_ ->
?SQL_UPSERT_T("muc_room", [..., "created_at=%(CurrentTime)t", ...])
end
After:**ang
?SQL_UPSERT_T(
"muc_room",
["!name=%(Name)s",
"!host=%(Host)s",
"server_host=%(LServer)s",
"opts=%(SOpts)s",
"-created_at=%(CurrentTime)t", % Only set on INSERT
"updated_at=%(CurrentTime)t"]) % Set on both INSERT and UPDATE### 2. Updated SQL schema files

  • sql/mysql.new.sql - Added updated_at column
  • sql/pg.new.sql - Added updated_at column
  • sql/lite.new.sql - Added updated_at column
  • sql/mssql.new.sql - Added updated_at column

Related Issues

This PR fixes:

Testing

Tested manually:

  1. ✅ New room creation: created_at and updated_at set correctly
  2. ✅ Room update: created_at preserved, updated_at updated
  3. ✅ No breaking changes to API
  4. ✅ Migration script works on existing databases

Please let me know if there are any other changes needed!

@YonesSohrabi YonesSohrabi requested a review from prefiks November 15, 2025 07:10
- Add updated_at column to muc_room table schema
- Use UPSERT special syntax (-created_at) to preserve created_at on updates
- Set created_at to current time for new rooms (not 1970-01-02)
- Add hibernation_time support in format_room_option
- Update SQL files: mysql.new.sql, pg.new.sql, lite.new.sql, mssql.new.sql
- Include migration guide and SQL scripts

This fixes the issue where newly created MUC rooms have
created_at = 1970-01-02 00:00:00 instead of actual creation time.

The solution:
- Uses ejabberd's UPSERT special syntax where '-field' means
  'only set on INSERT, not on UPDATE'
- Adds updated_at column to properly track room modifications
- Separates concerns: created_at vs updated_at vs hibernation_time
@YonesSohrabi YonesSohrabi force-pushed the fix-muc-room-timestamps branch from 644a79e to ec12913 Compare November 15, 2025 09:16
@YonesSohrabi
Copy link
Author

@Neustradamus Thank you for the feedback!

I've updated the PR:

Updated all SQL files:

  • sql/mysql.sql
  • sql/mysql.new.sql
  • sql/pg.sql
  • sql/pg.new.sql
  • sql/lite.sql
  • sql/lite.new.sql
  • sql/mssql.sql
  • sql/mssql.new.sql

Added issue references:

Simplified code using -created_at UPSERT syntax

All SQL schema files now include the updated_at column. The PR is ready for review!

@coveralls
Copy link

Coverage Status

coverage: 33.788% (+0.005%) from 33.783%
when pulling ec12913 on YonesSohrabi:fix-muc-room-timestamps
into 71dcced on processone:master.

@prefiks
Copy link
Member

prefiks commented Nov 18, 2025

I will try to review this today

@p1bot
Copy link
Collaborator

p1bot commented Nov 18, 2025

Hi @YonesSohrabi, many thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes.

@p1bot p1bot added the cla-missing Contributor needs to sign Contribution License Agreement label Nov 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-missing Contributor needs to sign Contribution License Agreement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MUC room created_at timestamp incorrectly set to 1970-01-02 Room creation time is not properly set in DB

4 participants