fix(review): address PR #94 review #37/#38 reviewer comments
Parser (src/database_flatfile_parser.c):
- ff_jid_to_dir: drop GString+step1, in-place mutate str_replace result
- ff_ensure_dir: g_file_test(G_FILE_TEST_IS_SYMLINK) instead of g_lstat
- ff_unescape_*: early-continue for non-escape branch (less nesting)
- ff_readline: strdup("") instead of malloc(1)+'\0' for skip-line return
- ff_write_line: auto_char safe_msg + single fprintf (drop GString full_line)
- ff_parse_line: early return on missing closing bracket;
extract metadata-parts loop into _ff_parse_meta_parts helper
Verify (src/database_flatfile_verify.c):
- Split monolithic ff_verify_integrity into _collect_contact_dirs,
_check_permissions, _first_pass, _lmc_pass, _verify_contact_dir
- _issue_new constructor; g_new0 throughout
- Separate seen_stanza_ids and seen_archive_ids tables (no cross-kind
duplicate confusion)
- ff_skip_bom in second pass (drop manual fgetc x3)
- log_debug when skipping a contact dir without history.log
- Doc comment on ff_verify_integrity in header
SQLite (src/database_sqlite.c):
- Doc block on ChatLogs schema (replaces_db_id <-> replaced_by_db_id
invariant maintained by AFTER INSERT trigger)
- log_error when _get_db_filename returns NULL during init
Flatfile dispatch (src/database_flatfile.c):
- Replace if/else with is_outgoing ternary for log-path contact pick
Tests:
- 6 new invalid-date parser tests: empty / partial ISO / garbage /
impossible calendar / negative year / far future timestamp
- New functional test message_db_history_multi_resource verifying
same-JID-different-resource history rehydration preserves resources
- test_database_stress.c LMC chain loop: auto_char buf, drop redundant
comment/empty pre-filter (ff_parse_line handles those)
This commit is contained in:
@@ -234,6 +234,7 @@ main(int argc, char* argv[])
|
||||
PROF_FUNC_TEST(message_db_history_service_chars),
|
||||
PROF_FUNC_TEST(message_db_history_verify),
|
||||
PROF_FUNC_TEST(message_db_history_lmc),
|
||||
PROF_FUNC_TEST(message_db_history_multi_resource),
|
||||
|
||||
/* Basic message send/receive */
|
||||
PROF_FUNC_TEST(message_send),
|
||||
|
||||
@@ -472,3 +472,67 @@ message_db_history_lmc(void** state)
|
||||
assert_false(prof_output_exact("lmc-before-correction-aaa"));
|
||||
prof_timeout_reset();
|
||||
}
|
||||
|
||||
/*
|
||||
* Test: messages from the SAME bare JID but with DIFFERENT resources are
|
||||
* stored together in one contact's history AND remain distinguishable on
|
||||
* reopen — the resource part of the sender JID must be preserved per row,
|
||||
* not collapsed.
|
||||
*
|
||||
* Flow:
|
||||
* 1. /history on
|
||||
* 2. buddy1@localhost/phone sends msg A
|
||||
* 3. buddy1@localhost/laptop sends msg B
|
||||
* 4. buddy1@localhost/tablet sends msg C
|
||||
* 5. Close the auto-opened chat window so its in-memory buffer is gone.
|
||||
* 6. /msg buddy1@localhost — chatwin_new() rehydrates from DB.
|
||||
* 7. All three bodies must reappear, AND the resource markers
|
||||
* "/phone", "/laptop", "/tablet" must each be visible on at least one
|
||||
* history line (PREF_RESOURCE_MESSAGE is on by default in the test
|
||||
* profile, so the renderer prefixes each line with "Buddy1/<res>").
|
||||
*/
|
||||
void
|
||||
message_db_history_multi_resource(void** state)
|
||||
{
|
||||
prof_connect();
|
||||
|
||||
prof_input("/history on");
|
||||
assert_true(prof_output_regex("Chat history enabled\\."));
|
||||
|
||||
stbbr_send(
|
||||
"<message id='res-multi-A' to='stabber@localhost' "
|
||||
"from='buddy1@localhost/phone' type='chat'>"
|
||||
"<body>multi-resource-A-from-phone</body>"
|
||||
"</message>");
|
||||
assert_true(prof_output_exact("<< chat message: Buddy1/phone (win 2)"));
|
||||
|
||||
stbbr_send(
|
||||
"<message id='res-multi-B' to='stabber@localhost' "
|
||||
"from='buddy1@localhost/laptop' type='chat'>"
|
||||
"<body>multi-resource-B-from-laptop</body>"
|
||||
"</message>");
|
||||
assert_true(prof_output_exact("<< chat message: Buddy1/laptop (win 2)"));
|
||||
|
||||
stbbr_send(
|
||||
"<message id='res-multi-C' to='stabber@localhost' "
|
||||
"from='buddy1@localhost/tablet' type='chat'>"
|
||||
"<body>multi-resource-C-from-tablet</body>"
|
||||
"</message>");
|
||||
assert_true(prof_output_exact("<< chat message: Buddy1/tablet (win 2)"));
|
||||
|
||||
prof_input("/close 2");
|
||||
assert_true(prof_output_exact("Closed window 2"));
|
||||
|
||||
prof_input("/msg buddy1@localhost");
|
||||
|
||||
/* All three bodies must be present in history (single-contact aggregation) */
|
||||
assert_true(prof_output_exact("multi-resource-A-from-phone"));
|
||||
assert_true(prof_output_exact("multi-resource-B-from-laptop"));
|
||||
assert_true(prof_output_exact("multi-resource-C-from-tablet"));
|
||||
|
||||
/* Each resource must remain identifiable on rehydrate — proves we don't
|
||||
* lose per-row resource info when loading the same JID's chat back. */
|
||||
assert_true(prof_output_regex("Buddy1/phone"));
|
||||
assert_true(prof_output_regex("Buddy1/laptop"));
|
||||
assert_true(prof_output_regex("Buddy1/tablet"));
|
||||
}
|
||||
|
||||
@@ -10,3 +10,4 @@ void message_db_history_newline(void** state);
|
||||
void message_db_history_service_chars(void** state);
|
||||
void message_db_history_verify(void** state);
|
||||
void message_db_history_lmc(void** state);
|
||||
void message_db_history_multi_resource(void** state);
|
||||
|
||||
@@ -500,6 +500,66 @@ test_ff_parse_line_invalid_timestamp(void** state)
|
||||
assert_null(pl);
|
||||
}
|
||||
|
||||
void
|
||||
test_ff_parse_line_empty_timestamp(void** state)
|
||||
{
|
||||
/* The space before "[" makes the timestamp empty -> g_date_time_new_from_iso8601
|
||||
must reject it. */
|
||||
ff_parsed_line_t* pl = ff_parse_line(
|
||||
" [chat|none] a@b.com: msg");
|
||||
assert_null(pl);
|
||||
}
|
||||
|
||||
void
|
||||
test_ff_parse_line_partial_iso_timestamp(void** state)
|
||||
{
|
||||
/* Year-only or yyyy-mm without time portion is not ISO-8601 enough for
|
||||
g_date_time_new_from_iso8601 — must fail rather than silently succeed. */
|
||||
assert_null(ff_parse_line("2025 [chat|none] a@b.com: msg"));
|
||||
assert_null(ff_parse_line("2025-06-15 [chat|none] a@b.com: msg"));
|
||||
}
|
||||
|
||||
void
|
||||
test_ff_parse_line_garbage_timestamp(void** state)
|
||||
{
|
||||
/* Random characters where timestamp should be — parse failure, no segfault. */
|
||||
assert_null(ff_parse_line("@@@@@@ [chat|none] a@b.com: msg"));
|
||||
assert_null(ff_parse_line("12345 [chat|none] a@b.com: msg"));
|
||||
}
|
||||
|
||||
void
|
||||
test_ff_parse_line_impossible_calendar_date(void** state)
|
||||
{
|
||||
/* Day 32 / Month 13 — well-formed ISO syntax but impossible date,
|
||||
g_date_time_new_from_iso8601 returns NULL. */
|
||||
assert_null(ff_parse_line(
|
||||
"2025-13-01T12:00:00Z [chat|none] a@b.com: msg"));
|
||||
assert_null(ff_parse_line(
|
||||
"2025-06-32T12:00:00Z [chat|none] a@b.com: msg"));
|
||||
assert_null(ff_parse_line(
|
||||
"2025-06-15T25:00:00Z [chat|none] a@b.com: msg"));
|
||||
}
|
||||
|
||||
void
|
||||
test_ff_parse_line_negative_year_timestamp(void** state)
|
||||
{
|
||||
/* Negative year is not parseable by g_date_time_new_from_iso8601. */
|
||||
assert_null(ff_parse_line(
|
||||
"-0001-06-15T12:00:00Z [chat|none] a@b.com: msg"));
|
||||
}
|
||||
|
||||
void
|
||||
test_ff_parse_line_far_future_timestamp(void** state)
|
||||
{
|
||||
/* Year 9999 is valid ISO-8601 and should parse — sanity check that we
|
||||
don't artificially reject valid far-future dates. */
|
||||
ff_parsed_line_t* pl = ff_parse_line(
|
||||
"9999-12-31T23:59:59Z [chat|none] a@b.com: hi");
|
||||
assert_non_null(pl);
|
||||
assert_non_null(pl->timestamp);
|
||||
ff_parsed_line_free(pl);
|
||||
}
|
||||
|
||||
/* ================================================================
|
||||
* Type / enc conversion round-trip
|
||||
* ================================================================ */
|
||||
|
||||
@@ -34,6 +34,12 @@ void test_ff_parse_line_comment(void** state);
|
||||
void test_ff_parse_line_legacy_format(void** state);
|
||||
void test_ff_parse_line_no_metadata(void** state);
|
||||
void test_ff_parse_line_invalid_timestamp(void** state);
|
||||
void test_ff_parse_line_empty_timestamp(void** state);
|
||||
void test_ff_parse_line_partial_iso_timestamp(void** state);
|
||||
void test_ff_parse_line_garbage_timestamp(void** state);
|
||||
void test_ff_parse_line_impossible_calendar_date(void** state);
|
||||
void test_ff_parse_line_negative_year_timestamp(void** state);
|
||||
void test_ff_parse_line_far_future_timestamp(void** state);
|
||||
|
||||
/* type/enc conversion round-trip */
|
||||
void test_ff_type_str_roundtrip(void** state);
|
||||
|
||||
@@ -1106,15 +1106,11 @@ test_stress_lmc_many_corrections(void** state)
|
||||
int corrections_found = 0;
|
||||
|
||||
while (1) {
|
||||
char* buf = ff_readline(fp, NULL);
|
||||
auto_char char* buf = ff_readline(fp, NULL);
|
||||
if (!buf)
|
||||
break;
|
||||
if (buf[0] == '#' || buf[0] == '\0') {
|
||||
free(buf);
|
||||
continue;
|
||||
}
|
||||
// ff_parse_line already rejects empty/comment lines, no need to pre-filter.
|
||||
ff_parsed_line_t* pl = ff_parse_line(buf);
|
||||
free(buf);
|
||||
if (pl) {
|
||||
total_parsed++;
|
||||
if (pl->replace_id && strlen(pl->replace_id) > 0)
|
||||
|
||||
@@ -692,6 +692,12 @@ main(int argc, char* argv[])
|
||||
cmocka_unit_test(test_ff_parse_line_legacy_format),
|
||||
cmocka_unit_test(test_ff_parse_line_no_metadata),
|
||||
cmocka_unit_test(test_ff_parse_line_invalid_timestamp),
|
||||
cmocka_unit_test(test_ff_parse_line_empty_timestamp),
|
||||
cmocka_unit_test(test_ff_parse_line_partial_iso_timestamp),
|
||||
cmocka_unit_test(test_ff_parse_line_garbage_timestamp),
|
||||
cmocka_unit_test(test_ff_parse_line_impossible_calendar_date),
|
||||
cmocka_unit_test(test_ff_parse_line_negative_year_timestamp),
|
||||
cmocka_unit_test(test_ff_parse_line_far_future_timestamp),
|
||||
|
||||
// Type/enc round-trip
|
||||
cmocka_unit_test(test_ff_type_str_roundtrip),
|
||||
|
||||
Reference in New Issue
Block a user