Skip to content

Handle ObservationSummary dict is None#898

Merged
dvida merged 20 commits into
CroatianMeteorNetwork:prereleasefrom
g7gpr:fix/ObservationSummary/NoneDict
Jul 1, 2026
Merged

Handle ObservationSummary dict is None#898
dvida merged 20 commits into
CroatianMeteorNetwork:prereleasefrom
g7gpr:fix/ObservationSummary/NoneDict

Conversation

@g7gpr

@g7gpr g7gpr commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

Avoid iterating over ObservationSummary dict when dict is None

@g7gpr g7gpr requested a review from Cybis320 June 21, 2026 11:35
@g7gpr g7gpr self-assigned this Jun 21, 2026
@g7gpr g7gpr marked this pull request as ready for review June 22, 2026 20:56
A working/final observation_summary JSON whose content is the literal
'null' parses to None and was returned as-is, crashing every consumer
(addObsParam, saveObservationSummaryDict, storeDictInDB). Treat a
non-dict parse result like a corrupt file: back it up and reset.

Also move the None guard in storeDictInDB above addRequiredColumns so
it short-circuits before use, and fix two typos in the log messages.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@dvida

dvida commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

@g7gpr Could you test this latest version with my modifications?

@g7gpr

g7gpr commented Jun 28, 2026

Copy link
Copy Markdown
Contributor Author

Incoporated pr #910, from issue #739, with thanks to @markmac99

Retesting overnight on au0004,6,7

@g7gpr

g7gpr commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

This needed a bit more work to catch the edge case where a key that was not a valid column name was not used to create a column, but then tried to write into a column with that name. Also incorporated features from issue #739. However, the changes by @dvida are good.

@g7gpr

g7gpr commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Tested and working on three stations. Ready for further review / merge.

addRequiredColumns creates columns lower-cased, so existing_columns only
ever holds lower-case names. The dict filter compared the original-case key,
so mixed-case keys such as "stationID" were silently dropped and never
inserted, leaving the stationid column NULL.

Match case-insensitively and key the filtered dict by the lower-cased name.
Also ungate the dropped-keys warning so real data loss is logged, not hidden
behind the debug flag.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@dvida

dvida commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Reviewed this — one correctness regression worth flagging, which I've pushed a fix for (98c1eb8).

storeDictInDB silently drops stationID (and any mixed-case key).

dict_filtered_by_columns = {k: v for k, v in d.items() if k in existing_columns}

Columns are always created lower-cased (addRequiredColumnsADD COLUMN {key.lower()}), so existing_columns only ever contains lower-case names. The new filter compares the original-case key, so stationID (added at addObsParam(d, "stationID", ...)) is filtered out and never inserted — the stationid column ends up always NULL. Pre-PR code inserted d.items() directly and relied on SQLite's case-insensitive column matching, so it stored fine. This is a data-loss regression for the station identifier.

Fix matches case-insensitively and keys the filtered dict by the lower-cased name:

dict_filtered_by_columns = {k.lower(): v for k, v in d.items() if k.lower() in existing_columns}

Verified in isolation: old filter drops stationID, new one keeps it.

Also ungated the dropped-keys log.warning (was behind debug), so genuine data loss is visible in normal logs rather than silent.

Everything else checked out: the timestampFromNTP 3-tuple change is consistent with its only caller, the None-dict guards are sound, and the non-dict-JSON rejection is a good hardening.

@g7gpr

g7gpr commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Agree with these changes, penultimate commit introduced an error, which latest commit has corrected.

(vRMS) au0004@birch:~/source/RMS$ sqlite3 /srv/rms/RMS_data/AU0004/observation.db -header -column "select commit_date, stationid from observations order by commit_date desc limit 4;"
commit_date      stationid
---------------  ---------
20260629_151259  AU0004   
20260628_020646           
20260628_020646           
20260627_031825  AU0004   

I'm testing on au0004,6,7; there should be enough night left to capture a small amount of data.

@g7gpr

g7gpr commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

All looks good now.

Post-review nits (non-functional):
- .config: use ';' comment prefix for time_server, matching file convention.
- parseSystem: read time_server once instead of calling parser.get twice.
- addRequiredColumns: return set() (not None) in the d-is-None branch, so the
  documented set contract holds even though the caller guards d is None first.
- storeDictInDB docstring typo: pring -> print.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@dvida dvida merged commit e7351f4 into CroatianMeteorNetwork:prerelease Jul 1, 2026
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.

2 participants